* [PATCH v4 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
@ 2022-07-24 22:49 Marek Vasut
2022-07-24 22:49 ` [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode Marek Vasut
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Marek Vasut @ 2022-07-24 22:49 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
V4: Include linux/spinlock.h
---
drivers/gpio/gpio-mxc.c | 11 +++++++++++
1 file changed, 11 insertions(+)
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 */
--
2.35.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode
2022-07-24 22:49 [PATCH v4 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock Marek Vasut
@ 2022-07-24 22:49 ` Marek Vasut
2022-07-26 8:15 ` Linus Walleij
2022-07-26 20:59 ` Linus Walleij
2022-07-25 20:37 ` [PATCH v4 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock Andy Shevchenko
2022-07-28 22:21 ` kernel test robot
2 siblings, 2 replies; 13+ messages in thread
From: Marek Vasut @ 2022-07-24 22:49 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
V4: No change
---
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 75e7aceecac0a..f4cadbf30c6b8 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -149,7 +149,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;
@@ -208,6 +208,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] 13+ messages in thread
* Re: [PATCH v4 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
2022-07-24 22:49 [PATCH v4 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock Marek Vasut
2022-07-24 22:49 ` [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode Marek Vasut
@ 2022-07-25 20:37 ` Andy Shevchenko
2022-07-25 22:30 ` Linus Walleij
2022-07-28 22:21 ` kernel test robot
2 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2022-07-25 20:37 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 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?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
2022-07-25 20:37 ` [PATCH v4 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock Andy Shevchenko
@ 2022-07-25 22:30 ` Linus Walleij
2022-07-25 22:33 ` Linus Walleij
2022-07-25 23:53 ` Marek Vasut
0 siblings, 2 replies; 13+ messages in thread
From: Linus Walleij @ 2022-07-25 22:30 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: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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
2022-07-25 22:30 ` Linus Walleij
@ 2022-07-25 22:33 ` Linus Walleij
2022-07-25 23:53 ` Marek Vasut
1 sibling, 0 replies; 13+ messages in thread
From: Linus Walleij @ 2022-07-25 22:33 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 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
2022-07-25 22:30 ` Linus Walleij
2022-07-25 22:33 ` Linus Walleij
@ 2022-07-25 23:53 ` Marek Vasut
1 sibling, 0 replies; 13+ messages in thread
From: Marek Vasut @ 2022-07-25 23:53 UTC (permalink / raw)
To: Linus Walleij, Andy Shevchenko
Cc: open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Loic Poulain,
Marc Zyngier, NXP Linux Team, Peng Fan, Shawn Guo
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode
2022-07-24 22:49 ` [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode Marek Vasut
@ 2022-07-26 8:15 ` Linus Walleij
2022-07-26 14:42 ` Marek Vasut
2022-07-26 20:59 ` Linus Walleij
1 sibling, 1 reply; 13+ messages in thread
From: Linus Walleij @ 2022-07-26 8:15 UTC (permalink / raw)
To: Marek Vasut, Hans Verkuil
Cc: linux-gpio, Bartosz Golaszewski, Loic Poulain, Marc Zyngier,
NXP Linux Team, Peng Fan, Shawn Guo
On Mon, Jul 25, 2022 at 12:50 AM 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.
>
> 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
> V4: No change
I understand what you are trying to achieve, and it makes sense.
There's is just this one generic GPIO-based driver that makes me
a little bit nervous here.
Consider:
drivers/media/cec/platform/cec-gpio/cec-gpio.c
Look what the driver is doing with the gpiod_* operations on it's
cec->cec_gpio.
A certain GPIO pin is switched back and forth between input and
output and in input mode, it is used to generate interrupts as well.
Will this still work fine with the MXC driver after this change?
At least it will be set to input mode twice, but I suppose that is
fine, it's not your fault that the frameworks are orthogonal.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode
2022-07-26 8:15 ` Linus Walleij
@ 2022-07-26 14:42 ` Marek Vasut
2022-07-26 15:13 ` Hans Verkuil
0 siblings, 1 reply; 13+ messages in thread
From: Marek Vasut @ 2022-07-26 14:42 UTC (permalink / raw)
To: Linus Walleij, Hans Verkuil
Cc: linux-gpio, Bartosz Golaszewski, Loic Poulain, Marc Zyngier,
NXP Linux Team, Peng Fan, Shawn Guo
On 7/26/22 10:15, Linus Walleij wrote:
> On Mon, Jul 25, 2022 at 12:50 AM 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.
>>
>> 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
>> V4: No change
>
> I understand what you are trying to achieve, and it makes sense.
>
> There's is just this one generic GPIO-based driver that makes me
> a little bit nervous here.
>
> Consider:
> drivers/media/cec/platform/cec-gpio/cec-gpio.c
> Look what the driver is doing with the gpiod_* operations on it's
> cec->cec_gpio.
>
> A certain GPIO pin is switched back and forth between input and
> output and in input mode, it is used to generate interrupts as well.
>
> Will this still work fine with the MXC driver after this change?
> At least it will be set to input mode twice, but I suppose that is
> fine, it's not your fault that the frameworks are orthogonal.
Ugh. I don't see why it shouldn't work, esp. if the CEC driver controls
the direction and the default is input, but I wonder what other corner
cases there are.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode
2022-07-26 14:42 ` Marek Vasut
@ 2022-07-26 15:13 ` Hans Verkuil
2022-07-29 3:01 ` Marek Vasut
0 siblings, 1 reply; 13+ messages in thread
From: Hans Verkuil @ 2022-07-26 15:13 UTC (permalink / raw)
To: Marek Vasut, Linus Walleij
Cc: linux-gpio, Bartosz Golaszewski, Loic Poulain, Marc Zyngier,
NXP Linux Team, Peng Fan, Shawn Guo
On 7/26/22 16:42, Marek Vasut wrote:
> On 7/26/22 10:15, Linus Walleij wrote:
>> On Mon, Jul 25, 2022 at 12:50 AM 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.
>>>
>>> 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
>>> V4: No change
>>
>> I understand what you are trying to achieve, and it makes sense.
>>
>> There's is just this one generic GPIO-based driver that makes me
>> a little bit nervous here.
>>
>> Consider:
>> drivers/media/cec/platform/cec-gpio/cec-gpio.c
>> Look what the driver is doing with the gpiod_* operations on it's
>> cec->cec_gpio.
>>
>> A certain GPIO pin is switched back and forth between input and
>> output and in input mode, it is used to generate interrupts as well.
>>
>> Will this still work fine with the MXC driver after this change?
>> At least it will be set to input mode twice, but I suppose that is
>> fine, it's not your fault that the frameworks are orthogonal.
>
> Ugh. I don't see why it shouldn't work, esp. if the CEC driver controls the direction and the default is input, but I wonder what other corner cases there are.
FYI: you can easily test cec-gpio by adding something along these lines to the dts:
cec-gpio {
compatible = "cec-gpio";
cec-gpios = <&gpio 6 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
};
(this is from a Raspberry Pi). As long as the cec-gpios pin can be configured as a
pull up, then you're OK. It doesn't have to be connected to anything.
If the cec-gpio driver is enabled as well, then you should see a /dev/cec0 device.
If you can run 'cec-ctl --playback -p 1.0.0.0', then it works.
Another driver that switches direction is drivers/gpu/drm/i2c/tda998x_drv.c
where the irq line has to be configured as an output during calibration.
See tda998x_cec_calibration().
Regards,
Hans
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode
2022-07-24 22:49 ` [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode Marek Vasut
2022-07-26 8:15 ` Linus Walleij
@ 2022-07-26 20:59 ` Linus Walleij
1 sibling, 0 replies; 13+ messages in thread
From: Linus Walleij @ 2022-07-26 20:59 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-gpio, Bartosz Golaszewski, Loic Poulain, Marc Zyngier,
NXP Linux Team, Peng Fan, Shawn Guo
On Mon, Jul 25, 2022 at 12:50 AM 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.
>
> 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>
It's up to the driver to deal with the final hardware state, and the
author knows what he's doing so:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock
2022-07-24 22:49 [PATCH v4 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock Marek Vasut
2022-07-24 22:49 ` [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode Marek Vasut
2022-07-25 20:37 ` [PATCH v4 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock Andy Shevchenko
@ 2022-07-28 22:21 ` kernel test robot
2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2022-07-28 22:21 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-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
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode
2022-07-26 15:13 ` Hans Verkuil
@ 2022-07-29 3:01 ` Marek Vasut
2022-07-29 6:56 ` Hans Verkuil
0 siblings, 1 reply; 13+ messages in thread
From: Marek Vasut @ 2022-07-29 3:01 UTC (permalink / raw)
To: Hans Verkuil, Linus Walleij
Cc: linux-gpio, Bartosz Golaszewski, Loic Poulain, Marc Zyngier,
NXP Linux Team, Peng Fan, Shawn Guo
On 7/26/22 17:13, Hans Verkuil wrote:
[...]
> FYI: you can easily test cec-gpio by adding something along these lines to the dts:
>
> cec-gpio {
> compatible = "cec-gpio";
> cec-gpios = <&gpio 6 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> };
>
> (this is from a Raspberry Pi). As long as the cec-gpios pin can be configured as a
> pull up, then you're OK. It doesn't have to be connected to anything.
>
> If the cec-gpio driver is enabled as well, then you should see a /dev/cec0 device.
>
> If you can run 'cec-ctl --playback -p 1.0.0.0', then it works.
>
> Another driver that switches direction is drivers/gpu/drm/i2c/tda998x_drv.c
> where the irq line has to be configured as an output during calibration.
> See tda998x_cec_calibration().
Does this look OK ?
$ cec-ctl --playback -p 1.0.0.0
Driver Info:
Driver Name : cec-gpio
Adapter Name : cec-gpio
Capabilities : 0x000000af
Physical Address
Logical Addresses
Transmit
Passthrough
Monitor All
Monitor Pin
Driver version : 5.19.0
Available Logical Addresses: 4
Connector Info : None
Physical Address : 1.0.0.0
Logical Address Mask : 0x0010
CEC Version : 2.0
Vendor ID : 0x000c03 (HDMI)
OSD Name : 'Playback'
Logical Addresses : 1 (Allow RC Passthrough)
Logical Address : 4 (Playback Device 1)
Primary Device Type : Playback
Logical Address Type : Playback
All Device Types : Playback
RC TV Profile : None
Device Features :
None
$ cec-compliance
Driver Info:
Driver Name : cec-gpio
Adapter Name : cec-gpio
Capabilities : 0x000000af
Physical Address
Logical Addresses
Transmit
Passthrough
Monitor All
Monitor Pin
Driver version : 5.19.0
Available Logical Addresses: 4
Connector Info : None
Physical Address : 1.0.0.0
Logical Address Mask : 0x0010
CEC Version : 2.0
Vendor ID : 0x000c03 (HDMI)
OSD Name : 'Playback'
Logical Addresses : 1 (Allow RC Passthrough)
Logical Address : 4 (Playback Device 1)
Primary Device Type : Playback
Logical Address Type : Playback
All Device Types : Playback
RC TV Profile : None
Device Features :
None
Compliance test for cec-gpio device /dev/cec0:
The test results mean the following:
OK Supported correctly by the device.
OK (Not Supported) Not supported and not mandatory for the
device.
OK (Presumed) Presumably supported. Manually check to
confirm.
OK (Unexpected) Supported correctly but is not expected
to be supported for this device.
OK (Refused) Supported by the device, but was refused.
OK (Expected Failure) Failed but this was expected (see -e option).
FAIL Failed and was expected to be supported
by this device.
Find remote devices:
Polling: OK
FAIL: No remote devices found, exiting.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode
2022-07-29 3:01 ` Marek Vasut
@ 2022-07-29 6:56 ` Hans Verkuil
0 siblings, 0 replies; 13+ messages in thread
From: Hans Verkuil @ 2022-07-29 6:56 UTC (permalink / raw)
To: Marek Vasut, Linus Walleij
Cc: linux-gpio, Bartosz Golaszewski, Loic Poulain, Marc Zyngier,
NXP Linux Team, Peng Fan, Shawn Guo
Hi Marek,
On 29/07/2022 05:01, Marek Vasut wrote:
> On 7/26/22 17:13, Hans Verkuil wrote:
>
> [...]
>
>> FYI: you can easily test cec-gpio by adding something along these lines to the dts:
>>
>> cec-gpio {
>> compatible = "cec-gpio";
>> cec-gpios = <&gpio 6 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
>> };
>>
>> (this is from a Raspberry Pi). As long as the cec-gpios pin can be configured as a
>> pull up, then you're OK. It doesn't have to be connected to anything.
>>
>> If the cec-gpio driver is enabled as well, then you should see a /dev/cec0 device.
>>
>> If you can run 'cec-ctl --playback -p 1.0.0.0', then it works.
>>
>> Another driver that switches direction is drivers/gpu/drm/i2c/tda998x_drv.c
>> where the irq line has to be configured as an output during calibration.
>> See tda998x_cec_calibration().
>
> Does this look OK ?
>
> $ cec-ctl --playback -p 1.0.0.0
> Driver Info:
> Driver Name : cec-gpio
> Adapter Name : cec-gpio
> Capabilities : 0x000000af
> Physical Address
> Logical Addresses
> Transmit
> Passthrough
> Monitor All
> Monitor Pin
> Driver version : 5.19.0
> Available Logical Addresses: 4
> Connector Info : None
> Physical Address : 1.0.0.0
> Logical Address Mask : 0x0010
> CEC Version : 2.0
> Vendor ID : 0x000c03 (HDMI)
> OSD Name : 'Playback'
> Logical Addresses : 1 (Allow RC Passthrough)
>
> Logical Address : 4 (Playback Device 1)
> Primary Device Type : Playback
> Logical Address Type : Playback
> All Device Types : Playback
> RC TV Profile : None
> Device Features :
> None
>
> $ cec-compliance
> Driver Info:
> Driver Name : cec-gpio
> Adapter Name : cec-gpio
> Capabilities : 0x000000af
> Physical Address
> Logical Addresses
> Transmit
> Passthrough
> Monitor All
> Monitor Pin
> Driver version : 5.19.0
> Available Logical Addresses: 4
> Connector Info : None
> Physical Address : 1.0.0.0
> Logical Address Mask : 0x0010
> CEC Version : 2.0
> Vendor ID : 0x000c03 (HDMI)
> OSD Name : 'Playback'
> Logical Addresses : 1 (Allow RC Passthrough)
>
> Logical Address : 4 (Playback Device 1)
> Primary Device Type : Playback
> Logical Address Type : Playback
> All Device Types : Playback
> RC TV Profile : None
> Device Features :
> None
>
> Compliance test for cec-gpio device /dev/cec0:
>
> The test results mean the following:
> OK Supported correctly by the device.
> OK (Not Supported) Not supported and not mandatory for the device.
> OK (Presumed) Presumably supported. Manually check to confirm.
> OK (Unexpected) Supported correctly but is not expected to be supported for this device.
> OK (Refused) Supported by the device, but was refused.
> OK (Expected Failure) Failed but this was expected (see -e option).
> FAIL Failed and was expected to be supported by this device.
>
> Find remote devices:
> Polling: OK
>
> FAIL: No remote devices found, exiting.
Yes, that looks fine.
Regards,
Hans
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-07-29 6:56 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-24 22:49 [PATCH v4 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock Marek Vasut
2022-07-24 22:49 ` [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode Marek Vasut
2022-07-26 8:15 ` Linus Walleij
2022-07-26 14:42 ` Marek Vasut
2022-07-26 15:13 ` Hans Verkuil
2022-07-29 3:01 ` Marek Vasut
2022-07-29 6:56 ` Hans Verkuil
2022-07-26 20:59 ` Linus Walleij
2022-07-25 20:37 ` [PATCH v4 1/2] gpio: mxc: Protect GPIO irqchip RMW with bgpio spinlock Andy Shevchenko
2022-07-25 22:30 ` Linus Walleij
2022-07-25 22:33 ` Linus Walleij
2022-07-25 23:53 ` Marek Vasut
2022-07-28 22:21 ` 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.