gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock

PATCH Superseded Marek Vasut 3 participants 13 messages Oct 13, 2022 – Jan 16, 2023

Subsystems (2)

Arm i.MX and Layerscape GPIO

lore

Share & tools

lore

Marek Vasut

author

2022-10-13 22:00 UTC #0 lore raw

PATCH v6 1/2 gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock

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.

Reviewed-by Linus Walleij <[email protected]>

Reviewed-by Marc Zyngier <[email protected]>

Fixes 07bd1a6cc7cbb ("MXC arch: Add gpio support for the whole platform")

Signed-off-by Marek Vasut <[email protected]>

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

drivers/gpio/gpio-mxc.c +11 -0

index c871602fc5ba9..6cc98a5684ae1 100644
@@ -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 */

Signature · 1 line

2.35.1

Marek Vasut

author

2022-10-13 22:00 UTC #1 lore raw

PATCH v6 2/2 gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode

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 the 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 <[email protected]>

---
Cc: Bartosz Golaszewski <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Loic Poulain <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: NXP Linux Team <[email protected]>
Cc: Peng Fan <[email protected]>
Cc: Shawn Guo <[email protected]>
---
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
V6: - Call gc->direction_input() to set direction, instead of GDIR register
poking. The bgpio (gpio-mmio) may cache the content of direction register, so updating the HW GDIR register is not enough. Calling the gc->direction() assures the cache is updated.
- Drop RBs since this is updated patch.

---
 drivers/gpio/gpio-mxc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpio/gpio-mxc.c +1 -1

index 6cc98a5684ae1..dd91908c72f19 100644
@@ -210,7 +210,7 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
 
 	raw_spin_unlock_irqrestore(&port->gc.bgpio_lock, flags);
 
-	return 0;
+	return port->gc.direction_input(&port->gc, gpio_idx);
 }
 
 static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)

Signature · 1 line

2.35.1

Linus Walleij

Reviewed-by on v6 2/2

2022-10-17 10:24 UTC #2 lore raw

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

Quoted 9 lines · 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 the 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 <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Marek Vasut

author

2022-12-16 21:51 UTC #3 lore raw

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

Quoted 7 lines · Linus Walleij

Quoted 5 lines · 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.

Quoted 6 lines

Quoted 4 lines
Always configure the 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 <[email protected]>
Reviewed-by Linus Walleij <[email protected]>

Can you please just pick these two patches up ?

Thank you

Linus Walleij

2022-12-18 23:21 UTC #4 lore raw

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

Quoted 8 lines · Marek Vasut

Quoted 7 lines · Linus Walleij
Quoted 5 lines · 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.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Always configure the 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 <[email protected]>
Reviewed-by Linus Walleij <[email protected]>
Can you please just pick these two patches up ?

Bartosz is queueing GPIO patches, I'm sure he will get to queue them soon enough.

Yours,
Linus Walleij

Marek Vasut

author

2022-12-19 00:30 UTC #5 lore raw

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

Quoted 14 lines · Linus Walleij

Quoted 13 lines · Marek Vasut
Quoted 12 lines · Linus Walleij
Quoted 9 lines · 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 the 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 <[email protected]>

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
Reviewed-by Linus Walleij <[email protected]>
Can you please just pick these two patches up ?
Bartosz is queueing GPIO patches, I'm sure he will get to queue them soon enough.

Thanks

Marek Vasut

author

2023-01-10 08:16 UTC #6 lore raw

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

In reply to Linus Walleij

Quoted 14 lines · Linus Walleij

Quoted 13 lines · Marek Vasut
Quoted 12 lines · Linus Walleij
Quoted 9 lines · 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 the 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 <[email protected]>

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
Reviewed-by Linus Walleij <[email protected]>
Can you please just pick these two patches up ?
Bartosz is queueing GPIO patches, I'm sure he will get to queue them soon enough.

Bartosz, any news ?

Bartosz Golaszewski

2023-01-16 08:32 UTC #7 lore raw

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

Quoted 16 lines · Marek Vasut

Quoted 14 lines · Linus Walleij
Quoted 13 lines · Marek Vasut
Quoted 12 lines · Linus Walleij
Quoted 9 lines · 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 the 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 <[email protected]>

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Reviewed-by Linus Walleij <[email protected]>
Can you please just pick these two patches up ?
Bartosz is queueing GPIO patches, I'm sure he will get to queue them soon enough.
Bartosz, any news ?

I don't know why this has escaped my attention for so long. I wanted to apply it but it no longer applies on top of my for-next branch. Could you rebase and resend? Sorry for the trouble Marek.

Bart

Marek Vasut

author

2023-01-16 09:51 UTC #8 lore raw

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

Quoted 21 lines · Bartosz Golaszewski

Quoted 20 lines · Marek Vasut
Quoted 18 lines · Linus Walleij
Quoted 16 lines · Marek Vasut
Quoted 7 lines · Linus Walleij
Quoted 5 lines · 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.
Quoted 6 lines
Quoted 4 lines
Always configure the 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 <[email protected]>
Reviewed-by Linus Walleij <[email protected]>
Can you please just pick these two patches up ?

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
Bartosz is queueing GPIO patches, I'm sure he will get to queue them soon enough.
Bartosz, any news ?
I don't know why this has escaped my attention for so long. I wanted to apply it but it no longer applies on top of my for-next branch. Could you rebase and resend? Sorry for the trouble Marek.

Done, V7 is out.

Linus Walleij

2022-10-17 10:23 UTC #9 lore raw

In reply to Marek Vasut

Quoted 9 lines · 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.
Reviewed-by Linus Walleij <[email protected]>
Reviewed-by Marc Zyngier <[email protected]>
Fixes 07bd1a6cc7cbb ("MXC arch: Add gpio support for the whole platform")
Signed-off-by Marek Vasut <[email protected]>

Unrelated, but Marek can you have a look at this MXC patch since you're obviously working on the platform:
https://lore.kernel.org/linux-gpio/[email protected]/

Yours,
Linus Walleij

Marek Vasut

author

2022-10-18 08:33 UTC #10 lore raw

Quoted 7 lines · Linus Walleij

Quoted 5 lines · 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.

Quoted 8 lines

Quoted 4 lines
Reviewed-by Linus Walleij <[email protected]>
Reviewed-by Marc Zyngier <[email protected]>
Fixes 07bd1a6cc7cbb ("MXC arch: Add gpio support for the whole platform")
Signed-off-by Marek Vasut <[email protected]>
Unrelated, but Marek can you have a look at this MXC patch since you're obviously working on the platform:
https://lore.kernel.org/linux-gpio/[email protected]/

Errrr, that's i.MX8, which is completely different chip than the i.MX8M (except for the naming, which ... oh well). I work on the simpler i.MX8M.

But looking at the patch, don't we already have a DT property which lets one set GPIO as wake up source, without massive enumeration tables in each GPIO driver ? It seems to me that's what NXP is trying to implement, per GPIO wake up.

Linus Walleij

2022-10-18 08:46 UTC #11 lore raw

Quoted 13 lines · Marek Vasut

Quoted 12 lines · Linus Walleij
Quoted 9 lines · 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.
Reviewed-by Linus Walleij <[email protected]>
Reviewed-by Marc Zyngier <[email protected]>
Fixes 07bd1a6cc7cbb ("MXC arch: Add gpio support for the whole platform")
Signed-off-by Marek Vasut <[email protected]>

Quoted 6 lines

Quoted 3 lines
Unrelated, but Marek can you have a look at this MXC patch since you're obviously working on the platform:
https://lore.kernel.org/linux-gpio/[email protected]/
Errrr, that's i.MX8, which is completely different chip than the i.MX8M (except for the naming, which ... oh well). I work on the simpler i.MX8M.

Yeah, I think a part of the problem is that the MXC GPIO is not connected to the back-end pin controller for i.MX so one rarely know which SoC it is used on.

Quoted 4 lines

But looking at the patch, don't we already have a DT property which lets one set GPIO as wake up source, without massive enumeration tables in each GPIO driver ? It seems to me that's what NXP is trying to implement, per GPIO wake up.

I had to bite the bullet and write a longer reply, hoping the i.MX and MXC maintainers wake up:
https://lore.kernel.org/linux-gpio/CACRpkdaKncznz5qej6owA2OGMeqbrif9e_QO3CWN6+sGhEApDw@mail.gmail.com/T/#mac3a8d5399c486657c5e168107ed591694d4b155

Yours,
Linus Walleij

Marek Vasut

author

2022-10-18 09:05 UTC #12 lore raw

Quoted 18 lines · Linus Walleij

Quoted 17 lines · Marek Vasut
Quoted 15 lines · Linus Walleij
Quoted 9 lines · 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.
Reviewed-by Linus Walleij <[email protected]>
Reviewed-by Marc Zyngier <[email protected]>
Fixes 07bd1a6cc7cbb ("MXC arch: Add gpio support for the whole platform")
Signed-off-by Marek Vasut <[email protected]>
Unrelated, but Marek can you have a look at this MXC patch since you're obviously working on the platform:
https://lore.kernel.org/linux-gpio/[email protected]/

Quoted 6 lines

Quoted 2 lines
Errrr, that's i.MX8, which is completely different chip than the i.MX8M (except for the naming, which ... oh well). I work on the simpler i.MX8M.
Yeah, I think a part of the problem is that the MXC GPIO is not connected to the back-end pin controller for i.MX so one rarely know which SoC it is used on.

The MXC GPIO is traditionally completely separate from IOMUXC pinmux controller, the MX8 (the non-M and non-X) is just a bit odd and I have little experience with that one.

Quoted 8 lines

Quoted 4 lines
But looking at the patch, don't we already have a DT property which lets one set GPIO as wake up source, without massive enumeration tables in each GPIO driver ? It seems to me that's what NXP is trying to implement, per GPIO wake up.
I had to bite the bullet and write a longer reply, hoping the i.MX and MXC maintainers wake up:
https://lore.kernel.org/linux-gpio/CACRpkdaKncznz5qej6owA2OGMeqbrif9e_QO3CWN6+sGhEApDw@mail.gmail.com/T/#mac3a8d5399c486657c5e168107ed591694d4b155

I saw that one, thanks for CCing me, I actually dropped the request from you here yesterday by accident (sorry).