On Fri, May 19, 2017 at 03:04:37PM -0500, Dave Gerlach wrote:
> AM335x and AM437x support various low power modes as documented
> in section 8.1.4.3 of the AM335x Technical Reference Manual and
> section 6.4.3 of the AM437x Technical Reference Manual.
>
> DeepSleep0 mode offers the lowest power mode with limited
> wakeup sources without a system reboot and is mapped as
> the suspend state in the kernel. In this state, MPU and
> PER domains are turned off with the internal RAM held in
> retention to facilitate the resume process. As part of
> the boot process, the assembly code is copied over to OCMCRAM
> so it can be executed to turn of the EMIF and put DDR into self
> refresh.
>
> Both platforms have a Cortex-M3 (WKUP_M3) which assists the MPU
> in DeepSleep0 entry and exit. WKUP_M3 takes care
> of the clockdomain and powerdomain transitions based on the
> intended low power state. MPU needs to load the appropriate
> WKUP_M3 binary onto the WKUP_M3 memory space before it can
> leverage any of the PM features like DeepSleep. This loading
> is handled by the remoteproc driver wkup_m3_rproc.
>
> Communication with the WKUP_M3 is handled by a wkup_m3_ipc
> driver that exposes the specific PM functionality to be used
> the PM code.
> +static void am33xx_pm_free_sram(void)
> +{
> + gen_pool_free(sram_pool, ocmcram_location, *pm_sram->do_wfi_sz);
> + gen_pool_free(sram_pool_data, ocmcram_location_data,
> + sizeof(struct am33xx_pm_ro_sram_data));
> +}
> +
> +/*
> + * Push the minimal suspend-resume code to SRAM
> + */
> +static int am33xx_prepare_push_sram_idle(void)
> +{
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, "ti,omap3-mpu");
> +
Stray newline.
> + if (!np) {
> + np = of_find_compatible_node(NULL, NULL, "ti,omap4-mpu");
> + if (!np) {
> + pr_warn("PM: %s: Unable to find device node for mpu\n",
> + __func__);
> + return -ENODEV;
> + }
> + }
You never put the reference to np you acquire here.
[snip]
> +static int am33xx_push_sram_idle(void)
> +{
> + struct am33xx_pm_ro_sram_data ro_sram_data;
> + int ret;
> + void *copy_addr;
> +
> + ro_sram_data.amx3_pm_sram_data_virt = ocmcram_location_data;
> + ro_sram_data.amx3_pm_sram_data_phys =
> + gen_pool_virt_to_phys(sram_pool_data, ocmcram_location_data);
> +
> + /* Save physical address to calculate resume offset during pm init */
> + am33xx_do_wfi_sram_phys = gen_pool_virt_to_phys(sram_pool,
> + ocmcram_location);
> +
> + am33xx_do_wfi_sram = sram_exec_copy(sram_pool, (void *)ocmcram_location,
> + pm_sram->do_wfi,
> + *pm_sram->do_wfi_sz);
> + if (!am33xx_do_wfi_sram) {
> + pr_err("PM: %s: am33xx_do_wfi copy to sram failed\n", __func__);
> + return -ENODEV;
> + }
> +
> + ret = ti_emif_copy_pm_function_table(sram_pool,
> + (void *)sram_suspend_address((unsigned long)pm_sram->emif_sram_table));
> + if (ret) {
> + pr_warn("PM: %s: EMIF function copy failed\n", __func__);
> + return -EPROBE_DEFER;
> + }
Here's the dependency to the emif device I commented on earlier (and
below).
> +
> + copy_addr = sram_exec_copy(sram_pool,
> + (void *)sram_suspend_address((unsigned long)pm_sram->ro_sram_data),
> + &ro_sram_data,
> + sizeof(ro_sram_data));
> + if (!copy_addr) {
> + pr_err("PM: %s: ro_sram_data copy to sram failed\n", __func__);
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> +static int am33xx_pm_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + int ret;
> +
> + if (!of_machine_is_compatible("ti,am33xx") &&
> + !of_machine_is_compatible("ti,am43"))
> + return -ENODEV;
> +
> + pm_ops = dev->platform_data;
> + if (!pm_ops) {
> + pr_err("PM: Cannot get core PM ops!\n");
> + return -ENODEV;
> + }
> +
> + pm_sram = pm_ops->get_sram_addrs();
> + if (!pm_sram) {
> + pr_err("PM: Cannot get PM asm function addresses!!\n");
> + return -ENODEV;
> + }
> +
> + ret = am33xx_prepare_push_sram_idle();
Perhaps calling this one am33xx_pm_alloc_sram() would be more
descriptive (and match the release function)?
> + if (ret)
> + return ret;
> +
> + ret = am33xx_push_sram_idle();
> + if (ret)
> + goto err_free_sram;
As I mentioned in my comments to the emif-sram driver, you may need to
create device link to the emif-sram device to prevent it from going away
under you here.
> +
> + m3_ipc = wkup_m3_ipc_get();
> + if (!m3_ipc) {
> + pr_err("PM: Cannot get wkup_m3_ipc handle\n");
You shouldn't log this as an error when probe is being deferred.
Why not use dev_err and friends for logging now that you have a struct
device?
And similarly to the emif-sram device, you may need to create a
device-link also to the ipc device to prevent its driver from being
unbound.
> + ret = -EPROBE_DEFER;
> + goto err_free_sram;
> + }
> +
> + am33xx_pm_set_ipc_ops();
> +
> +#ifdef CONFIG_SUSPEND
> + suspend_set_ops(&am33xx_pm_ops);
> +#endif /* CONFIG_SUSPEND */
This renders a lockdep splash about a circular locking dependency when
suspending since we're taking the pm_mutex in suspend_set_ops here, and
during suspend we flush any deferred probes while already holding the
mutex:
======================================================
WARNING: possible circular locking dependency detected
4.12.0-rc7 #11 Not tainted
------------------------------------------------------
bash/404 is trying to acquire lock:
(deferred_probe_work){+.+.+.}, at: [<c014cf3c>] flush_work+0x30/0x27c
but task is already holding lock:
(pm_mutex){+.+...}, at: [<c01792dc>] pm_suspend+0x190/0xc94
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (pm_mutex){+.+...}:
__mutex_lock+0x80/0x694
mutex_lock_nested+0x2c/0x34
suspend_set_ops+0x4c/0x128
am33xx_pm_probe+0x1fc/0x3a8
platform_drv_probe+0x5c/0xc0
driver_probe_device+0x37c/0x490
__device_attach_driver+0xac/0x128
bus_for_each_drv+0x74/0xa8
__device_attach+0xc4/0x154
device_initial_probe+0x1c/0x20
bus_probe_device+0x98/0xa0
deferred_probe_work_func+0x4c/0xe4
process_one_work+0x1f4/0x758
worker_thread+0x1e0/0x514
kthread+0x128/0x158
ret_from_fork+0x14/0x24
-> #0 (deferred_probe_work){+.+.+.}:
lock_acquire+0x108/0x264
flush_work+0x60/0x27c
wait_for_device_probe+0x24/0xa4
dpm_prepare+0xd0/0x91c
dpm_suspend_start+0x1c/0x70
suspend_devices_and_enter+0xc4/0xeac
pm_suspend+0x890/0xc94
state_store+0x80/0xdc
kobj_attr_store+0x1c/0x28
sysfs_kf_write+0x5c/0x60
kernfs_fop_write+0x128/0x254
__vfs_write+0x38/0x128
vfs_write+0xb4/0x174
SyS_write+0x54/0xb0
ret_fast_syscall+0x0/0x1c
Johan
On Mon, Jul 03, 2017 at 06:54:19PM +0200, Johan Hovold wrote:
> On Fri, May 19, 2017 at 03:04:37PM -0500, Dave Gerlach wrote:
> > +static int am33xx_pm_probe(struct platform_device *pdev)
> > +#ifdef CONFIG_SUSPEND
> > + suspend_set_ops(&am33xx_pm_ops);
> > +#endif /* CONFIG_SUSPEND */
>
> This renders a lockdep splash about a circular locking dependency when
> suspending since we're taking the pm_mutex in suspend_set_ops here, and
> during suspend we flush any deferred probes while already holding the
> mutex:
Here's the full splat against 4.12:
======================================================
WARNING: possible circular locking dependency detected
4.12.0 #30 Not tainted
------------------------------------------------------
bash/404 is trying to acquire lock:
(deferred_probe_work){+.+.+.}, at: [<c014cf3c>] flush_work+0x30/0x27c
but task is already holding lock:
(pm_mutex){+.+...}, at: [<c01792dc>] pm_suspend+0x190/0xc94
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (pm_mutex){+.+...}:
__mutex_lock+0x80/0x694
mutex_lock_nested+0x2c/0x34
suspend_set_ops+0x4c/0x128
am33xx_pm_probe+0x1fc/0x3a8
platform_drv_probe+0x5c/0xc0
driver_probe_device+0x37c/0x490
__device_attach_driver+0xac/0x128
bus_for_each_drv+0x74/0xa8
__device_attach+0xc4/0x154
device_initial_probe+0x1c/0x20
bus_probe_device+0x98/0xa0
deferred_probe_work_func+0x4c/0xe4
process_one_work+0x1f4/0x758
worker_thread+0x1e0/0x514
kthread+0x128/0x158
ret_from_fork+0x14/0x24
-> #0 (deferred_probe_work){+.+.+.}:
lock_acquire+0x108/0x264
flush_work+0x60/0x27c
wait_for_device_probe+0x24/0xa4
dpm_prepare+0xd0/0x91c
dpm_suspend_start+0x1c/0x70
suspend_devices_and_enter+0xc4/0xeac
pm_suspend+0x890/0xc94
state_store+0x80/0xdc
kobj_attr_store+0x1c/0x28
sysfs_kf_write+0x5c/0x60
kernfs_fop_write+0x128/0x254
__vfs_write+0x38/0x128
vfs_write+0xb4/0x174
SyS_write+0x54/0xb0
ret_fast_syscall+0x0/0x1c
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(pm_mutex);
lock(deferred_probe_work);
lock(pm_mutex);
lock(deferred_probe_work);
*** DEADLOCK ***
4 locks held by bash/404:
#0: (sb_writers#6){.+.+.+}, at: [<c0244fac>] vfs_write+0x160/0x174
#1: (&of->mutex){+.+.+.}, at: [<c02bab64>] kernfs_fop_write+0xe4/0x254
#2: (s_active#99){.+.+.+}, at: [<c02bab6c>] kernfs_fop_write+0xec/0x254
#3: (pm_mutex){+.+...}, at: [<c01792dc>] pm_suspend+0x190/0xc94
stack backtrace:
CPU: 0 PID: 404 Comm: bash Not tainted 4.12.0 #30
Hardware name: Generic AM33XX (Flattened Device Tree)
[<c011192c>] (unwind_backtrace) from [<c010e104>] (show_stack+0x20/0x24)
[<c010e104>] (show_stack) from [<c03f2da0>] (dump_stack+0x24/0x28)
[<c03f2da0>] (dump_stack) from [<c016e070>] (print_circular_bug+0x20c/0x334)
[<c016e070>] (print_circular_bug) from [<c0171660>] (__lock_acquire+0x1bf4/0x1c08)
[<c0171660>] (__lock_acquire) from [<c0172000>] (lock_acquire+0x108/0x264)
[<c0172000>] (lock_acquire) from [<c014cf6c>] (flush_work+0x60/0x27c)
[<c014cf6c>] (flush_work) from [<c04a7524>] (wait_for_device_probe+0x24/0xa4)
[<c04a7524>] (wait_for_device_probe) from [<c04bae20>] (dpm_prepare+0xd0/0x91c)
[<c04bae20>] (dpm_prepare) from [<c04bb688>] (dpm_suspend_start+0x1c/0x70)
[<c04bb688>] (dpm_suspend_start) from [<c0178364>] (suspend_devices_and_enter+0xc4/0xeac)
[<c0178364>] (suspend_devices_and_enter) from [<c01799dc>] (pm_suspend+0x890/0xc94)
[<c01799dc>] (pm_suspend) from [<c01773f0>] (state_store+0x80/0xdc)
[<c01773f0>] (state_store) from [<c03f442c>] (kobj_attr_store+0x1c/0x28)
[<c03f442c>] (kobj_attr_store) from [<c02bb858>] (sysfs_kf_write+0x5c/0x60)
[<c02bb858>] (sysfs_kf_write) from [<c02baba8>] (kernfs_fop_write+0x128/0x254)
[<c02baba8>] (kernfs_fop_write) from [<c0243830>] (__vfs_write+0x38/0x128)
[<c0243830>] (__vfs_write) from [<c0244f00>] (vfs_write+0xb4/0x174)
[<c0244f00>] (vfs_write) from [<c0245e08>] (SyS_write+0x54/0xb0)
[<c0245e08>] (SyS_write) from [<c01092e0>] (ret_fast_syscall+0x0/0x1c)
Johan
On 07/03/2017 11:54 AM, Johan Hovold wrote:
> On Fri, May 19, 2017 at 03:04:37PM -0500, Dave Gerlach wrote:
>> AM335x and AM437x support various low power modes as documented
>> in section 8.1.4.3 of the AM335x Technical Reference Manual and
>> section 6.4.3 of the AM437x Technical Reference Manual.
>>
>> DeepSleep0 mode offers the lowest power mode with limited
>> wakeup sources without a system reboot and is mapped as
>> the suspend state in the kernel. In this state, MPU and
>> PER domains are turned off with the internal RAM held in
>> retention to facilitate the resume process. As part of
>> the boot process, the assembly code is copied over to OCMCRAM
>> so it can be executed to turn of the EMIF and put DDR into self
>> refresh.
>>
>> Both platforms have a Cortex-M3 (WKUP_M3) which assists the MPU
>> in DeepSleep0 entry and exit. WKUP_M3 takes care
>> of the clockdomain and powerdomain transitions based on the
>> intended low power state. MPU needs to load the appropriate
>> WKUP_M3 binary onto the WKUP_M3 memory space before it can
>> leverage any of the PM features like DeepSleep. This loading
>> is handled by the remoteproc driver wkup_m3_rproc.
>>
>> Communication with the WKUP_M3 is handled by a wkup_m3_ipc
>> driver that exposes the specific PM functionality to be used
>> the PM code.
>
>> +static void am33xx_pm_free_sram(void)
>> +{
>> + gen_pool_free(sram_pool, ocmcram_location, *pm_sram->do_wfi_sz);
>> + gen_pool_free(sram_pool_data, ocmcram_location_data,
>> + sizeof(struct am33xx_pm_ro_sram_data));
>> +}
>> +
>> +/*
>> + * Push the minimal suspend-resume code to SRAM
>> + */
>> +static int am33xx_prepare_push_sram_idle(void)
>> +{
>> + struct device_node *np;
>> +
>> + np = of_find_compatible_node(NULL, NULL, "ti,omap3-mpu");
>> +
>
> Stray newline.
>
Yes thanks.
>> + if (!np) {
>> + np = of_find_compatible_node(NULL, NULL, "ti,omap4-mpu");
>> + if (!np) {
>> + pr_warn("PM: %s: Unable to find device node for mpu\n",
>> + __func__);
>> + return -ENODEV;
>> + }
>> + }
>
> You never put the reference to np you acquire here.
Whoops, it seems I did not, will fix.
>
> [snip]
>
>> +static int am33xx_push_sram_idle(void)
>> +{
>> + struct am33xx_pm_ro_sram_data ro_sram_data;
>> + int ret;
>> + void *copy_addr;
>> +
>> + ro_sram_data.amx3_pm_sram_data_virt = ocmcram_location_data;
>> + ro_sram_data.amx3_pm_sram_data_phys =
>> + gen_pool_virt_to_phys(sram_pool_data, ocmcram_location_data);
>> +
>> + /* Save physical address to calculate resume offset during pm init */
>> + am33xx_do_wfi_sram_phys = gen_pool_virt_to_phys(sram_pool,
>> + ocmcram_location);
>> +
>> + am33xx_do_wfi_sram = sram_exec_copy(sram_pool, (void *)ocmcram_location,
>> + pm_sram->do_wfi,
>> + *pm_sram->do_wfi_sz);
>> + if (!am33xx_do_wfi_sram) {
>> + pr_err("PM: %s: am33xx_do_wfi copy to sram failed\n", __func__);
>> + return -ENODEV;
>> + }
>> +
>> + ret = ti_emif_copy_pm_function_table(sram_pool,
>> + (void *)sram_suspend_address((unsigned long)pm_sram->emif_sram_table));
>> + if (ret) {
>> + pr_warn("PM: %s: EMIF function copy failed\n", __func__);
>> + return -EPROBE_DEFER;
>> + }
>
> Here's the dependency to the emif device I commented on earlier (and
> below).
>
I commented on this in the ti-emif-pm thread but we should be ok, we can't
remove that driver while pm33xx is loaded because of the direct call of exported
symbols, pm33xx holds a reference to ti-emif-pm until it is unloaded. I will
make sure the confirmation that these functions are valid is solid though.
>> +
>> + copy_addr = sram_exec_copy(sram_pool,
>> + (void *)sram_suspend_address((unsigned long)pm_sram->ro_sram_data),
>> + &ro_sram_data,
>> + sizeof(ro_sram_data));
>> + if (!copy_addr) {
>> + pr_err("PM: %s: ro_sram_data copy to sram failed\n", __func__);
>> + return -ENODEV;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int am33xx_pm_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + int ret;
>> +
>> + if (!of_machine_is_compatible("ti,am33xx") &&
>> + !of_machine_is_compatible("ti,am43"))
>> + return -ENODEV;
>> +
>> + pm_ops = dev->platform_data;
>> + if (!pm_ops) {
>> + pr_err("PM: Cannot get core PM ops!\n");
>> + return -ENODEV;
>> + }
>> +
>> + pm_sram = pm_ops->get_sram_addrs();
>> + if (!pm_sram) {
>> + pr_err("PM: Cannot get PM asm function addresses!!\n");
>> + return -ENODEV;
>> + }
>> +
>> + ret = am33xx_prepare_push_sram_idle();
>
> Perhaps calling this one am33xx_pm_alloc_sram() would be more
> descriptive (and match the release function)?
Not a bad point.
>
>> + if (ret)
>> + return ret;
>> +
>> + ret = am33xx_push_sram_idle();
>> + if (ret)
>> + goto err_free_sram;
>
> As I mentioned in my comments to the emif-sram driver, you may need to
> create device link to the emif-sram device to prevent it from going away
> under you here.
Addressed in ti-emif-pm thread.
>
>> +
>> + m3_ipc = wkup_m3_ipc_get();
>> + if (!m3_ipc) {
>> + pr_err("PM: Cannot get wkup_m3_ipc handle\n");
>
> You shouldn't log this as an error when probe is being deferred.
Yes, good point, just noise.
>
> Why not use dev_err and friends for logging now that you have a struct
> device?
I suppose I could now.
>
> And similarly to the emif-sram device, you may need to create a
> device-link also to the ipc device to prevent its driver from being
> unbound.
As described in the ti-emif-pm thread for that driver, we also call exported
symbols directly from wkup_m3_ipc driver, so pm33xx cannot probe at all if
wkup_m3_ipc is not loaded, and wkup_m3_ipc cannot be removed once pm33xx has
been loaded on top.
>
>> + ret = -EPROBE_DEFER;
>> + goto err_free_sram;
>> + }
>> +
>> + am33xx_pm_set_ipc_ops();
>> +
>> +#ifdef CONFIG_SUSPEND
>> + suspend_set_ops(&am33xx_pm_ops);
>> +#endif /* CONFIG_SUSPEND */
>
> This renders a lockdep splash about a circular locking dependency when
> suspending since we're taking the pm_mutex in suspend_set_ops here, and
> during suspend we flush any deferred probes while already holding the
> mutex:
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 4.12.0-rc7 #11 Not tainted
> ------------------------------------------------------
> bash/404 is trying to acquire lock:
> (deferred_probe_work){+.+.+.}, at: [<c014cf3c>] flush_work+0x30/0x27c
>
> but task is already holding lock:
> (pm_mutex){+.+...}, at: [<c01792dc>] pm_suspend+0x190/0xc94
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (pm_mutex){+.+...}:
> __mutex_lock+0x80/0x694
> mutex_lock_nested+0x2c/0x34
> suspend_set_ops+0x4c/0x128
> am33xx_pm_probe+0x1fc/0x3a8
> platform_drv_probe+0x5c/0xc0
> driver_probe_device+0x37c/0x490
> __device_attach_driver+0xac/0x128
> bus_for_each_drv+0x74/0xa8
> __device_attach+0xc4/0x154
> device_initial_probe+0x1c/0x20
> bus_probe_device+0x98/0xa0
> deferred_probe_work_func+0x4c/0xe4
> process_one_work+0x1f4/0x758
> worker_thread+0x1e0/0x514
> kthread+0x128/0x158
> ret_from_fork+0x14/0x24
>
> -> #0 (deferred_probe_work){+.+.+.}:
> lock_acquire+0x108/0x264
> flush_work+0x60/0x27c
> wait_for_device_probe+0x24/0xa4
> dpm_prepare+0xd0/0x91c
> dpm_suspend_start+0x1c/0x70
> suspend_devices_and_enter+0xc4/0xeac
> pm_suspend+0x890/0xc94
> state_store+0x80/0xdc
> kobj_attr_store+0x1c/0x28
> sysfs_kf_write+0x5c/0x60
> kernfs_fop_write+0x128/0x254
> __vfs_write+0x38/0x128
> vfs_write+0xb4/0x174
> SyS_write+0x54/0xb0
> ret_fast_syscall+0x0/0x1c
>
Yes thanks, I have seen this before myself now. I will need to look closer into
eliminating this. I am not sure how it is happening, pm_suspend should not be
able to be called if suspend_set_ops has not completed, at which point it should
have released the mutex.
Regards,
Dave
> Johan
>
On Thu, Jul 06, 2017 at 02:08:07PM -0500, Dave Gerlach wrote:
> On 07/03/2017 11:54 AM, Johan Hovold wrote:
> > On Fri, May 19, 2017 at 03:04:37PM -0500, Dave Gerlach wrote:
> >> AM335x and AM437x support various low power modes as documented
> >> in section 8.1.4.3 of the AM335x Technical Reference Manual and
> >> section 6.4.3 of the AM437x Technical Reference Manual.
> >>
> >> DeepSleep0 mode offers the lowest power mode with limited
> >> wakeup sources without a system reboot and is mapped as
> >> the suspend state in the kernel. In this state, MPU and
> >> PER domains are turned off with the internal RAM held in
> >> retention to facilitate the resume process. As part of
> >> the boot process, the assembly code is copied over to OCMCRAM
> >> so it can be executed to turn of the EMIF and put DDR into self
> >> refresh.
> >>
> >> Both platforms have a Cortex-M3 (WKUP_M3) which assists the MPU
> >> in DeepSleep0 entry and exit. WKUP_M3 takes care
> >> of the clockdomain and powerdomain transitions based on the
> >> intended low power state. MPU needs to load the appropriate
> >> WKUP_M3 binary onto the WKUP_M3 memory space before it can
> >> leverage any of the PM features like DeepSleep. This loading
> >> is handled by the remoteproc driver wkup_m3_rproc.
> >>
> >> Communication with the WKUP_M3 is handled by a wkup_m3_ipc
> >> driver that exposes the specific PM functionality to be used
> >> the PM code.
> > And similarly to the emif-sram device, you may need to create a
> > device-link also to the ipc device to prevent its driver from being
> > unbound.
>
> As described in the ti-emif-pm thread for that driver, we also call exported
> symbols directly from wkup_m3_ipc driver, so pm33xx cannot probe at all if
> wkup_m3_ipc is not loaded, and wkup_m3_ipc cannot be removed once pm33xx has
> been loaded on top.
As discussed in the other thread, the ipc driver can be unbound from its
device even if the module remains loaded.
> >
> >> + ret = -EPROBE_DEFER;
> >> + goto err_free_sram;
> >> + }
> >> +
> >> + am33xx_pm_set_ipc_ops();
> >> +
> >> +#ifdef CONFIG_SUSPEND
> >> + suspend_set_ops(&am33xx_pm_ops);
> >> +#endif /* CONFIG_SUSPEND */
> >
> > This renders a lockdep splash about a circular locking dependency when
> > suspending since we're taking the pm_mutex in suspend_set_ops here, and
> > during suspend we flush any deferred probes while already holding the
> > mutex:
> >
> > ======================================================
> > WARNING: possible circular locking dependency detected
> > 4.12.0-rc7 #11 Not tainted
> > ------------------------------------------------------
> > bash/404 is trying to acquire lock:
> > (deferred_probe_work){+.+.+.}, at: [<c014cf3c>] flush_work+0x30/0x27c
> >
> > but task is already holding lock:
> > (pm_mutex){+.+...}, at: [<c01792dc>] pm_suspend+0x190/0xc94
> >
> > which lock already depends on the new lock.
> >
> >
> > the existing dependency chain (in reverse order) is:
> >
> > -> #1 (pm_mutex){+.+...}:
> > __mutex_lock+0x80/0x694
> > mutex_lock_nested+0x2c/0x34
> > suspend_set_ops+0x4c/0x128
> > am33xx_pm_probe+0x1fc/0x3a8
> > platform_drv_probe+0x5c/0xc0
> > driver_probe_device+0x37c/0x490
> > __device_attach_driver+0xac/0x128
> > bus_for_each_drv+0x74/0xa8
> > __device_attach+0xc4/0x154
> > device_initial_probe+0x1c/0x20
> > bus_probe_device+0x98/0xa0
> > deferred_probe_work_func+0x4c/0xe4
> > process_one_work+0x1f4/0x758
> > worker_thread+0x1e0/0x514
> > kthread+0x128/0x158
> > ret_from_fork+0x14/0x24
> >
> > -> #0 (deferred_probe_work){+.+.+.}:
> > lock_acquire+0x108/0x264
> > flush_work+0x60/0x27c
> > wait_for_device_probe+0x24/0xa4
> > dpm_prepare+0xd0/0x91c
> > dpm_suspend_start+0x1c/0x70
> > suspend_devices_and_enter+0xc4/0xeac
> > pm_suspend+0x890/0xc94
> > state_store+0x80/0xdc
> > kobj_attr_store+0x1c/0x28
> > sysfs_kf_write+0x5c/0x60
> > kernfs_fop_write+0x128/0x254
> > __vfs_write+0x38/0x128
> > vfs_write+0xb4/0x174
> > SyS_write+0x54/0xb0
> > ret_fast_syscall+0x0/0x1c
> >
>
> Yes thanks, I have seen this before myself now. I will need to look closer into
> eliminating this. I am not sure how it is happening, pm_suspend should not be
> able to be called if suspend_set_ops has not completed, at which point it should
> have released the mutex.
So perhaps the deadlock cannot happen in practise then even if both
paths can indeed be taken (which triggers the lockdep warning).
Johan