Skip to contentSkip to conversation

gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock

PATCH In review Marek Vasut 2 participants 4 messages Jan 16, 2023

Subsystems (2)
Arm i.MX and Layerscape GPIO
lore
Share & tools
lore

Marek Vasut

author
#0 lore raw

PATCH v7 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]>

---
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]>
---
V3: New patch
V4: Include linux/spinlock.h
V5: Use raw_spinlock per 3c938cc5cebcb ("gpio: use raw spinlock for gpio chip shadowed data")
V6: No change
V7: Rebase on current next-20230116

---
 drivers/gpio/gpio-mxc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/gpio/gpio-mxc.c +11 -0
index d5626c572d24e..2d9d498727f10 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>

@@ -159,6 +160,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;

@@ -197,6 +199,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)

@@ -217,15 +221,20 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
 	writel(1 << gpio_idx, port->base + GPIO_ISR);
 	port->pad_type[gpio_idx] = type;
 
+	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);

@@ -243,6 +252,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.39.0
author
#1 lore raw

PATCH v7 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.

Reviewed-by Linus Walleij <[email protected]>
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.
V7: Rebase on current next-20230116

---
 drivers/gpio/gpio-mxc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/gpio-mxc.c +1 -1
index 2d9d498727f10..6f673b2f2a1bf 100644
@@ -223,7 +223,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.39.0
#2 lore raw
In reply to Marek Vasut
Quoted 88 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]>

---
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]>
---
V3: New patch
V4: Include linux/spinlock.h
V5: Use raw_spinlock per 3c938cc5cebcb ("gpio: use raw spinlock for gpio chip shadowed data")
V6: No change
V7: Rebase on current next-20230116

---
 drivers/gpio/gpio-mxc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/gpio/gpio-mxc.c +11 -1
index d5626c572d24e..2d9d498727f10 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>

@@ -159,6 +160,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;

@@ -197,6 +199,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)

@@ -217,15 +221,20 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
        writel(1 << gpio_idx, port->base + GPIO_ISR);
        port->pad_type[gpio_idx] = type;
 
+       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);

@@ -243,6 +252,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 */
--
Quoted 2 lines

2.39.0

Both now applied, thanks and sorry again.

Bart

author
#3 lore raw
Quoted 87 lines · Bartosz Golaszewski
Quoted 86 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]>

---
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]>
---
V3: New patch
V4: Include linux/spinlock.h
V5: Use raw_spinlock per 3c938cc5cebcb ("gpio: use raw spinlock for gpio chip shadowed data")
V6: No change
V7: Rebase on current next-20230116

---
  drivers/gpio/gpio-mxc.c | 11 +++++++++++
  1 file changed, 11 insertions(+)
drivers/gpio/gpio-mxc.c +11 -0
index d5626c572d24e..2d9d498727f10 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>

@@ -159,6 +160,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;

@@ -197,6 +199,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)

@@ -217,15 +221,20 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
         writel(1 << gpio_idx, port->base + GPIO_ISR);
         port->pad_type[gpio_idx] = type;
 
+       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);

@@ -243,6 +252,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);
  }
Quoted 6 lines
Quoted 4 lines
drivers/gpio/gpio-mxc.c +0 -1
  /* handle 32 interrupts in one status register */
--

2.39.0

Both now applied, thanks and sorry again.

Thanks !