[PATCH v5 0/2] gpio: mxc: Locking and direction input fix

[PATCH v5 0/2] gpio: mxc: Locking and direction input fix

[Marek Vasut]

The irqchip callbacks in gpio-mxc perform register read-modify-write operations
without locking, which may lead to a race condition. Add the missing locking.

In case the GPIO is used as IRQ, make sure it is configured as input.

Marek Vasut (2):
  gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
  gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode

 drivers/gpio/gpio-mxc.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

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>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Shawn Guo <shawnguo@kernel.org>

-- 
2.35.1

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

[Marek Vasut]

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@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>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Shawn Guo <shawnguo@kernel.org>
---
V3: New patch
V4: Include linux/spinlock.h
V5: Use raw_spinlock per 3c938cc5cebcb ("gpio: use raw spinlock for gpio chip shadowed data")
---
 drivers/gpio/gpio-mxc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index c871602fc5ba9..6cc98a5684ae1 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 #include <linux/syscore_ops.h>
 #include <linux/gpio/driver.h>
 #include <linux/of.h>
@@ -147,6 +148,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 +187,8 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
 		return -EINVAL;
 	}
 
+	raw_spin_lock_irqsave(&port->gc.bgpio_lock, flags);
+
 	if (GPIO_EDGE_SEL >= 0) {
 		val = readl(port->base + GPIO_EDGE_SEL);
 		if (edge == GPIO_INT_BOTH_EDGES)
@@ -204,15 +208,20 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
 
 	writel(1 << gpio_idx, port->base + GPIO_ISR);
 
+	raw_spin_unlock_irqrestore(&port->gc.bgpio_lock, flags);
+
 	return 0;
 }
 
 static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)
 {
 	void __iomem *reg = port->base;
+	unsigned long flags;
 	u32 bit, val;
 	int edge;
 
+	raw_spin_lock_irqsave(&port->gc.bgpio_lock, flags);
+
 	reg += GPIO_ICR1 + ((gpio & 0x10) >> 2); /* lower or upper register */
 	bit = gpio & 0xf;
 	val = readl(reg);
@@ -230,6 +239,8 @@ static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)
 		return;
 	}
 	writel(val | (edge << (bit << 1)), reg);
+
+	raw_spin_unlock_irqrestore(&port->gc.bgpio_lock, flags);
 }
 
 /* handle 32 interrupts in one status register */
-- 
2.35.1

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

[Linus Walleij]

On Mon, Jul 25, 2022 at 12:35 PM 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.
>
> Fixes: 07bd1a6cc7cbb ("MXC arch: Add gpio support for the whole platform")
> 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>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> ---
> V3: New patch
> V4: Include linux/spinlock.h
> V5: Use raw_spinlock per 3c938cc5cebcb ("gpio: use raw spinlock for gpio chip shadowed data")

OK I was a bit confused this looks good and that spinlock is indeed raw.
There is maybe a bit of over-locking with one single lock for all registers
but I'm not very picky so:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v5 2/2] 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.

Fixes: 07bd1a6cc7cbb ("MXC arch: Add gpio support for the whole platform")
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>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Shawn Guo <shawnguo@kernel.org>
---
V2: Actually update and clear bits in GDIR register
V3: Rebase on top of new patch 1/2, expand CC list, add Fixes tag
V4: No change
V5: No change
---
 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 6cc98a5684ae1..95327c8963b6c 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -149,7 +149,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 bit, val, dir;
 	u32 gpio_idx = d->hwirq;
 	int edge;
 	void __iomem *reg = port->base;
@@ -208,6 +208,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);
+
 	raw_spin_unlock_irqrestore(&port->gc.bgpio_lock, flags);
 
 	return 0;
-- 
2.35.1

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

[Andy Shevchenko]

On Mon, Jul 25, 2022 at 12:36 PM 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.
>
> Fixes: 07bd1a6cc7cbb ("MXC arch: Add gpio support for the whole platform")
> Signed-off-by: Marek Vasut <marex@denx.de>

Can you move below lines

> 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>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Shawn Guo <shawnguo@kernel.org>

under '---' cutter line? The LORE archive will have all people you Cc'ed.

...

The comment as per v3 still stays.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 0/2] gpio: mxc: Locking and direction input fix

[Marc Zyngier]

On Mon, 25 Jul 2022 11:34:43 +0100,
Marek Vasut <marex@denx.de> wrote:
> 
> The irqchip callbacks in gpio-mxc perform register read-modify-write operations
> without locking, which may lead to a race condition. Add the missing locking.
> 
> In case the GPIO is used as IRQ, make sure it is configured as input.
> 
> Marek Vasut (2):
>   gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
>   gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode
> 
>  drivers/gpio/gpio-mxc.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> 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>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Shawn Guo <shawnguo@kernel.org>

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.

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

Re: [PATCH v5 0/2] gpio: mxc: Locking and direction input fix

[Marek Vasut]

On 7/25/22 17:50, Marc Zyngier wrote:
> On Mon, 25 Jul 2022 11:34:43 +0100,
> Marek Vasut <marex@denx.de> wrote:
>>
>> The irqchip callbacks in gpio-mxc perform register read-modify-write operations
>> without locking, which may lead to a race condition. Add the missing locking.
>>
>> In case the GPIO is used as IRQ, make sure it is configured as input.
>>
>> Marek Vasut (2):
>>    gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
>>    gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode
>>
>>   drivers/gpio/gpio-mxc.c | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> 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>
>> Cc: NXP Linux Team <linux-imx@nxp.com>
>> Cc: Peng Fan <peng.fan@nxp.com>
>> Cc: Shawn Guo <shawnguo@kernel.org>
> 
> Reviewed-by: Marc Zyngier <maz@kernel.org>

Can these two now be applied or is there something more to do ?

Re: [PATCH v5 0/2] gpio: mxc: Locking and direction input fix

[Marc Zyngier]

On 2022-08-23 11:41, Marek Vasut wrote:
> On 7/25/22 17:50, Marc Zyngier wrote:
>> On Mon, 25 Jul 2022 11:34:43 +0100,
>> Marek Vasut <marex@denx.de> wrote:
>>> 
>>> The irqchip callbacks in gpio-mxc perform register read-modify-write 
>>> operations
>>> without locking, which may lead to a race condition. Add the missing 
>>> locking.
>>> 
>>> In case the GPIO is used as IRQ, make sure it is configured as input.
>>> 
>>> Marek Vasut (2):
>>>    gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
>>>    gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode
>>> 
>>>   drivers/gpio/gpio-mxc.c | 17 ++++++++++++++++-
>>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>> 
>>> 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>
>>> Cc: NXP Linux Team <linux-imx@nxp.com>
>>> Cc: Peng Fan <peng.fan@nxp.com>
>>> Cc: Shawn Guo <shawnguo@kernel.org>
>> 
>> Reviewed-by: Marc Zyngier <maz@kernel.org>
> 
> Can these two now be applied or is there something more to do ?

That'd be question for Linus and Bartosz, I guess. From my
own PoV, this is good to go.

Thanks,

      M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v5 0/2] gpio: mxc: Locking and direction input fix

[Marek Vasut]

On 8/23/22 16:20, Marc Zyngier wrote:
> On 2022-08-23 11:41, Marek Vasut wrote:
>> On 7/25/22 17:50, Marc Zyngier wrote:
>>> On Mon, 25 Jul 2022 11:34:43 +0100,
>>> Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> The irqchip callbacks in gpio-mxc perform register read-modify-write 
>>>> operations
>>>> without locking, which may lead to a race condition. Add the missing 
>>>> locking.
>>>>
>>>> In case the GPIO is used as IRQ, make sure it is configured as input.
>>>>
>>>> Marek Vasut (2):
>>>>    gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
>>>>    gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode
>>>>
>>>>   drivers/gpio/gpio-mxc.c | 17 ++++++++++++++++-
>>>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>>>
>>>> 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>
>>>> Cc: NXP Linux Team <linux-imx@nxp.com>
>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>>
>>> Reviewed-by: Marc Zyngier <maz@kernel.org>
>>
>> Can these two now be applied or is there something more to do ?
> 
> That'd be question for Linus and Bartosz, I guess. From my
> own PoV, this is good to go.

So uh ... could either of you schedule those for I guess 6.2 please ?

Re: [PATCH v5 0/2] gpio: mxc: Locking and direction input fix

[Linus Walleij]

On Mon, Jul 25, 2022 at 12:35 PM Marek Vasut <marex@denx.de> wrote:

> The irqchip callbacks in gpio-mxc perform register read-modify-write operations
> without locking, which may lead to a race condition. Add the missing locking.
>
> In case the GPIO is used as IRQ, make sure it is configured as input.

Changes look good to me:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij