Re: [PATCH] thermal: qcom-spmi-adc-tm5-gen3: move hardirq actions into threaded handler

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Nov 11, 2026 at 02:12:01PM +0530, Jishnu Prakash wrote:
> adc_tm5_probe() registers the ADC_TM interrupt with both a primary
> handler and thread function (adctm5_gen3_isr/adctm5_gen3_isr_thread)
> with IRQF_ONESHOT. The primary handler calls adc5_gen3_read(), which
> takes the SPMI regmap's lock (a regular, non-raw spinlock) from hardirq
> context. With CONFIG_PROVE_LOCKING enabled, this leads to a warning
> about an attempt to take a lock in an invalid wait context:
> 
>   swapper/0/0 is trying to lock:
>   ffff000b67201018
>   (qcom_spmi_pmic:259:(&spmi_regmap_config)->lock){....}-{3:3}, at:
>   regmap_lock_spinlock+0x20/0x40
>   other info that might help us debug this:
>   context-{2:2}
>   locks held by swapper/0/0: 0, last CPU#0
>   stack backtrace:
> 
>   CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 7.4.0-rc1 #1 PREEMPT(full)
> 
>   Call trace:

>   check_wait_context.isra.0+0x2d8/0x380
>   __lock_acquire+0x2ec/0x840
>   lock_acquire.part.0+0xe0/0x250
>   lock_acquire+0x8c/0x1e0

Please, drop unneeded churn, since regmap_lock_spinlock() the rest is not needed.
It's all documented in Submitting Patches, take your time to read and study that
document.

>   _raw_spin_lock_irqsave+0x6c/0xd8
>   regmap_lock_spinlock+0x20/0x40
>   regmap_raw_read+0x88/0x328
>   regmap_bulk_read+0x10c/0x340
>   adc5_gen3_read+0x38/0x50 [qcom_spmi_adc5_gen3]
>   adctm5_gen3_isr+0x90/0x130 [qcom_spmi_adc_tm5_gen3]
> 
> There is no requirement for any of the work done in the primary handler
> to run in hardirq context. Drop the primary handler and move all the
> actions in it to the threaded handler, which removes the invalid wait
> context warning.

...

> -static irqreturn_t adctm5_gen3_isr(int irq, void *dev_id)
> +static irqreturn_t adctm5_gen3_isr_thread(int irq, void *dev_id)
>  {
>  	struct adc_tm5_gen3_chip *adc_tm5 = dev_id;
> -	int ret, sdam_num;
> +	int sdam_index, ret;

Was the variable rename so important for the fix? Can we defer it to the later
cleanups, if ever needed?

>  	u8 tm_status[2];
>  	u8 status, val;

...

> -	struct adc_tm5_gen3_chip *adc_tm5 = dev_id;
> -	u8 tm_status[2];
> -	int sdam_index;

Okay, I see the reason now why it's done, but I noticed no mention of this in
the commit message. Also it's unclear which variable is used more often and
what is the real semantics of its value. Is it number or index? With all this
in mind, please at bare minimum elaborate in the commit message and, if it's
actually a number, replace the other part instead.

-- 
With Best Regards,
Andy Shevchenko