Message ID | 20220724224943.294057-1-marex@denx.de |
---|---|
State | New |
Headers | show |
Series | [v4,1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock | expand |
On Mon, Jul 25, 2022 at 12:51 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. ... > + spin_lock_irqsave(&port->gc.bgpio_lock, flags); To my surprise, is bgpio_lock not a raw spin lock?! How is it possible to work on RT?
On Mon, Jul 25, 2022 at 10:38 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Jul 25, 2022 at 12:51 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. > > ... > > > + spin_lock_irqsave(&port->gc.bgpio_lock, flags); > > To my surprise, is bgpio_lock not a raw spin lock?! How is it possible > to work on RT? It's a spinlock that is used both for the GPIO and irqchips, so if the GPIO doesn't have an irqchip it works fine, as only IRQs are problematic. If the registers used by the irqchip are separate from the registers used by the GPIO access, I think it is wise to use a separate raw spinlock to just protect the IRQ registers. They really only need to share a spinlock if they use the same registers and the gpiochip and irqchip risk stepping on each others toes. That doesn't seem to be the case here? Marek: could you see if the irqchip part of the driver could use its own raw spinlock? Yours, Linus Walleij
On Tue, Jul 26, 2022 at 12:30 AM Linus Walleij <linus.walleij@linaro.org> wrote: > Marek: could you see if the irqchip part of the driver could > use its own raw spinlock? Oh I see v5 already does, great! Linus
On 7/26/22 00:30, Linus Walleij wrote: > On Mon, Jul 25, 2022 at 10:38 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Mon, Jul 25, 2022 at 12:51 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. >> >> ... >> >>> + spin_lock_irqsave(&port->gc.bgpio_lock, flags); >> >> To my surprise, is bgpio_lock not a raw spin lock?! How is it possible >> to work on RT? > > It's a spinlock that is used both for the GPIO and irqchips, so if the > GPIO doesn't have an irqchip it works fine, as only IRQs are > problematic. > > If the registers used by the irqchip are separate from the registers > used by the GPIO access, I think it is wise to use a separate > raw spinlock to just protect the IRQ registers. > > They really only need to share a spinlock if they use the same > registers and the gpiochip and irqchip risk stepping on each > others toes. That doesn't seem to be the case here? > > Marek: could you see if the irqchip part of the driver could > use its own raw spinlock? It cannot, because of the GDIR register which is shared between the gpiochip and irqchip.
Hi Marek, I love your patch! Yet something to improve: [auto build test ERROR on brgl/gpio/for-next] [also build test ERROR on linus/master v5.19-rc8 next-20220728] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Marek-Vasut/gpio-mxc-Protect-GPIO-irqchip-RMW-with-bgpio-spinlock/20220725-065121 base: https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next config: mips-randconfig-r006-20220728 (https://download.01.org/0day-ci/archive/20220729/202207290626.5eWctWgO-lkp@intel.com/config) compiler: mipsel-linux-gcc (GCC) 12.1.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/670bfed8938f593e99c7784ff2efe48bc5b9e21d git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Marek-Vasut/gpio-mxc-Protect-GPIO-irqchip-RMW-with-bgpio-spinlock/20220725-065121 git checkout 670bfed8938f593e99c7784ff2efe48bc5b9e21d # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/gpio/ If you fix the issue, kindly add following tag where applicable Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): In file included from include/linux/rwsem.h:15, from include/linux/notifier.h:15, from include/linux/clk.h:14, from drivers/gpio/gpio-mxc.c:10: drivers/gpio/gpio-mxc.c: In function 'gpio_set_irq_type': >> drivers/gpio/gpio-mxc.c:190:27: error: passing argument 1 of 'spinlock_check' from incompatible pointer type [-Werror=incompatible-pointer-types] 190 | spin_lock_irqsave(&port->gc.bgpio_lock, flags); | ^~~~~~~~~~~~~~~~~~~~ | | | raw_spinlock_t * {aka struct raw_spinlock *} include/linux/spinlock.h:242:48: note: in definition of macro 'raw_spin_lock_irqsave' 242 | flags = _raw_spin_lock_irqsave(lock); \ | ^~~~ drivers/gpio/gpio-mxc.c:190:9: note: in expansion of macro 'spin_lock_irqsave' 190 | spin_lock_irqsave(&port->gc.bgpio_lock, flags); | ^~~~~~~~~~~~~~~~~ include/linux/spinlock.h:322:67: note: expected 'spinlock_t *' {aka 'struct spinlock *'} but argument is of type 'raw_spinlock_t *' {aka 'struct raw_spinlock *'} 322 | static __always_inline raw_spinlock_t *spinlock_check(spinlock_t *lock) | ~~~~~~~~~~~~^~~~ >> drivers/gpio/gpio-mxc.c:211:32: error: passing argument 1 of 'spin_unlock_irqrestore' from incompatible pointer type [-Werror=incompatible-pointer-types] 211 | spin_unlock_irqrestore(&port->gc.bgpio_lock, flags); | ^~~~~~~~~~~~~~~~~~~~ | | | raw_spinlock_t * {aka struct raw_spinlock *} include/linux/spinlock.h:402:64: note: expected 'spinlock_t *' {aka 'struct spinlock *'} but argument is of type 'raw_spinlock_t *' {aka 'struct raw_spinlock *'} 402 | static __always_inline void spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags) | ~~~~~~~~~~~~^~~~ drivers/gpio/gpio-mxc.c: In function 'mxc_flip_edge': drivers/gpio/gpio-mxc.c:223:27: error: passing argument 1 of 'spinlock_check' from incompatible pointer type [-Werror=incompatible-pointer-types] 223 | spin_lock_irqsave(&port->gc.bgpio_lock, flags); | ^~~~~~~~~~~~~~~~~~~~ | | | raw_spinlock_t * {aka struct raw_spinlock *} include/linux/spinlock.h:242:48: note: in definition of macro 'raw_spin_lock_irqsave' 242 | flags = _raw_spin_lock_irqsave(lock); \ | ^~~~ drivers/gpio/gpio-mxc.c:223:9: note: in expansion of macro 'spin_lock_irqsave' 223 | spin_lock_irqsave(&port->gc.bgpio_lock, flags); | ^~~~~~~~~~~~~~~~~ include/linux/spinlock.h:322:67: note: expected 'spinlock_t *' {aka 'struct spinlock *'} but argument is of type 'raw_spinlock_t *' {aka 'struct raw_spinlock *'} 322 | static __always_inline raw_spinlock_t *spinlock_check(spinlock_t *lock) | ~~~~~~~~~~~~^~~~ drivers/gpio/gpio-mxc.c:243:32: error: passing argument 1 of 'spin_unlock_irqrestore' from incompatible pointer type [-Werror=incompatible-pointer-types] 243 | spin_unlock_irqrestore(&port->gc.bgpio_lock, flags); | ^~~~~~~~~~~~~~~~~~~~ | | | raw_spinlock_t * {aka struct raw_spinlock *} include/linux/spinlock.h:402:64: note: expected 'spinlock_t *' {aka 'struct spinlock *'} but argument is of type 'raw_spinlock_t *' {aka 'struct raw_spinlock *'} 402 | static __always_inline void spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags) | ~~~~~~~~~~~~^~~~ cc1: some warnings being treated as errors vim +/spinlock_check +190 drivers/gpio/gpio-mxc.c 146 147 static int gpio_set_irq_type(struct irq_data *d, u32 type) 148 { 149 struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); 150 struct mxc_gpio_port *port = gc->private; 151 unsigned long flags; 152 u32 bit, val; 153 u32 gpio_idx = d->hwirq; 154 int edge; 155 void __iomem *reg = port->base; 156 157 port->both_edges &= ~(1 << gpio_idx); 158 switch (type) { 159 case IRQ_TYPE_EDGE_RISING: 160 edge = GPIO_INT_RISE_EDGE; 161 break; 162 case IRQ_TYPE_EDGE_FALLING: 163 edge = GPIO_INT_FALL_EDGE; 164 break; 165 case IRQ_TYPE_EDGE_BOTH: 166 if (GPIO_EDGE_SEL >= 0) { 167 edge = GPIO_INT_BOTH_EDGES; 168 } else { 169 val = port->gc.get(&port->gc, gpio_idx); 170 if (val) { 171 edge = GPIO_INT_LOW_LEV; 172 pr_debug("mxc: set GPIO %d to low trigger\n", gpio_idx); 173 } else { 174 edge = GPIO_INT_HIGH_LEV; 175 pr_debug("mxc: set GPIO %d to high trigger\n", gpio_idx); 176 } 177 port->both_edges |= 1 << gpio_idx; 178 } 179 break; 180 case IRQ_TYPE_LEVEL_LOW: 181 edge = GPIO_INT_LOW_LEV; 182 break; 183 case IRQ_TYPE_LEVEL_HIGH: 184 edge = GPIO_INT_HIGH_LEV; 185 break; 186 default: 187 return -EINVAL; 188 } 189 > 190 spin_lock_irqsave(&port->gc.bgpio_lock, flags); 191 192 if (GPIO_EDGE_SEL >= 0) { 193 val = readl(port->base + GPIO_EDGE_SEL); 194 if (edge == GPIO_INT_BOTH_EDGES) 195 writel(val | (1 << gpio_idx), 196 port->base + GPIO_EDGE_SEL); 197 else 198 writel(val & ~(1 << gpio_idx), 199 port->base + GPIO_EDGE_SEL); 200 } 201 202 if (edge != GPIO_INT_BOTH_EDGES) { 203 reg += GPIO_ICR1 + ((gpio_idx & 0x10) >> 2); /* lower or upper register */ 204 bit = gpio_idx & 0xf; 205 val = readl(reg) & ~(0x3 << (bit << 1)); 206 writel(val | (edge << (bit << 1)), reg); 207 } 208 209 writel(1 << gpio_idx, port->base + GPIO_ISR); 210 > 211 spin_unlock_irqrestore(&port->gc.bgpio_lock, flags); 212 213 return 0; 214 } 215
diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c index c871602fc5ba9..75e7aceecac0a 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; } + 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); + 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; + 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); + + spin_unlock_irqrestore(&port->gc.bgpio_lock, flags); } /* handle 32 interrupts in one status register */
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 --- drivers/gpio/gpio-mxc.c | 11 +++++++++++ 1 file changed, 11 insertions(+)