lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87r07gmej2.ffs@tglx>
Date: Tue, 12 Nov 2024 20:35:13 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>
Cc: linux-kernel@...r.kernel.org, Nuno Sá
 <noname.nuno@...il.com>, Jonathan Cameron <jic23@...nel.org>
Subject: Re: can/should a disabled irq become pending?

Uwe!

On Mon, Nov 11 2024 at 14:03, Uwe Kleine-König wrote:
> [This mail is about an issue identified in a thread on the linux-iio
> mailing list, but I guess you missed that because the Subject isn't
> "interesting".

Pretty much so :)

> The ad7124 ADC and a few cousins of it have the peculiarity to report
> their interrupt on the SPI MISO line. So while the SPI clock is toggling
> it behaves as MISO and after a transfer it's a falling edge irq
> signal.

Saves a pin and the fallout can be handled in software, or not :)

> So the driver masks the irq during SPI transfers (using irq_disable()).
> It also uses irq_set_status_flags(sigma_delta->irq_line,
> IRQ_DISABLE_UNLAZY);
>
> Now on some irq controllers (e.g. the rpi GPIO controller) the detection
> logic is off between calls to irq_disable() and irq_enable() and so on
> that platform calling irq_enable() makes the irq handler called only on
> a real irq, while for other irq controllers (e.g. the Altera GPIO
> controller) the SPI transfers make the irq pending and irq_enable()
> results in a call of the handler immediately (but very likely before the
> device actually asserted the interrupt).
>
> I think (but please correct me) that the latter behaviour has to be
> expected and the former is even broken as it might miss irq events.

The default lazy disabling of interrupts has two reasons:

    1) Avoid the potentially expensive mask/unmask operations for the
       common case where no interrupt comes in between mask() and
       unmask()

    2) Addressing the problem of edge interrupt controllers, which
       disable the edge detection logic when masked, which in turn
       causes interrupts to be lost.

IRQ_DISABLE_UNLAZY solves a different problem. See commit e9849777d0e2
("genirq: Add flag to force mask in disable_irq[_nosync]()") See the
full discussion at:

  https://lore.kernel.org/all/alpine.DEB.2.11.1510092348370.6097@nanos/T/#m08531ad84aa9a53c26f91fd55be60fad6b5607b9

> My conclusions from this are:

>  - the rpi GPIO controller should be fixed not to honor
>    IRQ_DISABLE_UNLAZY.

I don't think that there is something to fix which is specific to the
RPI GPIO controller. IRQ_DISABLE_UNLAZY is clearly defined and of course
it will cause the problem #2 for edge type interrupts on such chips.

Ignoring IRQ_DISABLE_UNLAZY might be the right thing to do for this
AD7*** use case, but is it universially correct?

It's probably correct to do so for edge type interrupts. For level type,
not so much.

>  - the ad7124 driver needs some additional logic to check the actual
>    line state in the irq handler to differentiate between "real" irqs
>    and spurious ones triggered by SPI transfers[1];
>
> [1] it can only check the level, not a passed edge, but that seems to
>     work in practise

Right, because once the pin is in /RDY mode again it stays low until the
next update happens.

But this requires that the chip is connected to a GPIO based interrupt
line. Non GPIO based interrupt controllers are not providing a line
state readback.

Thanks,

        tglx

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ