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

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

[Marek Vasut]

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@denx.de>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Loic Poulain <loic.poulain@linaro.org>
Cc: Marc Zyngier <maz@kernel.org>
---
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);
+
 	return 0;
 }
 
-- 
2.35.1

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

[Marc Zyngier]

On Sun, 24 Jul 2022 03:34:18 +0100,
Marek Vasut <marex@denx.de> 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@denx.de>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Loic Poulain <loic.poulain@linaro.org>
> Cc: Marc Zyngier <maz@kernel.org>
> ---
> 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.

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

[Marek Vasut]

On 7/24/22 11:28, Marc Zyngier wrote:
> On Sun, 24 Jul 2022 03:34:18 +0100,
> Marek Vasut <marex@denx.de> 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@denx.de>
>> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Loic Poulain <loic.poulain@linaro.org>
>> Cc: Marc Zyngier <maz@kernel.org>
>> ---
>> 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.

Done in separate (new) patch for V3.