| Message ID | 20221013215946.216184-1-marex@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v6,1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock | expand |
On Fri, Oct 14, 2022 at 12:00 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> 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/20221007152853.838136-1-shenwei.wang@nxp.com/ Yours, Linus Walleij
On 10/17/22 12:23, Linus Walleij wrote: > On Fri, Oct 14, 2022 at 12:00 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> > > 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/20221007152853.838136-1-shenwei.wang@nxp.com/ 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.
On Tue, Oct 18, 2022 at 10:33 AM Marek Vasut <marex@denx.de> wrote: > On 10/17/22 12:23, Linus Walleij wrote: > > On Fri, Oct 14, 2022 at 12:00 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> > > > > 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/20221007152853.838136-1-shenwei.wang@nxp.com/ > > 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. > 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
On 10/18/22 10:46, Linus Walleij wrote: > On Tue, Oct 18, 2022 at 10:33 AM Marek Vasut <marex@denx.de> wrote: >> On 10/17/22 12:23, Linus Walleij wrote: >>> On Fri, Oct 14, 2022 at 12:00 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> >>> >>> 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/20221007152853.838136-1-shenwei.wang@nxp.com/ >> >> 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. >> 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).
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 */