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

[Frank Li <Frank.li@nxp.com>]

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>
>