lkml.org 
[lkml]   [2021]   [Dec]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] PM: sleep: Avoid calling put_device() under dpm_list_mtx
From
Hi, Rafael,

On 11/4/21 18:26, Rafael J. Wysocki wrote:
> It is generally unsafe to call put_device() with dpm_list_mtx held,
> because the given device's release routine may carry out an action
> depending on that lock which then may deadlock, so modify the
> system-wide suspend and resume of devices to always drop dpm_list_mtx
> before calling put_device() (and adjust white space somewhat while
> at it).
>
> For instance, this prevents the following splat from showing up in
> the kernel log after a system resume in certain configurations:


<snip>


> @@ -1748,21 +1769,27 @@ int dpm_suspend(pm_message_t state)
> struct device *dev = to_device(dpm_prepared_list.prev);
>
> get_device(dev);
> +
> mutex_unlock(&dpm_list_mtx);
>
> error = device_suspend(dev);
>
> mutex_lock(&dpm_list_mtx);
> +
> if (error) {
> pm_dev_err(dev, state, "", error);
> dpm_save_failed_dev(dev_name(dev));
> - put_device(dev);
> - break;
> - }
> - if (!list_empty(&dev->power.entry))
> + } else if (!list_empty(&dev->power.entry)) {
> list_move(&dev->power.entry, &dpm_suspended_list);
> + }
> +
> + mutex_unlock(&dpm_list_mtx);
> +
> put_device(dev);
> - if (async_error)
> +
> + mutex_lock(&dpm_list_mtx);
> +
> + if (error || async_error)
> break;
> }
> mutex_unlock(&dpm_list_mtx);
> @@ -1879,6 +1906,7 @@ int dpm_prepare(pm_message_t state)
> struct device *dev = to_device(dpm_list.next);
>
> get_device(dev);
> +
> mutex_unlock(&dpm_list_mtx);
>
> trace_device_pm_callback_start(dev, "", state.event);
> @@ -1886,21 +1914,23 @@ int dpm_prepare(pm_message_t state)
> trace_device_pm_callback_end(dev, error);
>
> mutex_lock(&dpm_list_mtx);
> - if (error) {
> - if (error == -EAGAIN) {
> - put_device(dev);
> - error = 0;
> - continue;
> - }
> +
> + if (!error) {
> + dev->power.is_prepared = true;
> + if (!list_empty(&dev->power.entry))
> + list_move_tail(&dev->power.entry, &dpm_prepared_list);
> + } else if (error == -EAGAIN) {
> + error = 0;
> + } else {
> dev_info(dev, "not prepared for power transition: code %d\n",
> error);
> - put_device(dev);
> - break;

It appears the above break disappeared.


> }
> - dev->power.is_prepared = true;
> - if (!list_empty(&dev->power.entry))
> - list_move_tail(&dev->power.entry, &dpm_prepared_list);
> +
> + mutex_unlock(&dpm_list_mtx);
> +
> put_device(dev);

Should be

                 if (error)

                        break;

Here?

Symptoms is if we return an error from the device prepare callback, we
end up spinning forever with little clue what's going on.


> +
> + mutex_lock(&dpm_list_mtx);
> }
> mutex_unlock(&dpm_list_mtx);
> trace_suspend_resume(TPS("dpm_prepare"), state.event, false);

Thanks,

Thomas


\
 
 \ /
  Last update: 2021-12-16 14:28    [W:0.075 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site