[PATCH v1] PM: sleep: core: Clear device async state upfront during suspend

Posted by Rafael J. Wysocki 3 days, 14 hours ago

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

In all of the system suspend transition phases, async state of all
devices needs to be cleared before starting async processing for any of
them because the latter may race with power.work_in_progress updates for
the device's parent or suppliers and if it touches bit fields from the
same group (for example, power.must_resume or power.wakeup_path), bit
field corruption is possible.

Rearrange the code accordingly.

Fixes: aa7a9275ab81 ("PM: sleep: Suspend async parents after suspending children")
Fixes: 443046d1ad66 ("PM: sleep: Make suspend of devices more asynchronous")
Reported-by: Xuewen Yan <xuewen.yan@unisoc.com>
Closes: https://lore.kernel.org/linux-pm/20260203063459.12808-1-xuewen.yan@unisoc.com/
Cc: All applicable <stable@vger.kernel.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c |   33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1527,11 +1527,20 @@ static int dpm_noirq_suspend_devices(pm_
 	mutex_lock(&dpm_list_mtx);
 
 	/*
+	 * Clear the async state for all devices upfront to prevent the
+	 * power.work_in_progress updates from racing with power.must_resume
+	 * updates carried out by dpm_superior_set_must_resume(), since these
+	 * flags belong to the same group of bit fields and they should not be
+	 * updated at the same time without synchronization.
+	 */
+	list_for_each_entry_reverse(dev, &dpm_late_early_list, power.entry)
+		dpm_clear_async_state(dev);
+
+	/*
 	 * Start processing "async" leaf devices upfront so they don't need to
 	 * wait for the "sync" devices they don't depend on.
 	 */
 	list_for_each_entry_reverse(dev, &dpm_late_early_list, power.entry) {
-		dpm_clear_async_state(dev);
 		if (dpm_leaf_device(dev))
 			dpm_async_with_cleanup(dev, async_suspend_noirq);
 	}
@@ -1732,11 +1741,20 @@ int dpm_suspend_late(pm_message_t state)
 	mutex_lock(&dpm_list_mtx);
 
 	/*
+	 * Clear the async state for all devices upfront to prevent the
+	 * power.work_in_progress updates from racing with power.wakeup_path
+	 * updates carried out by dpm_propagate_wakeup_to_parent(), since these
+	 * flags belong to the same group of bit fields and they should not be
+	 * updated at the same time without synchronization.
+	 */
+	list_for_each_entry_reverse(dev, &dpm_suspended_list, power.entry)
+		dpm_clear_async_state(dev);
+
+	/*
 	 * Start processing "async" leaf devices upfront so they don't need to
 	 * wait for the "sync" devices they don't depend on.
 	 */
 	list_for_each_entry_reverse(dev, &dpm_suspended_list, power.entry) {
-		dpm_clear_async_state(dev);
 		if (dpm_leaf_device(dev))
 			dpm_async_with_cleanup(dev, async_suspend_late);
 	}
@@ -2023,11 +2041,20 @@ int dpm_suspend(pm_message_t state)
 	mutex_lock(&dpm_list_mtx);
 
 	/*
+	 * Clear the async state for all devices upfront to prevent the
+	 * power.work_in_progress updates from racing with power.wakeup_path
+	 * updates carried out by dpm_propagate_wakeup_to_parent(), since these
+	 * flags belong to the same group of bit fields and they should not be
+	 * updated at the same time without synchronization.
+	 */
+	list_for_each_entry_reverse(dev, &dpm_prepared_list, power.entry)
+		dpm_clear_async_state(dev);
+
+	/*
 	 * Start processing "async" leaf devices upfront so they don't need to
 	 * wait for the "sync" devices they don't depend on.
 	 */
 	list_for_each_entry_reverse(dev, &dpm_prepared_list, power.entry) {
-		dpm_clear_async_state(dev);
 		if (dpm_leaf_device(dev))
 			dpm_async_with_cleanup(dev, async_suspend);
 	}

Re: [PATCH v1] PM: sleep: core: Clear device async state upfront during suspend

Posted by Ulf Hansson 2 days, 21 hours ago

On Tue, 3 Feb 2026 at 21:37, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> In all of the system suspend transition phases, async state of all
> devices needs to be cleared before starting async processing for any of
> them because the latter may race with power.work_in_progress updates for
> the device's parent or suppliers and if it touches bit fields from the
> same group (for example, power.must_resume or power.wakeup_path), bit
> field corruption is possible.
>
> Rearrange the code accordingly.
>
> Fixes: aa7a9275ab81 ("PM: sleep: Suspend async parents after suspending children")
> Fixes: 443046d1ad66 ("PM: sleep: Make suspend of devices more asynchronous")
> Reported-by: Xuewen Yan <xuewen.yan@unisoc.com>
> Closes: https://lore.kernel.org/linux-pm/20260203063459.12808-1-xuewen.yan@unisoc.com/
> Cc: All applicable <stable@vger.kernel.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Looks good to me!

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/base/power/main.c |   33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
>
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1527,11 +1527,20 @@ static int dpm_noirq_suspend_devices(pm_
>         mutex_lock(&dpm_list_mtx);
>
>         /*
> +        * Clear the async state for all devices upfront to prevent the
> +        * power.work_in_progress updates from racing with power.must_resume
> +        * updates carried out by dpm_superior_set_must_resume(), since these
> +        * flags belong to the same group of bit fields and they should not be
> +        * updated at the same time without synchronization.
> +        */
> +       list_for_each_entry_reverse(dev, &dpm_late_early_list, power.entry)
> +               dpm_clear_async_state(dev);
> +
> +       /*
>          * Start processing "async" leaf devices upfront so they don't need to
>          * wait for the "sync" devices they don't depend on.
>          */
>         list_for_each_entry_reverse(dev, &dpm_late_early_list, power.entry) {
> -               dpm_clear_async_state(dev);
>                 if (dpm_leaf_device(dev))
>                         dpm_async_with_cleanup(dev, async_suspend_noirq);
>         }
> @@ -1732,11 +1741,20 @@ int dpm_suspend_late(pm_message_t state)
>         mutex_lock(&dpm_list_mtx);
>
>         /*
> +        * Clear the async state for all devices upfront to prevent the
> +        * power.work_in_progress updates from racing with power.wakeup_path
> +        * updates carried out by dpm_propagate_wakeup_to_parent(), since these
> +        * flags belong to the same group of bit fields and they should not be
> +        * updated at the same time without synchronization.
> +        */
> +       list_for_each_entry_reverse(dev, &dpm_suspended_list, power.entry)
> +               dpm_clear_async_state(dev);
> +
> +       /*
>          * Start processing "async" leaf devices upfront so they don't need to
>          * wait for the "sync" devices they don't depend on.
>          */
>         list_for_each_entry_reverse(dev, &dpm_suspended_list, power.entry) {
> -               dpm_clear_async_state(dev);
>                 if (dpm_leaf_device(dev))
>                         dpm_async_with_cleanup(dev, async_suspend_late);
>         }
> @@ -2023,11 +2041,20 @@ int dpm_suspend(pm_message_t state)
>         mutex_lock(&dpm_list_mtx);
>
>         /*
> +        * Clear the async state for all devices upfront to prevent the
> +        * power.work_in_progress updates from racing with power.wakeup_path
> +        * updates carried out by dpm_propagate_wakeup_to_parent(), since these
> +        * flags belong to the same group of bit fields and they should not be
> +        * updated at the same time without synchronization.
> +        */
> +       list_for_each_entry_reverse(dev, &dpm_prepared_list, power.entry)
> +               dpm_clear_async_state(dev);
> +
> +       /*
>          * Start processing "async" leaf devices upfront so they don't need to
>          * wait for the "sync" devices they don't depend on.
>          */
>         list_for_each_entry_reverse(dev, &dpm_prepared_list, power.entry) {
> -               dpm_clear_async_state(dev);
>                 if (dpm_leaf_device(dev))
>                         dpm_async_with_cleanup(dev, async_suspend);
>         }
>
>
>

Re: [PATCH v1] PM: sleep: core: Clear device async state upfront during suspend

Posted by Xuewen Yan 3 days, 8 hours ago

Hi Rafael,

On Wed, Feb 4, 2026 at 4:38 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> In all of the system suspend transition phases, async state of all
> devices needs to be cleared before starting async processing for any of
> them because the latter may race with power.work_in_progress updates for
> the device's parent or suppliers and if it touches bit fields from the
> same group (for example, power.must_resume or power.wakeup_path), bit
> field corruption is possible.
>
> Rearrange the code accordingly.

Could we use the following patch:

diff --git a/include/linux/pm.h b/include/linux/pm.h
index 98a899858ece..afcaaa37a812 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -681,10 +681,10 @@ struct dev_pm_info {
        struct list_head        entry;
        struct completion       completion;
        struct wakeup_source    *wakeup;
+       bool                    work_in_progress;       /* Owned by
the PM core */
        bool                    wakeup_path:1;
        bool                    syscore:1;
        bool                    no_pm_callbacks:1;      /* Owned by
the PM core */
-       bool                    work_in_progress:1;     /* Owned by
the PM core */
        bool                    smart_suspend:1;        /* Owned by
the PM core */
        bool                    must_resume:1;          /* Owned by
the PM core */
        bool                    may_skip_resume:1;      /* Set by subsystems */

Due to byte alignment, the size of struct dev_pm_info remains unchanged,
while also preventing concurrency issues between work_in_progress and
other variables.
Additionally, with this modification, there’s no need to traverse the
device list twice.


BR
---
xuewen

>
> Fixes: aa7a9275ab81 ("PM: sleep: Suspend async parents after suspending children")
> Fixes: 443046d1ad66 ("PM: sleep: Make suspend of devices more asynchronous")
> Reported-by: Xuewen Yan <xuewen.yan@unisoc.com>
> Closes: https://lore.kernel.org/linux-pm/20260203063459.12808-1-xuewen.yan@unisoc.com/
> Cc: All applicable <stable@vger.kernel.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/power/main.c |   33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
>
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1527,11 +1527,20 @@ static int dpm_noirq_suspend_devices(pm_
>         mutex_lock(&dpm_list_mtx);
>
>         /*
> +        * Clear the async state for all devices upfront to prevent the
> +        * power.work_in_progress updates from racing with power.must_resume
> +        * updates carried out by dpm_superior_set_must_resume(), since these
> +        * flags belong to the same group of bit fields and they should not be
> +        * updated at the same time without synchronization.
> +        */
> +       list_for_each_entry_reverse(dev, &dpm_late_early_list, power.entry)
> +               dpm_clear_async_state(dev);
> +
> +       /*
>          * Start processing "async" leaf devices upfront so they don't need to
>          * wait for the "sync" devices they don't depend on.
>          */
>         list_for_each_entry_reverse(dev, &dpm_late_early_list, power.entry) {
> -               dpm_clear_async_state(dev);
>                 if (dpm_leaf_device(dev))
>                         dpm_async_with_cleanup(dev, async_suspend_noirq);
>         }
> @@ -1732,11 +1741,20 @@ int dpm_suspend_late(pm_message_t state)
>         mutex_lock(&dpm_list_mtx);
>
>         /*
> +        * Clear the async state for all devices upfront to prevent the
> +        * power.work_in_progress updates from racing with power.wakeup_path
> +        * updates carried out by dpm_propagate_wakeup_to_parent(), since these
> +        * flags belong to the same group of bit fields and they should not be
> +        * updated at the same time without synchronization.
> +        */
> +       list_for_each_entry_reverse(dev, &dpm_suspended_list, power.entry)
> +               dpm_clear_async_state(dev);
> +
> +       /*
>          * Start processing "async" leaf devices upfront so they don't need to
>          * wait for the "sync" devices they don't depend on.
>          */
>         list_for_each_entry_reverse(dev, &dpm_suspended_list, power.entry) {
> -               dpm_clear_async_state(dev);
>                 if (dpm_leaf_device(dev))
>                         dpm_async_with_cleanup(dev, async_suspend_late);
>         }
> @@ -2023,11 +2041,20 @@ int dpm_suspend(pm_message_t state)
>         mutex_lock(&dpm_list_mtx);
>
>         /*
> +        * Clear the async state for all devices upfront to prevent the
> +        * power.work_in_progress updates from racing with power.wakeup_path
> +        * updates carried out by dpm_propagate_wakeup_to_parent(), since these
> +        * flags belong to the same group of bit fields and they should not be
> +        * updated at the same time without synchronization.
> +        */
> +       list_for_each_entry_reverse(dev, &dpm_prepared_list, power.entry)
> +               dpm_clear_async_state(dev);
> +
> +       /*
>          * Start processing "async" leaf devices upfront so they don't need to
>          * wait for the "sync" devices they don't depend on.
>          */
>         list_for_each_entry_reverse(dev, &dpm_prepared_list, power.entry) {
> -               dpm_clear_async_state(dev);
>                 if (dpm_leaf_device(dev))
>                         dpm_async_with_cleanup(dev, async_suspend);
>         }
>
>
>
>

Re: [PATCH v1] PM: sleep: core: Clear device async state upfront during suspend

Posted by Rafael J. Wysocki 2 days, 22 hours ago

On Wed, Feb 4, 2026 at 3:57 AM Xuewen Yan <xuewen.yan94@gmail.com> wrote:
>
> Hi Rafael,
>
> On Wed, Feb 4, 2026 at 4:38 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > In all of the system suspend transition phases, async state of all
> > devices needs to be cleared before starting async processing for any of
> > them because the latter may race with power.work_in_progress updates for
> > the device's parent or suppliers and if it touches bit fields from the
> > same group (for example, power.must_resume or power.wakeup_path), bit
> > field corruption is possible.
> >
> > Rearrange the code accordingly.
>
> Could we use the following patch:

Yes, we can make this change.

> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 98a899858ece..afcaaa37a812 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -681,10 +681,10 @@ struct dev_pm_info {
>         struct list_head        entry;
>         struct completion       completion;
>         struct wakeup_source    *wakeup;
> +       bool                    work_in_progress;       /* Owned by
> the PM core */
>         bool                    wakeup_path:1;
>         bool                    syscore:1;
>         bool                    no_pm_callbacks:1;      /* Owned by
> the PM core */
> -       bool                    work_in_progress:1;     /* Owned by
> the PM core */
>         bool                    smart_suspend:1;        /* Owned by
> the PM core */
>         bool                    must_resume:1;          /* Owned by
> the PM core */
>         bool                    may_skip_resume:1;      /* Set by subsystems */
>
> Due to byte alignment, the size of struct dev_pm_info remains unchanged,

I had considered making it, but I thought it would cause struct
dev_pm_info to grow.

> while also preventing concurrency issues between work_in_progress and
> other variables. Additionally, with this modification, there’s no need to traverse the
> device list twice.

Sure.

I'll just commit the above change with your sign-off, please let me
know if there are any issues with that.

Thanks!

Re: [PATCH v1] PM: sleep: core: Clear device async state upfront during suspend

Posted by Xuewen Yan 2 days, 22 hours ago

On Wed, Feb 4, 2026 at 8:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Feb 4, 2026 at 3:57 AM Xuewen Yan <xuewen.yan94@gmail.com> wrote:
> >
> > Hi Rafael,
> >
> > On Wed, Feb 4, 2026 at 4:38 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > In all of the system suspend transition phases, async state of all
> > > devices needs to be cleared before starting async processing for any of
> > > them because the latter may race with power.work_in_progress updates for
> > > the device's parent or suppliers and if it touches bit fields from the
> > > same group (for example, power.must_resume or power.wakeup_path), bit
> > > field corruption is possible.
> > >
> > > Rearrange the code accordingly.
> >
> > Could we use the following patch:
>
> Yes, we can make this change.
>
> > diff --git a/include/linux/pm.h b/include/linux/pm.h
> > index 98a899858ece..afcaaa37a812 100644
> > --- a/include/linux/pm.h
> > +++ b/include/linux/pm.h
> > @@ -681,10 +681,10 @@ struct dev_pm_info {
> >         struct list_head        entry;
> >         struct completion       completion;
> >         struct wakeup_source    *wakeup;
> > +       bool                    work_in_progress;       /* Owned by
> > the PM core */
> >         bool                    wakeup_path:1;
> >         bool                    syscore:1;
> >         bool                    no_pm_callbacks:1;      /* Owned by
> > the PM core */
> > -       bool                    work_in_progress:1;     /* Owned by
> > the PM core */
> >         bool                    smart_suspend:1;        /* Owned by
> > the PM core */
> >         bool                    must_resume:1;          /* Owned by
> > the PM core */
> >         bool                    may_skip_resume:1;      /* Set by subsystems */
> >
> > Due to byte alignment, the size of struct dev_pm_info remains unchanged,
>
> I had considered making it, but I thought it would cause struct
> dev_pm_info to grow.
>
> > while also preventing concurrency issues between work_in_progress and
> > other variables. Additionally, with this modification, there’s no need to traverse the
> > device list twice.
>
> Sure.
>
> I'll just commit the above change with your sign-off, please let me
> know if there are any issues with that.

I have no other questions.

Thanks!

BR
--
xuewen