| Message ID | 20251008104309.794273-1-flavra@baylibre.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: pca953x: enable latch only on edge-triggered inputs | expand |
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. Yours, Linus Walleij
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... Martyn > Yours, > Linus Walleij
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.
On 09/10/2025 08:44, Francesco Lavra wrote: > 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. Thanks for the detail Francesco. To me it seems that the latching is more or less required in the edge triggered case because, as the hardware manual for the PCAL6408A at least states: When an input latch register bit is 0, the corresponding input pin state is not latched... ...If the input goes back to its initial logic state before the Input port register is read, then the interrupt is cleared. So the hardware doesn't retain which bits triggered the interrupt without the latching. For level based interrupts I think you're right and it doesn't make sense to latch the value. This change seems sensible to me. Reviewed-by: Martyn Welch <martyn.welch@collabora.com>
On Thu, Oct 09, 2025 at 11:24:28AM +0100, Martyn Welch wrote: > CAUTION: This email originated from outside of GE HealthCare. Only open links or attachments if you trust the sender. Report suspicious emails using Outlook’s “Report” button. > > On 09/10/2025 08:44, Francesco Lavra wrote: > > 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. > > Thanks for the detail Francesco. > > To me it seems that the latching is more or less required in the edge > triggered case because, as the hardware manual for the PCAL6408A at > least states: > > When an input latch register bit is 0, the corresponding input pin > state is not latched... ...If the input goes back to its initial > logic state before the Input port register is read, then the > interrupt is cleared. > > So the hardware doesn't retain which bits triggered the interrupt > without the latching. > > For level based interrupts I think you're right and it doesn't make > sense to latch the value. > > This change seems sensible to me. > > Reviewed-by: Martyn Welch <martyn.welch@collabora.com> Makes sense to me. (Untested as the hardware I have access to does not use the IRQ line.) Reviewed-by: Ian Ray <ian.ray@gehealthcare.com>
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> On Wed, 08 Oct 2025 12:43:09 +0200, Francesco Lavra 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. > > [...] Due to the potential impact, I will not send it for fixes just yet but I'm willing to give it a spin in linux-next and see if anyone complains. I've queued it for v6.19. [1/1] gpio: pca953x: enable latch only on edge-triggered inputs https://git.kernel.org/brgl/linux/c/9f0fa1801fe4503eb119a4523a59a494768fda5d Best regards,
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); bitmap_complement(irq_mask, chip->irq_mask, gc->ngpio);
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(-)