Re: [PATCH v4 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock

From: Linus Walleij <linus.walleij@linaro.org>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Marek Vasut <marex@denx.de>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Loic Poulain <loic.poulain@linaro.org>,
	Marc Zyngier <maz@kernel.org>, NXP Linux Team <linux-imx@nxp.com>,
	Peng Fan <peng.fan@nxp.com>, Shawn Guo <shawnguo@kernel.org>
Subject: Re: [PATCH v4 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
Date: Tue, 26 Jul 2022 00:30:18 +0200	[thread overview]
Message-ID: <CACRpkdZvjBBhNy_fQ2_dirSc1ddXCAO7dPg9OQW2eGDFWpn5rA@mail.gmail.com> (raw)
In-Reply-To: <CAHp75Ve0EQfee+eiDDG_pgzt8V3yoWeVfRZswG3EdFZZRyxa2A@mail.gmail.com>

On Mon, Jul 25, 2022 at 10:38 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Jul 25, 2022 at 12:51 AM Marek Vasut <marex@denx.de> wrote:
> >
> > The driver currently performs register read-modify-write without locking
> > in its irqchip part, this could lead to a race condition when configuring
> > interrupt mode setting. Add the missing bgpio spinlock lock/unlock around
> > the register read-modify-write.
>
> ...
>
> > +       spin_lock_irqsave(&port->gc.bgpio_lock, flags);
>
> To my surprise, is bgpio_lock not a raw spin lock?! How is it possible
> to work on RT?

It's a spinlock that is used both for the GPIO and irqchips, so if the
GPIO doesn't have an irqchip it works fine, as only IRQs are
problematic.

If the registers used by the irqchip are separate from the registers
used by the GPIO access, I think it is wise to use a separate
raw spinlock to just protect the IRQ registers.

They really only need to share a spinlock if they use the same
registers and the gpiochip and irqchip risk stepping on each
others toes. That doesn't seem to be the case here?

Marek: could you see if the irqchip part of the driver could
use its own raw spinlock?

Yours,
Linus Walleij