Message ID | 20230116094957.5756-1-marex@denx.de |
---|---|
State | New |
Headers | show |
Series | [v7,1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock | expand |
On Mon, Jan 16, 2023 at 10:50 AM 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. > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Reviewed-by: Marc Zyngier <maz@kernel.org> > 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") > V6: No change > V7: Rebase on current next-20230116 > --- > 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 d5626c572d24e..2d9d498727f10 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> > @@ -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 */ > -- > 2.39.0 > Both now applied, thanks and sorry again. Bart
On 1/16/23 10:55, Bartosz Golaszewski wrote: > On Mon, Jan 16, 2023 at 10:50 AM 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. >> >> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> >> Reviewed-by: Marc Zyngier <maz@kernel.org> >> 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") >> V6: No change >> V7: Rebase on current next-20230116 >> --- >> 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 d5626c572d24e..2d9d498727f10 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> >> @@ -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 */ >> -- >> 2.39.0 >> > > Both now applied, thanks and sorry again. Thanks !
diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c index d5626c572d24e..2d9d498727f10 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> @@ -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 */