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

[Marc Zyngier <maz@xxxxxxxxxx>]

Where is the cover letter? If sending more than a single patch, please
include one.

On Sun, 24 Jul 2022 18:10:56 +0100,
Marek Vasut <marex@xxxxxxx> 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.
> 
> Fixes: 07bd1a6cc7cbb ("MXC arch: Add gpio support for the whole platform")
> Signed-off-by: Marek Vasut <marex@xxxxxxx>
> Cc: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Cc: Loic Poulain <loic.poulain@xxxxxxxxxx>
> Cc: Marc Zyngier <maz@xxxxxxxxxx>
> Cc: NXP Linux Team <linux-imx@xxxxxxx>
> Cc: Peng Fan <peng.fan@xxxxxxx>
> Cc: Shawn Guo <shawnguo@xxxxxxxxxx>
> ---
> V3: New patch
> ---
>  drivers/gpio/gpio-mxc.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index c871602fc5ba9..74a50139c9202 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -147,6 +147,7 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>  {
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>  	struct mxc_gpio_port *port = gc->private;
> +	unsigned long flags;
>  	u32 bit, val;
>  	u32 gpio_idx = d->hwirq;
>  	int edge;
> @@ -185,6 +186,8 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>  		return -EINVAL;
>  	}
>  
> +	spin_lock_irqsave(&port->gc.bgpio_lock, flags);

In my tree, bgpio is a raw spinlock, and has been since 3c938cc5cebcb.

Now, looking a bit closer at this code, I have to withdraw my earlier
comment about the lack of mutual exclusion in the existing code. All
writes are of the form:

	writel(single_bit_mask, some_addr + MXS_{SET,CLR});

which indicates that the write side can be accessed with a hot-bit
pattern, avoiding a RWM pattern and thus the need for a lock.

Your second patch, however requires the lock. I'm not sure it is safe
to do after the interrupt type has been configured though. You may
want to refer to the TRM for this.

	M.

-- 
Without deviation from the norm, progress is not possible.