Re: [PATCH] gpio: pca953x: enable latch only on edge-triggered inputs

[Francesco Lavra <flavra@baylibre.com>]

[-- Attachment #1: Type: text/plain, Size: 4720 bytes --]

On Thu, 2025-10-09 at 08:17 +0100, Martyn Welch wrote:
> On 09/10/2025 07:03, Linus Walleij wrote:
> > Hi Francesco,
> > 
> > thanks for your patch!
> > 
> > On Wed, Oct 8, 2025 at 12:43 PM Francesco Lavra <flavra@baylibre.com>
> > wrote:
> > 
> > 
> > > The latched input feature of the pca953x GPIO controller is useful
> > > when an input is configured to trigger interrupts on rising or
> > > falling edges, because it allows retrieving which edge type caused
> > > a given interrupt even if the pin state changes again before the
> > > interrupt handler has a chance to run. But for level-triggered
> > > interrupts, reading the latched input state can cause an active
> > > interrupt condition to be missed, e.g. if an active-low signal (for
> > > which an IRQ_TYPE_LEVEL_LOW interrupt has been configured) triggers
> > > an interrupt when switching to the inactive state, but then becomes
> > > active again before the interrupt handler has a chance to run: in
> > > this case, if the interrupt handler reads the latched input state,
> > > it will wrongly assume that the interrupt is not pending.
> > > Fix the above issue by enabling the latch only on edge-triggered
> > > inputs, instead of all interrupt-enabled inputs.
> > > 
> > > Signed-off-by: Francesco Lavra <flavra@baylibre.com>
> > > ---
> > >   drivers/gpio/gpio-pca953x.c | 7 +++++--
> > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-
> > > pca953x.c
> > > index e80a96f39788..e87ef2c3ff82 100644
> > > --- a/drivers/gpio/gpio-pca953x.c
> > > +++ b/drivers/gpio/gpio-pca953x.c
> > > @@ -761,10 +761,13 @@ static void pca953x_irq_bus_sync_unlock(struct
> > > irq_data *d)
> > >          int level;
> > > 
> > >          if (chip->driver_data & PCA_PCAL) {
> > > +               DECLARE_BITMAP(latched_inputs, MAX_LINE);
> > >                  guard(mutex)(&chip->i2c_lock);
> > > 
> > > -               /* Enable latch on interrupt-enabled inputs */
> > > -               pca953x_write_regs(chip, PCAL953X_IN_LATCH, chip-
> > > >irq_mask);
> > > +               /* Enable latch on edge-triggered interrupt-enabled
> > > inputs */
> > > +               bitmap_or(latched_inputs, chip->irq_trig_fall, chip-
> > > >irq_trig_raise, gc->ngpio);
> > > +               bitmap_and(latched_inputs, latched_inputs, chip-
> > > >irq_mask, gc->ngpio);
> > > +               pca953x_write_regs(chip, PCAL953X_IN_LATCH,
> > > latched_inputs);
> > 
> > This driver is used by a *lot* of systems and people.
> > 
> > It is maybe the most used GPIO driver in the kernel.
> > 
> > So I added a lot of affected developers to the To: line of
> > the mail so we can get a wider review and testing.
> > 
> 
> I don't have access to the relevant hardware to test this anymore and 
> it's been a while since I thought much about edge vs. level triggered 
> interrupts. But if the state of the interrupt is unilaterally returning 
> to an inactive state, it sounds like that should be configured as an 
> edge triggered interrupt, not a level triggered one...

I will try to better describe the problematic scenario:
- a device has an IRQ line that becomes active when the device needs to be
serviced, and becomes inactive when the device has been serviced (e.g. by
reading a status register); this is the classic use case for level-
triggered interrupts
- the IRQ line of this device is connected to a pca953x input, and this
input is configured as a level-triggered interrupt
- the device IRQ line becomes active, this triggers an interrupt in the
pca953x, the pca953x interrupt handler is invoked, it reads the input
state, then calls the nested interrupt handler
- the nested interrupt handler services the device, which causes the IRQ
line to become inactive: this triggers a second interrupt in the pca953x
- before the pca953x interrupt handler is invoked for the second time, the
device IRQ line becomes active again
- the pca953x interrupt handler is invoked, it reads the input state, which
shows the line as inactive (because that is the state that triggered the
second interrupt), and as a result the nested interrupt handler is not
invoked, and the device will stay forever with the interrupt line asserted

With my proposed change, in the last step above the pca953x interrupt
handler will read the current input state instead of the state that caused
the second interrupt, and thus will correctly invoke the nested interrupt
handler for the second time.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]