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

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

[Marek Vasut]

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

[PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode

[Marek Vasut]

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

Re: [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode

[Linus Walleij]

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

Re: [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode

[Marek Vasut]

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.

Re: [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode

[Hans Verkuil]

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

Re: [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode

[Marek Vasut]

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.

Re: [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode

[Hans Verkuil]

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

Re: [PATCH v4 2/2] gpio: mxc: Always set GPIOs used as interrupt source to INPUT mode

[Linus Walleij]

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

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

[Andy Shevchenko]

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

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

[Linus Walleij]

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

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

[Linus Walleij]

On Tue, Jul 26, 2022 at 12:30 AM Linus Walleij <linus.walleij@linaro.org> wrote:

> Marek: could you see if the irqchip part of the driver could
> use its own raw spinlock?

Oh I see v5 already does, great!

Linus

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

[Marek Vasut]

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.

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

[kernel test robot]

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