[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8d8462da853b6c147e3cdb790b2e3ea7d4aaf533.camel@suse.de>
Date: Fri, 27 Sep 2024 11:49:04 +0200
From: Jean Delvare <jdelvare@...e.de>
To: Ian Ray <ian.ray@...ealthcare.com>, Linus Walleij
<linus.walleij@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>
Cc: linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] gpio: pca953x: fix pca953x_irq_bus_sync_unlock race
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@...ealthcare.com>
> ---
> 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
Powered by blists - more mailing lists