* [PATCH v3 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
@ 2022-07-24 17:10 Marek Vasut
2022-07-24 17:10 ` [PATCH v3 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode Marek Vasut
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Marek Vasut @ 2022-07-24 17:10 UTC (permalink / raw)
To: linux-gpio
Cc: Marek Vasut, Bartosz Golaszewski, Linus Walleij, Loic Poulain,
Marc Zyngier, NXP Linux Team, Peng Fan, Shawn Guo
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);
+
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 */
--
2.35.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode
2022-07-24 17:10 [PATCH v3 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock Marek Vasut
@ 2022-07-24 17:10 ` Marek Vasut
2022-07-25 20:36 ` Andy Shevchenko
2022-07-24 17:50 ` [PATCH v3 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock Marc Zyngier
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Marek Vasut @ 2022-07-24 17:10 UTC (permalink / raw)
To: linux-gpio
Cc: Marek Vasut, Bartosz Golaszewski, Linus Walleij, Loic Poulain,
Marc Zyngier, NXP Linux Team, Peng Fan, Shawn Guo
Always configure GPIO pins which are used as interrupt source as INPUTs.
In case the default pin configuration is OUTPUT, or the prior stage does
configure the pins as OUTPUT, then Linux will not reconfigure the pin as
INPUT and no interrupts are received.
Always configure interrupt source GPIO pin as input to fix the above case.
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>
---
V2: Actually update and clear bits in GDIR register
V3: Rebase on top of new patch 1/2, expand CC list, add Fixes tag
---
drivers/gpio/gpio-mxc.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 74a50139c9202..945e8a2efb896 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -148,7 +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 bit, val, dir;
u32 gpio_idx = d->hwirq;
int edge;
void __iomem *reg = port->base;
@@ -207,6 +207,10 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
writel(1 << gpio_idx, port->base + GPIO_ISR);
+ dir = readl(port->base + GPIO_GDIR);
+ dir &= ~BIT(gpio_idx);
+ writel(dir, port->base + GPIO_GDIR);
+
spin_unlock_irqrestore(&port->gc.bgpio_lock, flags);
return 0;
--
2.35.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
2022-07-24 17:10 [PATCH v3 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock Marek Vasut
2022-07-24 17:10 ` [PATCH v3 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode Marek Vasut
@ 2022-07-24 17:50 ` Marc Zyngier
2022-07-24 18:15 ` Marek Vasut
2022-07-24 18:32 ` kernel test robot
2022-07-24 18:42 ` kernel test robot
3 siblings, 1 reply; 12+ messages in thread
From: Marc Zyngier @ 2022-07-24 17:50 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-gpio, Bartosz Golaszewski, Linus Walleij, Loic Poulain,
NXP Linux Team, Peng Fan, Shawn Guo
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.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
2022-07-24 17:50 ` [PATCH v3 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock Marc Zyngier
@ 2022-07-24 18:15 ` Marek Vasut
2022-07-25 6:53 ` Marc Zyngier
0 siblings, 1 reply; 12+ messages in thread
From: Marek Vasut @ 2022-07-24 18:15 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-gpio, Bartosz Golaszewski, Linus Walleij, Loic Poulain,
NXP Linux Team, Peng Fan, Shawn Guo
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 ?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
2022-07-24 17:10 [PATCH v3 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock Marek Vasut
2022-07-24 17:10 ` [PATCH v3 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode Marek Vasut
2022-07-24 17:50 ` [PATCH v3 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock Marc Zyngier
@ 2022-07-24 18:32 ` kernel test robot
2022-07-24 18:42 ` kernel test robot
3 siblings, 0 replies; 12+ messages in thread
From: kernel test robot @ 2022-07-24 18:32 UTC (permalink / raw)
To: Marek Vasut, linux-gpio
Cc: llvm, kbuild-all, Marek Vasut, Bartosz Golaszewski,
Linus Walleij, Loic Poulain, Marc Zyngier, NXP Linux Team,
Peng Fan, Shawn Guo
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
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
2022-07-24 17:10 [PATCH v3 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock Marek Vasut
` (2 preceding siblings ...)
2022-07-24 18:32 ` kernel test robot
@ 2022-07-24 18:42 ` kernel test robot
3 siblings, 0 replies; 12+ messages in thread
From: kernel test robot @ 2022-07-24 18:42 UTC (permalink / raw)
To: Marek Vasut, linux-gpio
Cc: kbuild-all, Marek Vasut, Bartosz Golaszewski, Linus Walleij,
Loic Poulain, Marc Zyngier, NXP Linux Team, Peng Fan, Shawn Guo
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
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
2022-07-24 18:15 ` Marek Vasut
@ 2022-07-25 6:53 ` Marc Zyngier
2022-07-25 9:57 ` Marek Vasut
0 siblings, 1 reply; 12+ messages in thread
From: Marc Zyngier @ 2022-07-25 6:53 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-gpio, Bartosz Golaszewski, Linus Walleij, Loic Poulain,
NXP Linux Team, Peng Fan, Shawn Guo
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.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
2022-07-25 6:53 ` Marc Zyngier
@ 2022-07-25 9:57 ` Marek Vasut
2022-07-25 10:06 ` Marc Zyngier
0 siblings, 1 reply; 12+ messages in thread
From: Marek Vasut @ 2022-07-25 9:57 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-gpio, Bartosz Golaszewski, Linus Walleij, Loic Poulain,
NXP Linux Team, Peng Fan, Shawn Guo
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
2022-07-25 9:57 ` Marek Vasut
@ 2022-07-25 10:06 ` Marc Zyngier
0 siblings, 0 replies; 12+ messages in thread
From: Marc Zyngier @ 2022-07-25 10:06 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-gpio, Bartosz Golaszewski, Linus Walleij, Loic Poulain,
NXP Linux Team, Peng Fan, Shawn Guo
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.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode
2022-07-24 17:10 ` [PATCH v3 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode Marek Vasut
@ 2022-07-25 20:36 ` Andy Shevchenko
2022-07-25 21:26 ` Marek Vasut
2022-07-26 8:22 ` Linus Walleij
0 siblings, 2 replies; 12+ messages in thread
From: Andy Shevchenko @ 2022-07-25 20:36 UTC (permalink / raw)
To: Marek Vasut
Cc: open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Linus Walleij,
Loic Poulain, Marc Zyngier, NXP Linux Team, Peng Fan, Shawn Guo
On Sun, Jul 24, 2022 at 7:21 PM Marek Vasut <marex@denx.de> wrote:
>
> Always configure GPIO pins which are used as interrupt source as INPUTs.
> In case the default pin configuration is OUTPUT, or the prior stage does
> configure the pins as OUTPUT, then Linux will not reconfigure the pin as
> INPUT and no interrupts are received.
>
> Always configure interrupt source GPIO pin as input to fix the above case.
the interrupt
...
I'm wondering if it's configured as output (by who?) shouldn't you
issue a warning?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode
2022-07-25 20:36 ` Andy Shevchenko
@ 2022-07-25 21:26 ` Marek Vasut
2022-07-26 8:22 ` Linus Walleij
1 sibling, 0 replies; 12+ messages in thread
From: Marek Vasut @ 2022-07-25 21:26 UTC (permalink / raw)
To: Andy Shevchenko
Cc: open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Linus Walleij,
Loic Poulain, Marc Zyngier, NXP Linux Team, Peng Fan, Shawn Guo
On 7/25/22 22:36, Andy Shevchenko wrote:
> On Sun, Jul 24, 2022 at 7:21 PM Marek Vasut <marex@denx.de> wrote:
>>
>> Always configure GPIO pins which are used as interrupt source as INPUTs.
>> In case the default pin configuration is OUTPUT, or the prior stage does
>> configure the pins as OUTPUT, then Linux will not reconfigure the pin as
>> INPUT and no interrupts are received.
>>
>> Always configure interrupt source GPIO pin as input to fix the above case.
>
> the interrupt
>
> ...
>
> I'm wondering if it's configured as output (by who?) shouldn't you
> issue a warning?
Probably not, I can think of valid use-case where the pin can be
configured as output by the prior stage e.g. to operate an LED, but then
reconfigured as input by Linux to e.g. sample a button. There is
hardware which shares button and LED on the same GPIO line to my knowledge.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode
2022-07-25 20:36 ` Andy Shevchenko
2022-07-25 21:26 ` Marek Vasut
@ 2022-07-26 8:22 ` Linus Walleij
1 sibling, 0 replies; 12+ messages in thread
From: Linus Walleij @ 2022-07-26 8:22 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Marek Vasut, open list:GPIO SUBSYSTEM, Bartosz Golaszewski,
Loic Poulain, Marc Zyngier, NXP Linux Team, Peng Fan, Shawn Guo
On Mon, Jul 25, 2022 at 10:36 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sun, Jul 24, 2022 at 7:21 PM Marek Vasut <marex@denx.de> wrote:
> >
> > Always configure GPIO pins which are used as interrupt source as INPUTs.
> > In case the default pin configuration is OUTPUT, or the prior stage does
> > configure the pins as OUTPUT, then Linux will not reconfigure the pin as
> > INPUT and no interrupts are received.
> >
> > Always configure interrupt source GPIO pin as input to fix the above case.
>
> the interrupt
>
> ...
>
> I'm wondering if it's configured as output (by who?) shouldn't you
> issue a warning?
That could be the power-on default or boot loader that put it
into output mode I think, so it can be something outside of the kernel
and outside of our control. We just adjust the reality to the map
and go on :P
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-07-26 8:22 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-24 17:10 [PATCH v3 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock Marek Vasut
2022-07-24 17:10 ` [PATCH v3 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode Marek Vasut
2022-07-25 20:36 ` Andy Shevchenko
2022-07-25 21:26 ` Marek Vasut
2022-07-26 8:22 ` Linus Walleij
2022-07-24 17:50 ` [PATCH v3 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock Marc Zyngier
2022-07-24 18:15 ` Marek Vasut
2022-07-25 6:53 ` Marc Zyngier
2022-07-25 9:57 ` Marek Vasut
2022-07-25 10:06 ` Marc Zyngier
2022-07-24 18:32 ` kernel test robot
2022-07-24 18:42 ` kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.