Re: [PATCH] gpio: pca953x: fix pca953x_irq_bus_sync_unlock race

[Jean Delvare <jdelvare@xxxxxxx>]

Hello Ian,

On Thu, 2024-06-20 at 07:29 +0300, Ian Ray wrote:
> Ensure that `i2c_lock' is held when setting interrupt latch and mask in
> pca953x_irq_bus_sync_unlock() in order to avoid races.
> 
> The other (non-probe) call site pca953x_gpio_set_multiple() ensures the
> lock is held before calling pca953x_write_regs().
> 
> The problem occurred when a request raced against irq_bus_sync_unlock()
> approximately once per thousand reboots on an i.MX8MP based system.
> 
>  * Normal case
> 
>    0-0022: write register AI|3a {03,02,00,00,01} Input latch P0
>    0-0022: write register AI|49 {fc,fd,ff,ff,fe} Interrupt mask P0
>    0-0022: write register AI|08 {ff,00,00,00,00} Output P3
>    0-0022: write register AI|12 {fc,00,00,00,00} Config P3
> 
>  * Race case
> 
>    0-0022: write register AI|08 {ff,00,00,00,00} Output P3
>    0-0022: write register AI|08 {03,02,00,00,01} *** Wrong register ***
>    0-0022: write register AI|12 {fc,00,00,00,00} Config P3
>    0-0022: write register AI|49 {fc,fd,ff,ff,fe} Interrupt mask P0
> 
> Signed-off-by: Ian Ray <ian.ray@xxxxxxxxxxxxxxxx>
> ---
>  drivers/gpio/gpio-pca953x.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 77a2812f2974..732a6964748c 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -758,6 +758,8 @@ static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
>         int level;
>  
>         if (chip->driver_data & PCA_PCAL) {
> +               guard(mutex)(&chip->i2c_lock);
> +
>                 /* Enable latch on interrupt-enabled inputs */
>                 pca953x_write_regs(chip, PCAL953X_IN_LATCH, chip->irq_mask);
>  

I've been asked to backport this fix to SUSE kernels and I have a
concern about it.

You take the i2c_lock mutex inside the (chip->driver_data & PCA_PCAL)
conditional block, where pca953x_write_regs() is being called, and the
commit description implies this is indeed the call you wanted to
protect.

However, immediately after the conditional block, the common code path
includes a call to pca953x_read_regs(). Looking at the rest of the
driver code, I see that the i2c_lock mutex is *also* always held
(except during device probe) when calling this function. Which isn't
really surprising as I seem to understand the device uses a banked
register addressing, and this typically affects both reading from and
writing to registers.

So I suspect the i2c_lock mutex needs to be held for this call to
pca953x_read_regs() as well (unless you are familiar with the register
map and know for sure that the "direction" register is outside of the
banked register range).

I'm not familiar with the gpio-pca953x driver at all so I may be
missing something and maybe everything is actually fine, but I would
appreciate if someone could take a look and give a second opinion.

Thanks,
-- 
Jean Delvare
SUSE L3 Support