[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZvaYopCACdP-dQIi@852ed68de471>
Date: Fri, 27 Sep 2024 14:36:02 +0300
From: Ian Ray <ian.ray@...ealthcare.com>
To: Jean Delvare <jdelvare@...e.de>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <brgl@...ev.pl>, linux-gpio@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] gpio: pca953x: fix pca953x_irq_bus_sync_unlock race
On Fri, Sep 27, 2024 at 11:49:04AM +0200, Jean Delvare wrote:
>
> 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.
:
> > --- 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).
Hello Jean,
Direction is indeed banked (see, for example, PCA953x_BANK_CONFIG).
It certainly looks plausible that a race between
pca953x_gpio_direction_input or pca953x_gpio_direction_output and
the register read in pca953x_irq_bus_sync_unlock may occur.
In practice, I think that this is unlikely to ever be observed because
(IMHO) GPIO direction is rarely changed after initialization.
(Disclaimer: this is true for the embedded systems I work with.)
Hope this clarifies things.
Thanks,
Ian
>
> 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