diff mbox series

[v4,1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock

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

Commit Message

Marek Vasut July 24, 2022, 10:49 p.m. UTC 
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(+)

Comments

Andy Shevchenko July 25, 2022, 8:37 p.m. UTC | #1 
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?
Linus Walleij July 25, 2022, 10:30 p.m. UTC | #2 
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
Linus Walleij July 25, 2022, 10:33 p.m. UTC | #3 
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
Marek Vasut July 25, 2022, 11:53 p.m. UTC | #4 
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.
kernel test robot July 28, 2022, 10:21 p.m. UTC | #5 
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 mbox series

Patch

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 */