On Mon, 2024-11-04 at 13:49 +0100, Uwe Kleine-König wrote: > Hello, > > [adding rmk to Cc as the docs state that he invented lazy disabling] > > On Thu, Oct 31, 2024 at 01:05:21PM +0100, Nuno Sá wrote: > > On Thu, 2024-10-31 at 11:40 +0100, Uwe Kleine-König wrote: > > > On Wed, Oct 30, 2024 at 08:44:29PM +0000, Jonathan Cameron wrote: > > > > On Wed, 30 Oct 2024 14:04:58 +0100 > > > > Nuno Sá <noname.nuno@xxxxxxxxx> wrote: > > > > > Regarding this, I do share some of the concerns already raised by Jonathan. > > > > > I fear > > > > > that we're papering around an issue with the IRQ controller rather than > > > > > being an > > > > > issue with the device. When I look at irq_disable() docs [1], it feels that > > > > > we're > > > > > already doing what we're supposed to do. IOW, we disable the lazy approach > > > > > so we > > > > > *should* not get any pending IRQ. > > > > > > I think this is wrong and you always have to be prepared to see an irq > > > triggering that became pending while masked. > > I did some research, here are my findings: > > https://www.kernel.org/doc/html/v6.12-rc6/core-api/genericirq.html#delayed-interrupt-disable > reads: > > The interrupt is kept enabled and is masked in the flow handler > when an interrupt event happens. This prevents losing edge > interrupts on hardware which does not store an edge interrupt > event while the interrupt is disabled at the hardware level. > > This suggests that lazy disabling is needed for some controllers that > stop their event detection when disabled. I read that as: *Normally* an > irq event gets pending in hardware while the irq is disabled. I might be wrong, but I think that the lazy approach is the one for optimization. I think the reasoning is that __normally__ no more IRQs will come so no need to access the HW. But also thinking more on the subject and looking at what the lazy approach is doing, I take back what I said in previous emails. I *think* the expectation for a received IRQ while the line is masked (or disabled?!), is to keep it as pending (both on HW - when possible - and in SW). > > The lazy disable approach is expected to work fine always, the reason to > implement non-lazy disabling is "only" a performance optimisation. See > commit e9849777d0e27cdd2902805be51da73e7c79578c. Not sure If I understood you correctly, but I think is the other way around? Also, as said in the commit, I think it also prevents the same interrupt from happening twice (in some cases). > > With the DOUT/̅R̅D̅Y pin the ad7124 (and others) is in this "Unfortunately > there are devices which do not allow the interrupt to be disabled easily > at the device level." class. > > However that makes me wonder what is the difference between the > irq_mask() and irq_disable() callbacks defined in struct irq_chip. Wondering the same... Thanks for digging into this. This has been a long standing thing with sigma delta ADCs (I'm fairly sure this discussion about being an issue on the IRQ controller or not already happened before). - Nuno Sá
