diff mbox series

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

Message ID 20220724171057.18549-1-marex@denx.de
State New
Headers show
Series [v3,1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock | expand

Commit Message

Marek Vasut July 24, 2022, 5:10 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
---
 drivers/gpio/gpio-mxc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Marc Zyngier July 24, 2022, 5:50 p.m. UTC | #1 
Where is the cover letter? If sending more than a single patch, please
include one.

On Sun, 24 Jul 2022 18:10:56 +0100,
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.
> 
> 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
> ---
>  drivers/gpio/gpio-mxc.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index c871602fc5ba9..74a50139c9202 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -147,6 +147,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 +186,8 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>  		return -EINVAL;
>  	}
>  
> +	spin_lock_irqsave(&port->gc.bgpio_lock, flags);

In my tree, bgpio is a raw spinlock, and has been since 3c938cc5cebcb.

Now, looking a bit closer at this code, I have to withdraw my earlier
comment about the lack of mutual exclusion in the existing code. All
writes are of the form:

	writel(single_bit_mask, some_addr + MXS_{SET,CLR});

which indicates that the write side can be accessed with a hot-bit
pattern, avoiding a RWM pattern and thus the need for a lock.

Your second patch, however requires the lock. I'm not sure it is safe
to do after the interrupt type has been configured though. You may
want to refer to the TRM for this.

	M.
Marek Vasut July 24, 2022, 6:15 p.m. UTC | #2 
On 7/24/22 19:50, Marc Zyngier wrote:

[...]

>> +++ b/drivers/gpio/gpio-mxc.c
>> @@ -147,6 +147,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 +186,8 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>>   		return -EINVAL;
>>   	}
>>   
>> +	spin_lock_irqsave(&port->gc.bgpio_lock, flags);
> 
> In my tree, bgpio is a raw spinlock, and has been since 3c938cc5cebcb.
> 
> Now, looking a bit closer at this code, I have to withdraw my earlier
> comment about the lack of mutual exclusion in the existing code. All
> writes are of the form:
> 
> 	writel(single_bit_mask, some_addr + MXS_{SET,CLR});
> 
> which indicates that the write side can be accessed with a hot-bit
> pattern, avoiding a RWM pattern and thus the need for a lock.

Only for the ISR/IMR, not for the GDIR register, that's why the locks 
are added only around the RMW which don't have these "hot bits".

> Your second patch, however requires the lock. I'm not sure it is safe
> to do after the interrupt type has been configured though. You may
> want to refer to the TRM for this.

There is in fact another unprotected RMW in gpio_set_irq_type() , look 
for GPIO_EDGE_SEL, so the locks should be valid as they are now, right ?
kernel test robot July 24, 2022, 6:32 p.m. UTC | #3 
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-rc7 next-20220722]
[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-011230
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
config: hexagon-randconfig-r041-20220724 (https://download.01.org/0day-ci/archive/20220725/202207250224.XFYIB7dF-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 12fbd2d377e396ad61bce56d71c98a1eb1bebfa9)
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/af6c815e16849c7ec370f755916d06a6b1e5b759
        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-011230
        git checkout af6c815e16849c7ec370f755916d06a6b1e5b759
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon 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 >>):

>> drivers/gpio/gpio-mxc.c:189:20: error: incompatible pointer types passing 'raw_spinlock_t *' (aka 'struct raw_spinlock *') to parameter of type 'spinlock_t *' (aka 'struct spinlock *') [-Werror,-Wincompatible-pointer-types]
           spin_lock_irqsave(&port->gc.bgpio_lock, flags);
                             ^~~~~~~~~~~~~~~~~~~~
   include/linux/spinlock.h:379:39: note: expanded from macro 'spin_lock_irqsave'
           raw_spin_lock_irqsave(spinlock_check(lock), flags);     \
                                                ^~~~
   include/linux/spinlock.h:264:26: note: expanded from macro 'raw_spin_lock_irqsave'
                   _raw_spin_lock_irqsave(lock, flags);    \
                                          ^~~~
   include/linux/spinlock_api_up.h:69:60: note: expanded from macro '_raw_spin_lock_irqsave'
   #define _raw_spin_lock_irqsave(lock, flags)     __LOCK_IRQSAVE(lock, flags)
                                                                  ^~~~
   include/linux/spinlock_api_up.h:40:38: note: expanded from macro '__LOCK_IRQSAVE'
     do { local_irq_save(flags); __LOCK(lock); } while (0)
                                        ^~~~
   include/linux/spinlock_api_up.h:31:35: note: expanded from macro '__LOCK'
     do { preempt_disable(); ___LOCK(lock); } while (0)
                                     ^~~~
   include/linux/spinlock_api_up.h:28:32: note: expanded from macro '___LOCK'
     do { __acquire(lock); (void)(lock); } while (0)
                                  ^~~~
   include/linux/spinlock.h:322:67: note: passing argument to parameter 'lock' here
   static __always_inline raw_spinlock_t *spinlock_check(spinlock_t *lock)
                                                                     ^
   drivers/gpio/gpio-mxc.c:210:25: error: incompatible pointer types passing 'raw_spinlock_t *' (aka 'struct raw_spinlock *') to parameter of type 'spinlock_t *' (aka 'struct spinlock *') [-Werror,-Wincompatible-pointer-types]
           spin_unlock_irqrestore(&port->gc.bgpio_lock, flags);
                                  ^~~~~~~~~~~~~~~~~~~~
   include/linux/spinlock.h:402:64: note: passing argument to parameter 'lock' here
   static __always_inline void spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
                                                                  ^
   drivers/gpio/gpio-mxc.c:222:20: error: incompatible pointer types passing 'raw_spinlock_t *' (aka 'struct raw_spinlock *') to parameter of type 'spinlock_t *' (aka 'struct spinlock *') [-Werror,-Wincompatible-pointer-types]
           spin_lock_irqsave(&port->gc.bgpio_lock, flags);
                             ^~~~~~~~~~~~~~~~~~~~
   include/linux/spinlock.h:379:39: note: expanded from macro 'spin_lock_irqsave'
           raw_spin_lock_irqsave(spinlock_check(lock), flags);     \
                                                ^~~~
   include/linux/spinlock.h:264:26: note: expanded from macro 'raw_spin_lock_irqsave'
                   _raw_spin_lock_irqsave(lock, flags);    \
                                          ^~~~
   include/linux/spinlock_api_up.h:69:60: note: expanded from macro '_raw_spin_lock_irqsave'
   #define _raw_spin_lock_irqsave(lock, flags)     __LOCK_IRQSAVE(lock, flags)
                                                                  ^~~~
   include/linux/spinlock_api_up.h:40:38: note: expanded from macro '__LOCK_IRQSAVE'
     do { local_irq_save(flags); __LOCK(lock); } while (0)
                                        ^~~~
   include/linux/spinlock_api_up.h:31:35: note: expanded from macro '__LOCK'
     do { preempt_disable(); ___LOCK(lock); } while (0)
                                     ^~~~
   include/linux/spinlock_api_up.h:28:32: note: expanded from macro '___LOCK'
     do { __acquire(lock); (void)(lock); } while (0)
                                  ^~~~
   include/linux/spinlock.h:322:67: note: passing argument to parameter 'lock' here
   static __always_inline raw_spinlock_t *spinlock_check(spinlock_t *lock)
                                                                     ^
   drivers/gpio/gpio-mxc.c:242:25: error: incompatible pointer types passing 'raw_spinlock_t *' (aka 'struct raw_spinlock *') to parameter of type 'spinlock_t *' (aka 'struct spinlock *') [-Werror,-Wincompatible-pointer-types]
           spin_unlock_irqrestore(&port->gc.bgpio_lock, flags);
                                  ^~~~~~~~~~~~~~~~~~~~
   include/linux/spinlock.h:402:64: note: passing argument to parameter 'lock' here
   static __always_inline void spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
                                                                  ^
   4 errors generated.


vim +189 drivers/gpio/gpio-mxc.c

   145	
   146	static int gpio_set_irq_type(struct irq_data *d, u32 type)
   147	{
   148		struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
   149		struct mxc_gpio_port *port = gc->private;
   150		unsigned long flags;
   151		u32 bit, val;
   152		u32 gpio_idx = d->hwirq;
   153		int edge;
   154		void __iomem *reg = port->base;
   155	
   156		port->both_edges &= ~(1 << gpio_idx);
   157		switch (type) {
   158		case IRQ_TYPE_EDGE_RISING:
   159			edge = GPIO_INT_RISE_EDGE;
   160			break;
   161		case IRQ_TYPE_EDGE_FALLING:
   162			edge = GPIO_INT_FALL_EDGE;
   163			break;
   164		case IRQ_TYPE_EDGE_BOTH:
   165			if (GPIO_EDGE_SEL >= 0) {
   166				edge = GPIO_INT_BOTH_EDGES;
   167			} else {
   168				val = port->gc.get(&port->gc, gpio_idx);
   169				if (val) {
   170					edge = GPIO_INT_LOW_LEV;
   171					pr_debug("mxc: set GPIO %d to low trigger\n", gpio_idx);
   172				} else {
   173					edge = GPIO_INT_HIGH_LEV;
   174					pr_debug("mxc: set GPIO %d to high trigger\n", gpio_idx);
   175				}
   176				port->both_edges |= 1 << gpio_idx;
   177			}
   178			break;
   179		case IRQ_TYPE_LEVEL_LOW:
   180			edge = GPIO_INT_LOW_LEV;
   181			break;
   182		case IRQ_TYPE_LEVEL_HIGH:
   183			edge = GPIO_INT_HIGH_LEV;
   184			break;
   185		default:
   186			return -EINVAL;
   187		}
   188	
 > 189		spin_lock_irqsave(&port->gc.bgpio_lock, flags);
   190	
   191		if (GPIO_EDGE_SEL >= 0) {
   192			val = readl(port->base + GPIO_EDGE_SEL);
   193			if (edge == GPIO_INT_BOTH_EDGES)
   194				writel(val | (1 << gpio_idx),
   195					port->base + GPIO_EDGE_SEL);
   196			else
   197				writel(val & ~(1 << gpio_idx),
   198					port->base + GPIO_EDGE_SEL);
   199		}
   200	
   201		if (edge != GPIO_INT_BOTH_EDGES) {
   202			reg += GPIO_ICR1 + ((gpio_idx & 0x10) >> 2); /* lower or upper register */
   203			bit = gpio_idx & 0xf;
   204			val = readl(reg) & ~(0x3 << (bit << 1));
   205			writel(val | (edge << (bit << 1)), reg);
   206		}
   207	
   208		writel(1 << gpio_idx, port->base + GPIO_ISR);
   209	
   210		spin_unlock_irqrestore(&port->gc.bgpio_lock, flags);
   211	
   212		return 0;
   213	}
   214
kernel test robot July 24, 2022, 6:42 p.m. UTC | #4 
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-rc7 next-20220722]
[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-011230
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
config: openrisc-buildonly-randconfig-r005-20220724 (https://download.01.org/0day-ci/archive/20220725/202207250224.0POKsKcZ-lkp@intel.com/config)
compiler: or1k-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/af6c815e16849c7ec370f755916d06a6b1e5b759
        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-011230
        git checkout af6c815e16849c7ec370f755916d06a6b1e5b759
        # 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=openrisc 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:189:27: error: passing argument 1 of 'spinlock_check' from incompatible pointer type [-Werror=incompatible-pointer-types]
     189 |         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:189:9: note: in expansion of macro 'spin_lock_irqsave'
     189 |         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:210:32: error: passing argument 1 of 'spin_unlock_irqrestore' from incompatible pointer type [-Werror=incompatible-pointer-types]
     210 |         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:222:27: error: passing argument 1 of 'spinlock_check' from incompatible pointer type [-Werror=incompatible-pointer-types]
     222 |         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:222:9: note: in expansion of macro 'spin_lock_irqsave'
     222 |         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:242:32: error: passing argument 1 of 'spin_unlock_irqrestore' from incompatible pointer type [-Werror=incompatible-pointer-types]
     242 |         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 +189 drivers/gpio/gpio-mxc.c

   145	
   146	static int gpio_set_irq_type(struct irq_data *d, u32 type)
   147	{
   148		struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
   149		struct mxc_gpio_port *port = gc->private;
   150		unsigned long flags;
   151		u32 bit, val;
   152		u32 gpio_idx = d->hwirq;
   153		int edge;
   154		void __iomem *reg = port->base;
   155	
   156		port->both_edges &= ~(1 << gpio_idx);
   157		switch (type) {
   158		case IRQ_TYPE_EDGE_RISING:
   159			edge = GPIO_INT_RISE_EDGE;
   160			break;
   161		case IRQ_TYPE_EDGE_FALLING:
   162			edge = GPIO_INT_FALL_EDGE;
   163			break;
   164		case IRQ_TYPE_EDGE_BOTH:
   165			if (GPIO_EDGE_SEL >= 0) {
   166				edge = GPIO_INT_BOTH_EDGES;
   167			} else {
   168				val = port->gc.get(&port->gc, gpio_idx);
   169				if (val) {
   170					edge = GPIO_INT_LOW_LEV;
   171					pr_debug("mxc: set GPIO %d to low trigger\n", gpio_idx);
   172				} else {
   173					edge = GPIO_INT_HIGH_LEV;
   174					pr_debug("mxc: set GPIO %d to high trigger\n", gpio_idx);
   175				}
   176				port->both_edges |= 1 << gpio_idx;
   177			}
   178			break;
   179		case IRQ_TYPE_LEVEL_LOW:
   180			edge = GPIO_INT_LOW_LEV;
   181			break;
   182		case IRQ_TYPE_LEVEL_HIGH:
   183			edge = GPIO_INT_HIGH_LEV;
   184			break;
   185		default:
   186			return -EINVAL;
   187		}
   188	
 > 189		spin_lock_irqsave(&port->gc.bgpio_lock, flags);
   190	
   191		if (GPIO_EDGE_SEL >= 0) {
   192			val = readl(port->base + GPIO_EDGE_SEL);
   193			if (edge == GPIO_INT_BOTH_EDGES)
   194				writel(val | (1 << gpio_idx),
   195					port->base + GPIO_EDGE_SEL);
   196			else
   197				writel(val & ~(1 << gpio_idx),
   198					port->base + GPIO_EDGE_SEL);
   199		}
   200	
   201		if (edge != GPIO_INT_BOTH_EDGES) {
   202			reg += GPIO_ICR1 + ((gpio_idx & 0x10) >> 2); /* lower or upper register */
   203			bit = gpio_idx & 0xf;
   204			val = readl(reg) & ~(0x3 << (bit << 1));
   205			writel(val | (edge << (bit << 1)), reg);
   206		}
   207	
   208		writel(1 << gpio_idx, port->base + GPIO_ISR);
   209	
 > 210		spin_unlock_irqrestore(&port->gc.bgpio_lock, flags);
   211	
   212		return 0;
   213	}
   214
Marc Zyngier July 25, 2022, 6:53 a.m. UTC | #5 
On Sun, 24 Jul 2022 19:15:26 +0100,
Marek Vasut <marex@denx.de> wrote:
> 
> On 7/24/22 19:50, Marc Zyngier wrote:
> 
> [...]
> 
> >> +++ b/drivers/gpio/gpio-mxc.c
> >> @@ -147,6 +147,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 +186,8 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
> >>   		return -EINVAL;
> >>   	}
> >>   +	spin_lock_irqsave(&port->gc.bgpio_lock, flags);
> > 
> > In my tree, bgpio is a raw spinlock, and has been since 3c938cc5cebcb.
> > 
> > Now, looking a bit closer at this code, I have to withdraw my earlier
> > comment about the lack of mutual exclusion in the existing code. All
> > writes are of the form:
> > 
> > 	writel(single_bit_mask, some_addr + MXS_{SET,CLR});
> > 
> > which indicates that the write side can be accessed with a hot-bit
> > pattern, avoiding a RWM pattern and thus the need for a lock.
> 
> Only for the ISR/IMR, not for the GDIR register, that's why the locks
> are added only around the RMW which don't have these "hot bits".

Only your patch adds any GDIR access.

> > Your second patch, however requires the lock. I'm not sure it is safe
> > to do after the interrupt type has been configured though. You may
> > want to refer to the TRM for this.
> 
> There is in fact another unprotected RMW in gpio_set_irq_type() , look
> for GPIO_EDGE_SEL, so the locks should be valid as they are now, right
> ?

I seem to be confused with gpio-mxs.c, and gpio-mxc indeed needs the
lock.

However, you have totally ignored my earlier comments in your v4:

- This doesn't compile, as bgpio_lock has been changed to a *raw*
  spinlock. You obviously haven't even bothered testing your patch.

- I asked for a cover letter for any series with multiple patch.
  That's not exactly a new requirement.

So we got 4 versions in just over 24 hours, none of which actually
work. Do you see the overarching problem?

	M.
Marek Vasut July 25, 2022, 9:57 a.m. UTC | #6 
On 7/25/22 08:53, Marc Zyngier wrote:
> On Sun, 24 Jul 2022 19:15:26 +0100,
> Marek Vasut <marex@denx.de> wrote:
>>
>> On 7/24/22 19:50, Marc Zyngier wrote:
>>
>> [...]
>>
>>>> +++ b/drivers/gpio/gpio-mxc.c
>>>> @@ -147,6 +147,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 +186,8 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>>>>    		return -EINVAL;
>>>>    	}
>>>>    +	spin_lock_irqsave(&port->gc.bgpio_lock, flags);
>>>
>>> In my tree, bgpio is a raw spinlock, and has been since 3c938cc5cebcb.
>>>
>>> Now, looking a bit closer at this code, I have to withdraw my earlier
>>> comment about the lack of mutual exclusion in the existing code. All
>>> writes are of the form:
>>>
>>> 	writel(single_bit_mask, some_addr + MXS_{SET,CLR});
>>>
>>> which indicates that the write side can be accessed with a hot-bit
>>> pattern, avoiding a RWM pattern and thus the need for a lock.
>>
>> Only for the ISR/IMR, not for the GDIR register, that's why the locks
>> are added only around the RMW which don't have these "hot bits".
> 
> Only your patch adds any GDIR access.
> 
>>> Your second patch, however requires the lock. I'm not sure it is safe
>>> to do after the interrupt type has been configured though. You may
>>> want to refer to the TRM for this.
>>
>> There is in fact another unprotected RMW in gpio_set_irq_type() , look
>> for GPIO_EDGE_SEL, so the locks should be valid as they are now, right
>> ?
> 
> I seem to be confused with gpio-mxs.c, and gpio-mxc indeed needs the
> lock.
> 
> However, you have totally ignored my earlier comments in your v4:
> 
> - This doesn't compile, as bgpio_lock has been changed to a *raw*
>    spinlock. You obviously haven't even bothered testing your patch.

Yes indeed, I tested every single one on 5.18.y . I noticed the raw 
spinlock change is only in next.

> - I asked for a cover letter for any series with multiple patch.
>    That's not exactly a new requirement.
> 
> So we got 4 versions in just over 24 hours, none of which actually
> work. Do you see the overarching problem?

Lemme rebase this on next and send v5.
Marc Zyngier July 25, 2022, 10:06 a.m. UTC | #7 
On Mon, 25 Jul 2022 10:57:32 +0100,
Marek Vasut <marex@denx.de> wrote:
> 
> On 7/25/22 08:53, Marc Zyngier wrote:
> > However, you have totally ignored my earlier comments in your v4:
> > 
> > - This doesn't compile, as bgpio_lock has been changed to a *raw*
> >    spinlock. You obviously haven't even bothered testing your patch.
> 
> Yes indeed, I tested every single one on 5.18.y . I noticed the raw
> spinlock change is only in next.

$ git describe --contains 3c938cc5cebcb --match=v*
v5.19-rc1~134^2~25

Only in -next? Not quite.

> 
> > - I asked for a cover letter for any series with multiple patch.
> >    That's not exactly a new requirement.
> > 
> > So we got 4 versions in just over 24 hours, none of which actually
> > work. Do you see the overarching problem?
> 
> Lemme rebase this on next and send v5.

Rebasing on -rc1 is the right thing to do. You should never base
something on -next, as this is a moving target.

	M.
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index c871602fc5ba9..74a50139c9202 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -147,6 +147,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 +186,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 +207,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 +238,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 */