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

[Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>]

Hi Andy,

On 11/11/2026 7:53 PM, Andy Shevchenko wrote:
> 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.

Yes, I'll trim the call stack and drop the less relevant error lines.

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

In both adctm5_gen3_isr() and adctm5_gen3_isr_thread(), we needed a variable to
store the index of the SDAM whose IRQ was triggered. We called it 'sdam_num'
in adctm5_gen3_isr() and 'sdam_index' in adctm5_gen3_isr_thread(). In the rest
of the file, it is called 'sdam_index', so ideally that same name should have
been used in adctm5_gen3_isr() too, as it is more accurate than 'sdam_num'.

As I mentioned at the end of the commit message, I just meant to drop adctm5_gen3_isr()
and expand adctm5_gen3_isr_thread() to include the actions formerly done in
adctm5_gen3_isr(), but I see the diff makes it look more like I modified adctm5_gen3_isr()
into adctm5_gen3_isr_thread() with a variable rename, which is a bit misleading.

I can add a line in the commit message to emphasize that 'sdam_num' is the same as
'sdam_index' if you think it helps.


Thanks,
JIshnu

>