Re: [PATCH v2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode

[Marc Zyngier <maz@xxxxxxxxxx>]

On Sun, 24 Jul 2022 03:34:18 +0100,
Marek Vasut <marex@xxxxxxx> wrote:
> 
> Always configure GPIO pins which are used as interrupt source as INPUTs.
> In case the default pin configuration is OUTPUT, or the prior stage does
> configure the pins as OUTPUT, then Linux will not reconfigure the pin as
> INPUT and no interrupts are received.
> 
> Always configure interrupt source GPIO pin as input to fix the above case.
> 
> 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>
> ---
> V2: Actually update and clear bits in GDIR register
> ---
>  drivers/gpio/gpio-mxc.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index c871602fc5ba9..ed58441627d92 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -147,7 +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;
> -	u32 bit, val;
> +	u32 bit, val, dir;
>  	u32 gpio_idx = d->hwirq;
>  	int edge;
>  	void __iomem *reg = port->base;
> @@ -204,6 +204,10 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>  
>  	writel(1 << gpio_idx, port->base + GPIO_ISR);
>  
> +	dir = readl(port->base + GPIO_GDIR);
> +	dir &= ~BIT(gpio_idx);
> +	writel(dir, port->base + GPIO_GDIR);
> +

This is obviously extremely racy, as another CPU could be doing the
same thing in parallel for a line that shares the same register.
Looking at the driver, it is clear that it was all written at a time
when SMP was only a pretty distant concern (i.e. the authors never
considered it a possibility).

I'd be fine with it if there was no SMP platform using this, but there
is at least one (imx7d).

So please fix this first (placing all RMW sequences behind a lock),
and only then add this change.

Thanks,

	M.

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