Re: [PATCH v2 2/2] thermal: qcom: add support for PMIC5 Gen3 ADC thermal monitoring

[Jonathan Cameron]

> >> +static irqreturn_t adctm5_gen3_isr_thread(int irq, void *dev_id)
> >> +{
> >> +	struct adc_tm5_gen3_chip *adc_tm5 = dev_id;
> >> +	int sdam_index = -1;
> >> +	u8 tm_status[2] = { };
> >> +	u8 buf[16] = { };
> >> +
> >> +	for (int i = 0; i < adc_tm5->nchannels; i++) {
> >> +		struct adc_tm5_gen3_channel_props *chan_prop = &adc_tm5->chan_props[i];
> >> +		int offset = chan_prop->tm_chan_index;
> >> +		bool upper_set, lower_set;
> >> +		int ret;
> >> +
> >> +		scoped_guard(adc5_gen3, adc_tm5) {
> >> +			if (chan_prop->sdam_index != sdam_index) {
> >> +				sdam_index = chan_prop->sdam_index;
> >> +				ret = adc5_gen3_tm_status_check(adc_tm5, sdam_index,
> >> +								tm_status, buf);  
> > 
> > I think the clear of other sdam interrupt status that sashiko was pointing out
> > is here as somewhat unexpectedly a function called status_check clears as well.
> >   
> 
> This is the full comment from Sashiko at this point:
> 
> > "Does the threaded handler clear statuses across all SDAMs indiscriminately?  
> 
> > Since this thread loops over all channels and clears the high status on any
> > SDAM with an active event, could it clear a pending event on a different SDAM
> > than the one that triggered the IRQ?  
> 
> > Because each SDAM has its own independent IRQ line, if the thread clears a
> > pending event on SDAM 1 while servicing SDAM 0, couldn't SDAM 1's subsequent
> > hardirq read a status of 0 and return IRQ_NONE? Could repeated IRQ_NONE
> > returns cause the IRQ subsystem to shut down SDAM 1's interrupt line as a
> > spurious interrupt storm?"  
> 
> This sequence of events can happen, but it should not be an issue.
> 
> It is possible that the threaded handler is called for servicing an
> interrupt on SDAM0, and in the loop there is a violation detected on
> a TM channel on SDAM1, and the SDAM1 TM status is cleared. But in this
> case, this violation would also be handled after we notify the thermal
> framework at the end of the loop, by some threshold update or disablement.
> 
> Even if the subsequent hardirq fires for SDAM1 and it returns IRQ_NONE
> as the TM status was cleared, the violation would have been handled
> by some threshold update, so the interrupt would not keep getting
> triggered afterwards for the same threshold's violation.
> 
> 
> I also checked the conditions from note_interrupt() in kernel/irq/spurious.c,
> for enough repeated IRQ_NONE returns to happen to cause a spurious interrupt
> disablement. 
> It looks like there needs to be more than one interrupts returning IRQ_NONE
> within 0.1 second to increment the irqs_unhandled counter once, but there can be
> at most one TM interrupt in one second since we set the time period of
> recurring TM measurement as one second here in the probe:
> 
> 		adc_tm5->chan_props[i].timer = MEAS_INT_1S;
> 
> So a spurious interrupt storm is not possible here.

Whilst sounds valid, it's a convoluted argument given it relies
on us getting spurious interrupts reported, but not enough to trigger
the stuff to stop interrupt storms. The rules around that may change
in future given it's a heuristic to stop us seeing problems on dodgy
hardware.

Can we just avoid handling interrupts for SDAMs that weren't the one that
triggered this particular interrupt?

Thanks,

Jonathan