Re: [PATCH 2/2] regulator: irq: freeze deferred hardware access during suspend

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 16/10/2026 13:05, Jiale Yao wrote:
> The regulator IRQ helper delays status checks and recovery callbacks on
> non-freezable workqueues. Those callbacks may access regulator hardware
> after the backing bus has been suspended.

I wonder if there has been a real system where the problem has been seen 
or if this is purely a theoretical problem? But yes, I think this can 
happen.
> Use system_freezable_wq for normal-priority callbacks. Allocate a
> freezable high-priority workqueue when requested so the high_prio
> contract is preserved while hardware access is frozen across suspend.
 > > Signed-off-by: Jiale Yao <yaojiale02@163.com>

I don't really know the internals of workqueues well enough to say if 
there are any potential side-effects. Hence my review is a bit of 
superficial. With this remark:
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

> ---
>   drivers/regulator/irq_helpers.c | 38 +++++++++++++++++++--------------
>   1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/regulator/irq_helpers.c b/drivers/regulator/irq_helpers.c
> index 5b3cfac28667..cdbd170d2b15 100644
> --- a/drivers/regulator/irq_helpers.c
> +++ b/drivers/regulator/irq_helpers.c
> @@ -15,6 +15,7 @@
>   #include <linux/regmap.h>
>   #include <linux/slab.h>
>   #include <linux/spinlock.h>
> +#include <linux/workqueue.h>
>   #include <linux/regulator/driver.h>
>   
>   #include "internal.h"
> @@ -27,6 +28,7 @@ struct regulator_irq {
>   	int irq;
>   	int retry_cnt;
>   	struct delayed_work isr_work;
> +	struct workqueue_struct *wq;
>   };
>   
>   /*
> @@ -145,12 +147,7 @@ static void regulator_notifier_isr_work(struct work_struct *work)
>   	return;
>   
>   reschedule:
> -	if (!d->high_prio)
> -		mod_delayed_work(system_dfl_wq, &h->isr_work,
> -				 msecs_to_jiffies(tmo));
> -	else
> -		mod_delayed_work(system_highpri_wq, &h->isr_work,
> -				 msecs_to_jiffies(tmo));
> +	mod_delayed_work(h->wq, &h->isr_work, msecs_to_jiffies(tmo));

I like the fact that this change removes the if (!d->high_prio) from the 
ISR (below) and this worker, and does the queue selection at the 
initialization time. I suppose this could be done even if workqueue 
types weren't changed. Thanks for this :)

>   }
>   
>   static irqreturn_t regulator_notifier_isr(int irq, void *data)
> @@ -248,13 +245,8 @@ static irqreturn_t regulator_notifier_isr(int irq, void *data)
>   	}
>   
>   	if (d->irq_off_ms) {
> -		if (!d->high_prio)
> -			schedule_delayed_work(&h->isr_work,
> -					      msecs_to_jiffies(d->irq_off_ms));
> -		else
> -			mod_delayed_work(system_highpri_wq,
> -					 &h->isr_work,
> -					 msecs_to_jiffies(d->irq_off_ms));
> +		mod_delayed_work(h->wq, &h->isr_work,
> +				 msecs_to_jiffies(d->irq_off_ms));
>   	}
>   
>   	return IRQ_HANDLED;
> @@ -363,13 +355,25 @@ void *regulator_irq_helper(struct device *dev,
>   
>   	init_rdev_errors(h);
>   
> -	if (h->desc.irq_off_ms)
> +	if (h->desc.irq_off_ms) {
> +		if (h->desc.high_prio) {
> +			h->wq = alloc_workqueue("regulator_irq_highpri",
> +						WQ_FREEZABLE | WQ_HIGHPRI, 0);
> +			if (!h->wq)
> +				return ERR_PTR(-ENOMEM);
> +		} else {
> +			h->wq = system_freezable_wq;
> +		}
> +
>   		INIT_DELAYED_WORK(&h->isr_work, regulator_notifier_isr_work);
> +	}
>   
>   	ret = request_threaded_irq(h->irq, NULL, regulator_notifier_isr,
>   				   IRQF_ONESHOT | irq_flags, h->desc.name, h);
>   	if (ret) {
>   		dev_err(dev, "Failed to request IRQ %d\n", irq);
> +		if (h->desc.irq_off_ms && h->desc.high_prio)
> +			destroy_workqueue(h->wq);
>   
>   		return ERR_PTR(ret);
>   	}
> @@ -393,8 +397,11 @@ void regulator_irq_helper_cancel(void **handle)
>   		struct regulator_irq *h = *handle;
>   
>   		free_irq(h->irq, h);
> -		if (h->desc.irq_off_ms)
> +		if (h->desc.irq_off_ms) {
>   			cancel_delayed_work_sync(&h->isr_work);
> +			if (h->desc.high_prio)
> +				destroy_workqueue(h->wq);
> +		}
>   
>   		h = NULL;
>   	}
> @@ -441,4 +448,3 @@ int regulator_irq_map_event_simple(int irq, struct regulator_irq_data *rid,
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(regulator_irq_map_event_simple);
> -


-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~