Re: [PATCH] gpio: pca953x: fix pca953x_irq_bus_sync_unlock regmap lock

[Bartosz Golaszewski]

On Wed, 1 Jul 2026 03:02:28 +0200, Mark Tomlinson
<mark.tomlinson@xxxxxxxxxxxxxxxxxxx> said:
> Locking is disabled in the regmap config as this driver uses its own
> lock. This means that all calls to regmap functions (read or write) must
> hold the i2c_lock. The function pca953x_irq_bus_sync_unlock() did not do
> this, and it was therefore possible that multiple threads could cause an
> incorrect register to be read/written.
>
> A previous patch partly fixed this, but only protected the write to the
> interrupt mask register, and not the read from the direction register.
>
> Signed-off-by: Mark Tomlinson <mark.tomlinson@xxxxxxxxxxxxxxxxxxx>

Plase add Fixes: and Cc: stable.

> ---
>  drivers/gpio/gpio-pca953x.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 2ee35e855e4d..9689e3f3c517 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -605,20 +605,27 @@ static int pca953x_read_regs(struct pca953x_chip *chip, int reg, unsigned long *
>  	return 0;
>  }
>
> -static int pca953x_gpio_direction_input(struct gpio_chip *gc, unsigned off)
> +static int _pca953x_gpio_direction_input(struct gpio_chip *gc, unsigned off)

Please prefer pca953x_gpio_direction_input_unlocked() as it's more verbose
about the purpose.

>  {
>  	struct pca953x_chip *chip = gpiochip_get_data(gc);
>  	u8 dirreg = chip->recalc_addr(chip, chip->regs->direction, off);
>  	u8 bit = pca953x_get_bit_mask(chip, off);
>
> -	guard(mutex)(&chip->i2c_lock);
> -
>  	if (PCA_CHIP_TYPE(chip->driver_data) == TCA6418_TYPE)
>  		return regmap_update_bits(chip->regmap, dirreg, bit, 0);
>
>  	return regmap_update_bits(chip->regmap, dirreg, bit, bit);
>  }
>
> +static int pca953x_gpio_direction_input(struct gpio_chip *gc, unsigned off)
> +{
> +	struct pca953x_chip *chip = gpiochip_get_data(gc);
> +
> +	guard(mutex)(&chip->i2c_lock);
> +
> +	return _pca953x_gpio_direction_input(gc, off);
> +}
> +
>  static int pca953x_gpio_direction_output(struct gpio_chip *gc,
>  		unsigned off, int val)
>  {
> @@ -856,9 +863,10 @@ static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
>  	DECLARE_BITMAP(reg_direction, MAX_LINE);
>  	int level;
>
> +	guard(mutex)(&chip->i2c_lock);
> +
>  	if (chip->driver_data & PCA_PCAL) {
>  		DECLARE_BITMAP(latched_inputs, MAX_LINE);
> -		guard(mutex)(&chip->i2c_lock);
>
>  		/* Enable latch on edge-triggered interrupt-enabled inputs */
>  		bitmap_or(latched_inputs, chip->irq_trig_fall, chip->irq_trig_raise, gc->ngpio);
> @@ -880,7 +888,7 @@ static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
>
>  	/* Look for any newly setup interrupt */
>  	for_each_andnot_bit(level, irq_mask, reg_direction, gc->ngpio)
> -		pca953x_gpio_direction_input(&chip->gpio_chip, level);
> +		_pca953x_gpio_direction_input(&chip->gpio_chip, level);
>
>  	mutex_unlock(&chip->irq_lock);
>  }
> --
> 2.54.0
>
>

Thanks,
Bartosz