linux-kernel - Re: [PATCH] gpio: pca953x: Improve interrupt support

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: <CAHp75VeLyacKo3rY5iyq+kZnLjEQsBN2eOJExHrqHuesaVyTQQ@mail.gmail.com>
Date: Tue, 14 Jan 2025 11:36:30 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: lakabd <lakabd.work@...il.com>
Cc: mark.tomlinson@...iedtelesis.co.nz, brgl@...ev.pl, 
	linus.walleij@...aro.org, linux-gpio@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Abderrahim LAKBIR <abderrahim.lakbir@...ia.fr>
Subject: Re: [PATCH] gpio: pca953x: Improve interrupt support

On Tue, Jan 14, 2025 at 12:03 AM lakabd <lakabd.work@...il.com> wrote:
>
> Hi All,
>
> I'm encountering exactly the same issue, and there is indeed a problem in the process of pca953x_irq_pending().
>
> Mark has already explained the issue, but as apparently the discussion stopped, I've tried below to add some details to help better understand the issue. I've also a solution to propose.
>
> The issue:
> In the current implementation, when an IRQ occurs, the function pca953x_irq_pending() is called to fill the pending list of IRQs. This function will accomplish the following (for PCA_PCAL):
> 1- read the interrupt status register
> 2- read the latched inputs to clear the interrupt
> 3- apply filter for rising/falling edge selection on the input value
> 4- filter any bits that aren't related to the IRQ by applying a bitmap_and operation between: value calculated in step 3 and the value of ISR in step 1
> 5- return True with the pending bitmap if not empty
>
> The problem here is that any interrupt that occurs between operation 1 and 2 will be lost even if latching is enabled !

This is clear.

> Example:
> * Interrupt occurs in pin 0 of port 0
> 1- Interrupt status register read (port0) = 0x10
> ** Interrupt occurs in pin 4 of port 0
> 2- input register read (port0) = 0x11 --> resets Interruptline
> 4- bitmap_and operation will remove the newly changed bit:0x11 & 0x10 = 0x10 and the returned pending bitmap will have only the pin0 interrupt !
>
> The latching helps with very short interrupts to not be lost, but in the situation above it is not relevant.
>
> Proposed solution:
> In the 4th step apply bitmap_and between the filtered latched input and the bitmap of the unmasked interrupts. This will ensure the same outcome by letting only pins for which the IRQ is unmasked to pass but will not remove newly triggered interrupts.
> This new unmasked interrupts bitmap can be filled in pca953x_irq_bus_sync_unlock() when an irq mask is getting set.
> Also, by applying this, we can discard completely the read of the interrupt status register in step 1. Hence, the only I2C read that will be sent is the read of the Input register which minimizes the time to interrupt forwarding.

...

> +                             /* Store irq_mask for later use when checking pending IRQs */
> +                             bitmap_or(chip->unmasked_interrupts, chip->unmasked_interrupts, chip->irq_mask, gc->ngpio);

This solution has a flaw. Where is any code that clears this new
bitmap? The code starts with 0 (obviously) and step by step it gets
saturated to all-1s.

-- 
With Best Regards,
Andy Shevchenko