[PATCH] PM: sleep: core: Fix race condition in async noirq suspend

[PATCH] PM: sleep: core: Fix race condition in async noirq suspend

[Jacky Bai]

A race condition occurs during the noirq suspend stage when accessing
bitfields in struct dev_pm_info, causing system suspend to hang.

During async noirq suspend, leaf devices are queued for asynchronous
processing while the main suspend thread continues clearing the
'work_in_progress' flag of other devices, including potential parents
of those leaf devices.

In struct dev_pm_info, 'work_in_progress' and 'must_resume' are
bitfields stored in the same byte. This creates a race when:

1. A leaf device's async thread sets its parent's 'must_resume' flag
   via dpm_superior_set_must_resume()
2. Simultaneously, the main thread clears the parent's
   'work_in_progress' flag

Since bitfield operations are not atomic, concurrent modifications to
the same byte can corrupt both fields. This leaves the parent device
in an incorrect state, causing the system suspend to hang.

Fix this by adding lock protection around the bitfield accesses to
ensure atomic read-modify-write operations.

Fixes: 443046d1ad66 ("PM: sleep: Make suspend of devices more asynchronous")
Signed-off-by: Jacky Bai <ping.bai@nxp.com>
---
 drivers/base/power/main.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 189de5250f256bdc1cdc33006b2c386d0794485f..561e24c257d779db51a0f0d50dcfee61e98de64f 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -633,7 +633,9 @@ static bool __dpm_async(struct device *dev, async_func_t func)
 	if (!is_async(dev))
 		return false;
 
+	spin_lock_irq(&dev->power.lock);
 	dev->power.work_in_progress = true;
+	spin_unlock_irq(&dev->power.lock);
 
 	get_device(dev);
 
@@ -656,8 +658,11 @@ static int dpm_async_with_cleanup(struct device *dev, void *fn)
 {
 	guard(mutex)(&async_wip_mtx);
 
-	if (!__dpm_async(dev, fn))
+	if (!__dpm_async(dev, fn)) {
+		spin_lock_irq(&dev->power.lock);
 		dev->power.work_in_progress = false;
+		spin_unlock_irq(&dev->power.lock);
+	}
 
 	return 0;
 }
@@ -698,7 +703,10 @@ static void dpm_async_resume_subordinate(struct device *dev, async_func_t func)
 static void dpm_clear_async_state(struct device *dev)
 {
 	reinit_completion(&dev->power.completion);
+
+	spin_lock_irq(&dev->power.lock);
 	dev->power.work_in_progress = false;
+	spin_unlock_irq(&dev->power.lock);
 }
 
 static bool dpm_root_device(struct device *dev)
@@ -1407,13 +1415,19 @@ static void dpm_superior_set_must_resume(struct device *dev)
 	struct device_link *link;
 	int idx;
 
-	if (dev->parent)
+	if (dev->parent) {
+		spin_lock_irq(&dev->parent->power.lock);
 		dev->parent->power.must_resume = true;
+		spin_unlock_irq(&dev->parent->power.lock);
+	}
 
 	idx = device_links_read_lock();
 
-	dev_for_each_link_to_supplier(link, dev)
+	dev_for_each_link_to_supplier(link, dev) {
+		spin_lock_irq(&link->supplier->power.lock);
 		link->supplier->power.must_resume = true;
+		spin_unlock_irq(&link->supplier->power.lock);
+	}
 
 	device_links_read_unlock(idx);
 }

---
base-commit: 9845cf73f7db6094c0d8419d6adb848028f4a921
change-id: 20260206-noirq_async_suspend-645bd6b8305e

Best regards,
-- 
Jacky Bai <ping.bai@nxp.com>

Re: [PATCH] PM: sleep: core: Fix race condition in async noirq suspend

[Frank Li]

On Fri, Feb 06, 2026 at 11:05:11PM +0800, Jacky Bai wrote:
> A race condition occurs during the noirq suspend stage when accessing
> bitfields in struct dev_pm_info, causing system suspend to hang.
>
> During async noirq suspend, leaf devices are queued for asynchronous
> processing while the main suspend thread continues clearing the
> 'work_in_progress' flag of other devices, including potential parents
> of those leaf devices.
>
> In struct dev_pm_info, 'work_in_progress' and 'must_resume' are
> bitfields stored in the same byte. This creates a race when:
>
> 1. A leaf device's async thread sets its parent's 'must_resume' flag
>    via dpm_superior_set_must_resume()
> 2. Simultaneously, the main thread clears the parent's
>    'work_in_progress' flag
>
> Since bitfield operations are not atomic, concurrent modifications to
> the same byte can corrupt both fields. This leaves the parent device
> in an incorrect state, causing the system suspend to hang.
>
> Fix this by adding lock protection around the bitfield accesses to
> ensure atomic read-modify-write operations.
>
> Fixes: 443046d1ad66 ("PM: sleep: Make suspend of devices more asynchronous")
> Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> ---
>  drivers/base/power/main.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 189de5250f256bdc1cdc33006b2c386d0794485f..561e24c257d779db51a0f0d50dcfee61e98de64f 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -633,7 +633,9 @@ static bool __dpm_async(struct device *dev, async_func_t func)
>  	if (!is_async(dev))
>  		return false;
>
> +	spin_lock_irq(&dev->power.lock);
>  	dev->power.work_in_progress = true;
> +	spin_unlock_irq(&dev->power.lock);

Thanks, it is really hard to find this problem. I just worry about if it is
enough.

for example in __dpm_async(), if "check power.work_in_progress" should be
protect also.  And does other power.xx need be protect?

Frank

>
>  	get_device(dev);
>
> @@ -656,8 +658,11 @@ static int dpm_async_with_cleanup(struct device *dev, void *fn)
>  {
>  	guard(mutex)(&async_wip_mtx);
>
> -	if (!__dpm_async(dev, fn))
> +	if (!__dpm_async(dev, fn)) {
> +		spin_lock_irq(&dev->power.lock);
>  		dev->power.work_in_progress = false;
> +		spin_unlock_irq(&dev->power.lock);
> +	}
>
>  	return 0;
>  }
> @@ -698,7 +703,10 @@ static void dpm_async_resume_subordinate(struct device *dev, async_func_t func)
>  static void dpm_clear_async_state(struct device *dev)
>  {
>  	reinit_completion(&dev->power.completion);
> +
> +	spin_lock_irq(&dev->power.lock);
>  	dev->power.work_in_progress = false;
> +	spin_unlock_irq(&dev->power.lock);
>  }
>
>  static bool dpm_root_device(struct device *dev)
> @@ -1407,13 +1415,19 @@ static void dpm_superior_set_must_resume(struct device *dev)
>  	struct device_link *link;
>  	int idx;
>
> -	if (dev->parent)
> +	if (dev->parent) {
> +		spin_lock_irq(&dev->parent->power.lock);
>  		dev->parent->power.must_resume = true;
> +		spin_unlock_irq(&dev->parent->power.lock);
> +	}
>
>  	idx = device_links_read_lock();
>
> -	dev_for_each_link_to_supplier(link, dev)
> +	dev_for_each_link_to_supplier(link, dev) {
> +		spin_lock_irq(&link->supplier->power.lock);
>  		link->supplier->power.must_resume = true;
> +		spin_unlock_irq(&link->supplier->power.lock);
> +	}
>
>  	device_links_read_unlock(idx);
>  }
>
> ---
> base-commit: 9845cf73f7db6094c0d8419d6adb848028f4a921
> change-id: 20260206-noirq_async_suspend-645bd6b8305e
>
> Best regards,
> --
> Jacky Bai <ping.bai@nxp.com>
>

Re: [PATCH] PM: sleep: core: Fix race condition in async noirq suspend

[Rafael J. Wysocki]

On Fri, Feb 6, 2026 at 4:03 PM Jacky Bai <ping.bai@nxp.com> wrote:
>
> A race condition occurs during the noirq suspend stage when accessing
> bitfields in struct dev_pm_info, causing system suspend to hang.
>
> During async noirq suspend, leaf devices are queued for asynchronous
> processing while the main suspend thread continues clearing the
> 'work_in_progress' flag of other devices, including potential parents
> of those leaf devices.
>
> In struct dev_pm_info, 'work_in_progress' and 'must_resume' are
> bitfields stored in the same byte. This creates a race when:
>
> 1. A leaf device's async thread sets its parent's 'must_resume' flag
>    via dpm_superior_set_must_resume()
> 2. Simultaneously, the main thread clears the parent's
>    'work_in_progress' flag
>
> Since bitfield operations are not atomic, concurrent modifications to
> the same byte can corrupt both fields. This leaves the parent device
> in an incorrect state, causing the system suspend to hang.
>
> Fix this by adding lock protection around the bitfield accesses to
> ensure atomic read-modify-write operations.
>
> Fixes: 443046d1ad66 ("PM: sleep: Make suspend of devices more asynchronous")
> Signed-off-by: Jacky Bai <ping.bai@nxp.com>

There is a fix for this already in my tree, see

https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=pm&id=0491f3f9f664e7e0131eb4d2a8b19c49562e5c64

> ---
>  drivers/base/power/main.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 189de5250f256bdc1cdc33006b2c386d0794485f..561e24c257d779db51a0f0d50dcfee61e98de64f 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -633,7 +633,9 @@ static bool __dpm_async(struct device *dev, async_func_t func)
>         if (!is_async(dev))
>                 return false;
>
> +       spin_lock_irq(&dev->power.lock);
>         dev->power.work_in_progress = true;
> +       spin_unlock_irq(&dev->power.lock);
>
>         get_device(dev);
>
> @@ -656,8 +658,11 @@ static int dpm_async_with_cleanup(struct device *dev, void *fn)
>  {
>         guard(mutex)(&async_wip_mtx);
>
> -       if (!__dpm_async(dev, fn))
> +       if (!__dpm_async(dev, fn)) {
> +               spin_lock_irq(&dev->power.lock);
>                 dev->power.work_in_progress = false;
> +               spin_unlock_irq(&dev->power.lock);
> +       }
>
>         return 0;
>  }
> @@ -698,7 +703,10 @@ static void dpm_async_resume_subordinate(struct device *dev, async_func_t func)
>  static void dpm_clear_async_state(struct device *dev)
>  {
>         reinit_completion(&dev->power.completion);
> +
> +       spin_lock_irq(&dev->power.lock);
>         dev->power.work_in_progress = false;
> +       spin_unlock_irq(&dev->power.lock);
>  }
>
>  static bool dpm_root_device(struct device *dev)
> @@ -1407,13 +1415,19 @@ static void dpm_superior_set_must_resume(struct device *dev)
>         struct device_link *link;
>         int idx;
>
> -       if (dev->parent)
> +       if (dev->parent) {
> +               spin_lock_irq(&dev->parent->power.lock);
>                 dev->parent->power.must_resume = true;
> +               spin_unlock_irq(&dev->parent->power.lock);
> +       }
>
>         idx = device_links_read_lock();
>
> -       dev_for_each_link_to_supplier(link, dev)
> +       dev_for_each_link_to_supplier(link, dev) {
> +               spin_lock_irq(&link->supplier->power.lock);
>                 link->supplier->power.must_resume = true;
> +               spin_unlock_irq(&link->supplier->power.lock);
> +       }
>
>         device_links_read_unlock(idx);
>  }
>
> ---
> base-commit: 9845cf73f7db6094c0d8419d6adb848028f4a921
> change-id: 20260206-noirq_async_suspend-645bd6b8305e
>
> Best regards,
> --
> Jacky Bai <ping.bai@nxp.com>
>
>

RE: [PATCH] PM: sleep: core: Fix race condition in async noirq suspend

[Jacky Bai]

> Subject: Re: [PATCH] PM: sleep: core: Fix race condition in async noirq
> suspend
> 
> On Fri, Feb 6, 2026 at 4:03 PM Jacky Bai <ping.bai@nxp.com> wrote:
> >
> > A race condition occurs during the noirq suspend stage when accessing
> > bitfields in struct dev_pm_info, causing system suspend to hang.
> >
> > During async noirq suspend, leaf devices are queued for asynchronous
> > processing while the main suspend thread continues clearing the
> > 'work_in_progress' flag of other devices, including potential parents
> > of those leaf devices.
> >
> > In struct dev_pm_info, 'work_in_progress' and 'must_resume' are
> > bitfields stored in the same byte. This creates a race when:
> >
> > 1. A leaf device's async thread sets its parent's 'must_resume' flag
> >    via dpm_superior_set_must_resume()
> > 2. Simultaneously, the main thread clears the parent's
> >    'work_in_progress' flag
> >
> > Since bitfield operations are not atomic, concurrent modifications to
> > the same byte can corrupt both fields. This leaves the parent device
> > in an incorrect state, causing the system suspend to hang.
> >
> > Fix this by adding lock protection around the bitfield accesses to
> > ensure atomic read-modify-write operations.
> >
> > Fixes: 443046d1ad66 ("PM: sleep: Make suspend of devices more
> > asynchronous")
> > Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> 
> There is a fix for this already in my tree, see
> 
Thx Rafael. The patch in your tree provide a non-locking way. My patch can be dropped.
I Will pick this patch Into our internal tree to fix the issue we meet.

BR
Jacky Bai
> 
> > ---
> >  drivers/base/power/main.c | 20 +++++++++++++++++---
> >  1 file changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > index
> >
> 189de5250f256bdc1cdc33006b2c386d0794485f..561e24c257d779db51a0f0d
> 50dcf
> > ee61e98de64f 100644
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -633,7 +633,9 @@ static bool __dpm_async(struct device *dev,
> async_func_t func)
> >         if (!is_async(dev))
> >                 return false;
> >
> > +       spin_lock_irq(&dev->power.lock);
> >         dev->power.work_in_progress = true;
> > +       spin_unlock_irq(&dev->power.lock);
> >
> >         get_device(dev);
> >
> > @@ -656,8 +658,11 @@ static int dpm_async_with_cleanup(struct device
> > *dev, void *fn)  {
> >         guard(mutex)(&async_wip_mtx);
> >
> > -       if (!__dpm_async(dev, fn))
> > +       if (!__dpm_async(dev, fn)) {
> > +               spin_lock_irq(&dev->power.lock);
> >                 dev->power.work_in_progress = false;
> > +               spin_unlock_irq(&dev->power.lock);
> > +       }
> >
> >         return 0;
> >  }
> > @@ -698,7 +703,10 @@ static void dpm_async_resume_subordinate(struct
> > device *dev, async_func_t func)  static void
> > dpm_clear_async_state(struct device *dev)  {
> >         reinit_completion(&dev->power.completion);
> > +
> > +       spin_lock_irq(&dev->power.lock);
> >         dev->power.work_in_progress = false;
> > +       spin_unlock_irq(&dev->power.lock);
> >  }
> >
> >  static bool dpm_root_device(struct device *dev) @@ -1407,13 +1415,19
> > @@ static void dpm_superior_set_must_resume(struct device *dev)
> >         struct device_link *link;
> >         int idx;
> >
> > -       if (dev->parent)
> > +       if (dev->parent) {
> > +               spin_lock_irq(&dev->parent->power.lock);
> >                 dev->parent->power.must_resume = true;
> > +               spin_unlock_irq(&dev->parent->power.lock);
> > +       }
> >
> >         idx = device_links_read_lock();
> >
> > -       dev_for_each_link_to_supplier(link, dev)
> > +       dev_for_each_link_to_supplier(link, dev) {
> > +               spin_lock_irq(&link->supplier->power.lock);
> >                 link->supplier->power.must_resume = true;
> > +               spin_unlock_irq(&link->supplier->power.lock);
> > +       }
> >
> >         device_links_read_unlock(idx);  }
> >
> > ---
> > base-commit: 9845cf73f7db6094c0d8419d6adb848028f4a921
> > change-id: 20260206-noirq_async_suspend-645bd6b8305e
> >
> > Best regards,
> > --
> > Jacky Bai <ping.bai@nxp.com>
> >
> >

RE: [PATCH] PM: sleep: core: Fix race condition in async noirq suspend

[Jacky Bai]

> Subject: Re: [PATCH] PM: sleep: core: Fix race condition in async noirq
> suspend
> 
> On Fri, Feb 06, 2026 at 11:05:11PM +0800, Jacky Bai wrote:
> > A race condition occurs during the noirq suspend stage when accessing
> > bitfields in struct dev_pm_info, causing system suspend to hang.
> >
> > During async noirq suspend, leaf devices are queued for asynchronous
> > processing while the main suspend thread continues clearing the
> > 'work_in_progress' flag of other devices, including potential parents
> > of those leaf devices.
> >
> > In struct dev_pm_info, 'work_in_progress' and 'must_resume' are
> > bitfields stored in the same byte. This creates a race when:
> >
> > 1. A leaf device's async thread sets its parent's 'must_resume' flag
> >    via dpm_superior_set_must_resume()
> > 2. Simultaneously, the main thread clears the parent's
> >    'work_in_progress' flag
> >
> > Since bitfield operations are not atomic, concurrent modifications to
> > the same byte can corrupt both fields. This leaves the parent device
> > in an incorrect state, causing the system suspend to hang.
> >
> > Fix this by adding lock protection around the bitfield accesses to
> > ensure atomic read-modify-write operations.
> >
> > Fixes: 443046d1ad66 ("PM: sleep: Make suspend of devices more
> > asynchronous")
> > Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> > ---
> >  drivers/base/power/main.c | 20 +++++++++++++++++---
> >  1 file changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > index
> >
> 189de5250f256bdc1cdc33006b2c386d0794485f..561e24c257d779db51a0f0d
> 50dcf
> > ee61e98de64f 100644
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -633,7 +633,9 @@ static bool __dpm_async(struct device *dev,
> async_func_t func)
> >  	if (!is_async(dev))
> >  		return false;
> >
> > +	spin_lock_irq(&dev->power.lock);
> >  	dev->power.work_in_progress = true;
> > +	spin_unlock_irq(&dev->power.lock);
> 
> Thanks, it is really hard to find this problem. I just worry about if it is enough.
> 
> for example in __dpm_async(), if "check power.work_in_progress" should be
> protect also.  And does other power.xx need be protect?
> 

For other bits, it seems no chance to trigger concurrent access with the work_in_progress bit. 

BR
Jacky Bai
> Frank
> 
> >
> >  	get_device(dev);
> >
> > @@ -656,8 +658,11 @@ static int dpm_async_with_cleanup(struct device
> > *dev, void *fn)  {
> >  	guard(mutex)(&async_wip_mtx);
> >
> > -	if (!__dpm_async(dev, fn))
> > +	if (!__dpm_async(dev, fn)) {
> > +		spin_lock_irq(&dev->power.lock);
> >  		dev->power.work_in_progress = false;
> > +		spin_unlock_irq(&dev->power.lock);
> > +	}
> >
> >  	return 0;
> >  }
> > @@ -698,7 +703,10 @@ static void dpm_async_resume_subordinate(struct
> > device *dev, async_func_t func)  static void
> > dpm_clear_async_state(struct device *dev)  {
> >  	reinit_completion(&dev->power.completion);
> > +
> > +	spin_lock_irq(&dev->power.lock);
> >  	dev->power.work_in_progress = false;
> > +	spin_unlock_irq(&dev->power.lock);
> >  }
> >
> >  static bool dpm_root_device(struct device *dev) @@ -1407,13 +1415,19
> > @@ static void dpm_superior_set_must_resume(struct device *dev)
> >  	struct device_link *link;
> >  	int idx;
> >
> > -	if (dev->parent)
> > +	if (dev->parent) {
> > +		spin_lock_irq(&dev->parent->power.lock);
> >  		dev->parent->power.must_resume = true;
> > +		spin_unlock_irq(&dev->parent->power.lock);
> > +	}
> >
> >  	idx = device_links_read_lock();
> >
> > -	dev_for_each_link_to_supplier(link, dev)
> > +	dev_for_each_link_to_supplier(link, dev) {
> > +		spin_lock_irq(&link->supplier->power.lock);
> >  		link->supplier->power.must_resume = true;
> > +		spin_unlock_irq(&link->supplier->power.lock);
> > +	}
> >
> >  	device_links_read_unlock(idx);
> >  }
> >
> > ---
> > base-commit: 9845cf73f7db6094c0d8419d6adb848028f4a921
> > change-id: 20260206-noirq_async_suspend-645bd6b8305e
> >
> > Best regards,
> > --
> > Jacky Bai <ping.bai@nxp.com>
> >