On Thu, 2024-10-31 at 13:05 +0100, Nuno Sá wrote: > On Thu, 2024-10-31 at 11:40 +0100, Uwe Kleine-König wrote: > > Hello, > > > > 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: > > > > > > > On Mon, 2024-10-28 at 17:07 +0100, Uwe Kleine-König wrote: > > > > > Some of the ADCs by Analog signal their irq condition on the MISO line. > > > > > So typically that line is connected to an SPI controller and a GPIO. The > > > > > GPIO is used as input and the respective interrupt is enabled when the > > > > > last SPI transfer is completed. > > > > > > > > > > Depending on the GPIO controller the toggling MISO line might make the > > > > > interrupt pending even while it's masked. In that case the irq handler > > > > > is called immediately after irq_enable() and so before the device > > > > > actually pulls that line low which results in non-sense values being > > > > > reported to the upper layers. > > > > > > > > > > The only way to find out if the line was actually pulled low is to read > > > > > the GPIO. (There is a flag in AD7124's status register that also signals > > > > > if an interrupt was asserted, but reading that register toggles the MISO > > > > > line and so might trigger another spurious interrupt.) > > > > > > > > > > Add the possibility to specify an interrupt GPIO in the machine > > > > > description instead of a plain interrupt. This GPIO is used as interrupt > > > > > source and to check if the irq line is actually active in the irq > > > > > handler. > > > > > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxx> > > > > > --- > > > > > > > > Hi all, > > > > > > > > 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. > > > > > > Also looking at drivers as the xilinx gpio controller, it seems some > > > > are careful about this [2] and make sure to clear all pending IRQs > > > > when unmasking. > > > Your links are both to the same place. > > > > The right one is: > > https://elixir.bootlin.com/linux/v6.11.5/source/drivers/gpio/gpio-xilinx.c#L419 > > > > I think this is buggy, see below for the reasoning. > > > > > > Jonathan also said this: > > > > > > > > "True enough - that race is a possibility, but not all interrupt inputs > > > > are capable of gpio usage whilst setup to received interrupts." > > > Race should be easy to avoid using a level interrupt now I think more on that: > > > can't miss a level. > > > > In general this isn't true. If it were that easy we could just assume > > all irqs being level interrupts and simplify the irq code a bit. At > > least for the ad7124 if a conversion is done, the chip holds the line > > low until the next conversion is done. In that case it deasserts > > DOUT/̅R̅D̅Y for a short while to create another falling edge signalling > > another event. I can imagine this to confuse level detection?! > > > > > > To my understanding this also means this is doomed to fail for some devices > > > > or > > > > am I > > > > not following it? > > > > > > If you were wired to one of those, you couldn't use the GPIO trick, but then > > > don't have a GPIO in your DT in that case. > > > > Yes. If the device isn't properly connected in hardware you're out of > > luck. But that is also true if the spi clock line isn't connected. So > > apart from the requirement that "properly" involves things that are > > unusual for other SPI devices, that's expected. Having said that it was > > clear before because the MISO (aka DOUT/̅R̅D̅Y) line was already know to have > > to be connected to an irq capable pin. > > > > > > All that said, my naive feeling would be for a masked line to not get any > > > > pending IRQ > > > > and if it does, the driver should make sure to clean all outstanding > > > > interrupts > > > > when > > > > unmasking. But I'm far from being an expert of the IRQ subsystem. Maybe it > > > > would be > > > > interesting to get some inputs about someone who actually knows better? > > > +CC Thomas Gleixner, > > > > > > Annoying case where a wire is both the interrupt source for dataready and the > > > SPI data line (if separate clock signal is toggling) So currently the driver > > > masks interrupts at the host end, but we have at least one interrupt controller > > > where they end up pending and fire on reenabling the interrupt. Querying the > > > device to check the status register then ends up causing it to happen again, > > > so that doesn't help. > > > > > > Proposal is to query it as a GPIO (or maybe a separate GPIO wired to the same > > > pin) to check the level when an interrupt comes in. > > > > In my understanding it's the expected behaviour of an irq controller > > that a masked irq becomes pending if the irq event (level or edge) > > happens and then triggers immediately after enable_irq() -- independent > > of laziness being used or not. > > > > I'm really not sure about that. If a consumer disables/masks an interrupt, then I > would think it expects no interrupts. If one comes during that time, it seems > reasonable to me that the IRQ is discarded. And if the expected behavior is to have > a > pending IRQ if we got it while masked, then I'm not sure why we have the UNLAZY > thing. I mean, let's always do the lazy approach which is effectively only masking > the line if we get an IRQ while disabled (and also mark it as pending). If both > approaches result in a pending IRQ... Hmmm maybe the real point with UNLAZY is to not take the same interrupt twice. One through the resend mechanism and another one through HW. - Nuno Sá
