[PATCH] gpio: fix locking open drain IRQ lines

[PATCH] gpio: fix locking open drain IRQ lines

[Linus Walleij]

We provided the right semantics on open drain lines being
by definition output but incidentally the irq set up function
would only allow IRQs on lines that were "not output".

Fix the semantics to allow output open drain lines to be used
for IRQs.

Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Russell King <linux@armlinux.org.uk>
Cc: stable@vger.kernel.org
Fixes: 256efaea1fdc ("gpiolib: fix up emulated open drain outputs")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index b4b5792fe2ff..edd74ff31cea 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4220,7 +4220,9 @@ int gpiochip_lock_as_irq(struct gpio_chip *gc, unsigned int offset)
 		}
 	}
 
-	if (test_bit(FLAG_IS_OUT, &desc->flags)) {
+	/* To be valid for IRQ the line needs to be input or open drain */
+	if (test_bit(FLAG_IS_OUT, &desc->flags) &&
+	    !test_bit(FLAG_OPEN_DRAIN, &desc->flags)) {
 		chip_err(gc,
 			 "%s: tried to flag a GPIO set as output for IRQ\n",
 			 __func__);
-- 
2.25.4

Re: [PATCH] gpio: fix locking open drain IRQ lines

[Russell King - ARM Linux admin]

On Wed, May 27, 2020 at 04:07:58PM +0200, Linus Walleij wrote:
> We provided the right semantics on open drain lines being
> by definition output but incidentally the irq set up function
> would only allow IRQs on lines that were "not output".
> 
> Fix the semantics to allow output open drain lines to be used
> for IRQs.
> 
> Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: stable@vger.kernel.org
> Fixes: 256efaea1fdc ("gpiolib: fix up emulated open drain outputs")

As I've pointed out in the reporting thread, I don't think it can be
justified as a regression - it's a bug in its own right that has been
discovered by unifying the gpiolib semantics, since the cec-gpio code
will fail on hardware that can provide real open-drain outputs
irrespective of that commit.

So, you're really fixing a deeper problem that was never discovered
until gpiolib's semantics were fixed to be more uniform.

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/gpiolib.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index b4b5792fe2ff..edd74ff31cea 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -4220,7 +4220,9 @@ int gpiochip_lock_as_irq(struct gpio_chip *gc, unsigned int offset)
>  		}
>  	}
>  
> -	if (test_bit(FLAG_IS_OUT, &desc->flags)) {
> +	/* To be valid for IRQ the line needs to be input or open drain */
> +	if (test_bit(FLAG_IS_OUT, &desc->flags) &&
> +	    !test_bit(FLAG_OPEN_DRAIN, &desc->flags)) {
>  		chip_err(gc,
>  			 "%s: tried to flag a GPIO set as output for IRQ\n",
>  			 __func__);
> -- 
> 2.25.4
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH] gpio: fix locking open drain IRQ lines

[Hans Verkuil]

On 27/05/2020 16:07, Linus Walleij wrote:
> We provided the right semantics on open drain lines being
> by definition output but incidentally the irq set up function
> would only allow IRQs on lines that were "not output".
> 
> Fix the semantics to allow output open drain lines to be used
> for IRQs.
> 
> Reported-by: Hans Verkuil <hverkuil@xs4all.nl>

Tested-by: Hans Verkuil <hverkuil@xs4all.nl>

Whether this is the right/best fix or not, I cannot tell, but it certainly
fixes the cec-gpio driver!

Regards,

	Hans

> Cc: Russell King <linux@armlinux.org.uk>
> Cc: stable@vger.kernel.org
> Fixes: 256efaea1fdc ("gpiolib: fix up emulated open drain outputs")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/gpiolib.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index b4b5792fe2ff..edd74ff31cea 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -4220,7 +4220,9 @@ int gpiochip_lock_as_irq(struct gpio_chip *gc, unsigned int offset)
>  		}
>  	}
>  
> -	if (test_bit(FLAG_IS_OUT, &desc->flags)) {
> +	/* To be valid for IRQ the line needs to be input or open drain */
> +	if (test_bit(FLAG_IS_OUT, &desc->flags) &&
> +	    !test_bit(FLAG_OPEN_DRAIN, &desc->flags)) {
>  		chip_err(gc,
>  			 "%s: tried to flag a GPIO set as output for IRQ\n",
>  			 __func__);
> 

Re: [PATCH] gpio: fix locking open drain IRQ lines

[Linus Walleij]

On Wed, May 27, 2020 at 4:18 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
> On Wed, May 27, 2020 at 04:07:58PM +0200, Linus Walleij wrote:

> > We provided the right semantics on open drain lines being
> > by definition output but incidentally the irq set up function
> > would only allow IRQs on lines that were "not output".
> >
> > Fix the semantics to allow output open drain lines to be used
> > for IRQs.
> >
> > Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: stable@vger.kernel.org
> > Fixes: 256efaea1fdc ("gpiolib: fix up emulated open drain outputs")
>
> As I've pointed out in the reporting thread, I don't think it can be
> justified as a regression - it's a bug in its own right that has been
> discovered by unifying the gpiolib semantics, since the cec-gpio code
> will fail on hardware that can provide real open-drain outputs
> irrespective of that commit.
>
> So, you're really fixing a deeper problem that was never discovered
> until gpiolib's semantics were fixed to be more uniform.

You're right, I was thinking of Fixes: as more of a mechanical
instruction to the stable kernel maintainers administrative machinery.

I will use the other way to signal to stable where to apply this.

Yours,
Linus Walleij

Re: [PATCH] gpio: fix locking open drain IRQ lines

[Russell King - ARM Linux admin]

On Thu, May 28, 2020 at 03:46:04PM +0200, Linus Walleij wrote:
> On Wed, May 27, 2020 at 4:18 PM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> > On Wed, May 27, 2020 at 04:07:58PM +0200, Linus Walleij wrote:
> 
> > > We provided the right semantics on open drain lines being
> > > by definition output but incidentally the irq set up function
> > > would only allow IRQs on lines that were "not output".
> > >
> > > Fix the semantics to allow output open drain lines to be used
> > > for IRQs.
> > >
> > > Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
> > > Cc: Russell King <linux@armlinux.org.uk>
> > > Cc: stable@vger.kernel.org
> > > Fixes: 256efaea1fdc ("gpiolib: fix up emulated open drain outputs")
> >
> > As I've pointed out in the reporting thread, I don't think it can be
> > justified as a regression - it's a bug in its own right that has been
> > discovered by unifying the gpiolib semantics, since the cec-gpio code
> > will fail on hardware that can provide real open-drain outputs
> > irrespective of that commit.
> >
> > So, you're really fixing a deeper problem that was never discovered
> > until gpiolib's semantics were fixed to be more uniform.
> 
> You're right, I was thinking of Fixes: as more of a mechanical
> instruction to the stable kernel maintainers administrative machinery.
> 
> I will use the other way to signal to stable where to apply this.

I think it makes sense to apply this patch to stable kernels prior to
the commit mentioned in the Fixes tag - but how far back is a good
question.  Certainly to the point that we ended up with code relying
on this behaviour (so when cec-gpio was introduced?)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH] gpio: fix locking open drain IRQ lines

[Hans Verkuil]

On 28/05/2020 15:58, Russell King - ARM Linux admin wrote:
> On Thu, May 28, 2020 at 03:46:04PM +0200, Linus Walleij wrote:
>> On Wed, May 27, 2020 at 4:18 PM Russell King - ARM Linux admin
>> <linux@armlinux.org.uk> wrote:
>>> On Wed, May 27, 2020 at 04:07:58PM +0200, Linus Walleij wrote:
>>
>>>> We provided the right semantics on open drain lines being
>>>> by definition output but incidentally the irq set up function
>>>> would only allow IRQs on lines that were "not output".
>>>>
>>>> Fix the semantics to allow output open drain lines to be used
>>>> for IRQs.
>>>>
>>>> Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
>>>> Cc: Russell King <linux@armlinux.org.uk>
>>>> Cc: stable@vger.kernel.org
>>>> Fixes: 256efaea1fdc ("gpiolib: fix up emulated open drain outputs")
>>>
>>> As I've pointed out in the reporting thread, I don't think it can be
>>> justified as a regression - it's a bug in its own right that has been
>>> discovered by unifying the gpiolib semantics, since the cec-gpio code
>>> will fail on hardware that can provide real open-drain outputs
>>> irrespective of that commit.
>>>
>>> So, you're really fixing a deeper problem that was never discovered
>>> until gpiolib's semantics were fixed to be more uniform.
>>
>> You're right, I was thinking of Fixes: as more of a mechanical
>> instruction to the stable kernel maintainers administrative machinery.
>>
>> I will use the other way to signal to stable where to apply this.
> 
> I think it makes sense to apply this patch to stable kernels prior to
> the commit mentioned in the Fixes tag - but how far back is a good
> question.  Certainly to the point that we ended up with code relying
> on this behaviour (so when cec-gpio was introduced?)
> 

For the record, cec-gpio started to rely on this behavior in kernel 5.3.
There is no need to go back to pre-5.3 kernels.

Regards,

	Hans

Re: [PATCH] gpio: fix locking open drain IRQ lines

[Hans Verkuil]

On 28/05/2020 14:24, Hans Verkuil wrote:
> On 27/05/2020 16:07, Linus Walleij wrote:
>> We provided the right semantics on open drain lines being
>> by definition output but incidentally the irq set up function
>> would only allow IRQs on lines that were "not output".
>>
>> Fix the semantics to allow output open drain lines to be used
>> for IRQs.
>>
>> Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
> 
> Tested-by: Hans Verkuil <hverkuil@xs4all.nl>
> 
> Whether this is the right/best fix or not, I cannot tell, but it certainly
> fixes the cec-gpio driver!

Close, but no cigar. I didn't check the kernel log and I now see this warning:

[    4.157000] ------------[ cut here ]------------
[    4.161699] WARNING: CPU: 0 PID: 1 at drivers/gpio/gpiolib.c:4260 gpiochip_enable_irq+0x6c/0x78
[    4.170522] Modules linked in:
[    4.173626] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.7.0-rc2-rpi3 #179
[    4.180510] Hardware name: Raspberry Pi 3 Model B (DT)
[    4.185724] pstate: 80000085 (Nzcv daIf -PAN -UAO)
[    4.190585] pc : gpiochip_enable_irq+0x6c/0x78
[    4.195094] lr : gpiochip_irq_enable+0x20/0x58
[    4.199597] sp : ffff8000100239e0
[    4.202956] x29: ffff8000100239e0 x28: 0000000000000000
[    4.208346] x27: ffff0000042b9808 x26: ffff00000425eca4
[    4.213735] x25: 0000000000000000 x24: ffff00000425ed58
[    4.219124] x23: 0000000000000000 x22: 0000000000000000
[    4.224512] x21: 0000000000000001 x20: ffff0000041d1b00
[    4.229902] x19: ffff00000425ec00 x18: 0000000000000000
[    4.235291] x17: ffff8000107ddce0 x16: ffff8000107dd1b8
[    4.240680] x15: ffff000037870450 x14: ffffffffffffffff
[    4.246070] x13: ffff0000041d1a87 x12: ffff0000041d1a85
[    4.251459] x11: 0000000000000000 x10: 0000000000000040
[    4.256848] x9 : 0000000000000000 x8 : 0000000000000080
[    4.262237] x7 : 0000000000000000 x6 : 000000000000003f
[    4.267626] x5 : 0000000000000000 x4 : ffff0000372d3990
[    4.273014] x3 : ffff00000425ec28 x2 : 0000000000000036
[    4.278403] x1 : ffff0000372d40e0 x0 : 0000000000000683
[    4.283793] Call trace:
[    4.286274]  gpiochip_enable_irq+0x6c/0x78
[    4.290432]  irq_enable+0x3c/0x80
[    4.293795]  __irq_startup+0x7c/0xa8
[    4.297419]  irq_startup+0x5c/0x138
[    4.300957]  __setup_irq+0x82c/0x8f8
[    4.304583]  request_threaded_irq+0xd8/0x198
[    4.308913]  devm_request_threaded_irq+0x78/0xf8
[    4.313599]  cec_gpio_probe+0x118/0x288
[    4.317490]  platform_drv_probe+0x54/0xb0
[    4.321558]  really_probe+0xd8/0x330
[    4.325184]  driver_probe_device+0x58/0xf8
[    4.329339]  device_driver_attach+0x74/0x80
[    4.333582]  __driver_attach+0x58/0xe0
[    4.337383]  bus_for_each_dev+0x70/0xc0
[    4.341273]  driver_attach+0x24/0x30
[    4.344899]  bus_add_driver+0x150/0x1e0
[    4.348789]  driver_register+0x64/0x128
[    4.352678]  __platform_driver_register+0x48/0x58
[    4.357452]  cec_gpio_pdrv_init+0x1c/0x28
[    4.361519]  do_one_initcall+0x54/0x1b8
[    4.365412]  kernel_init_freeable+0x1cc/0x244
[    4.369833]  kernel_init+0x14/0x108
[    4.373371]  ret_from_fork+0x10/0x1c
[    4.377000] ---[ end trace 49bf91ba04222a1a ]---
[    4.382253] Registered IR keymap rc-cec
[    4.386344] rc rc0: cec-gpio@7 as /devices/platform/cec-gpio@7/rc/rc0
[    4.393135] input: cec-gpio@7 as /devices/platform/cec-gpio@7/rc/rc0/input0

The fix seems to be to just add this change to the patch:

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 40f2d7f69be2..5a8214d5e927 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4255,7 +4257,8 @@ void gpiochip_enable_irq(struct gpio_chip *gc, unsigned int offset)

 	if (!IS_ERR(desc) &&
 	    !WARN_ON(!test_bit(FLAG_USED_AS_IRQ, &desc->flags))) {
-		WARN_ON(test_bit(FLAG_IS_OUT, &desc->flags));
+		WARN_ON(test_bit(FLAG_IS_OUT, &desc->flags) &&
+			!test_bit(FLAG_OPEN_DRAIN, &desc->flags));
 		set_bit(FLAG_IRQ_IS_ENABLED, &desc->flags);
 	}
 }

I wish I could test this on the BeagleBone Black, which is the other use-case,
but it's in another country and the earliest I can test is probably August or
September (depending on how long the travel restrictions stay in place).

If someone else has a BBB handy, then it would be nice if this patch can be
tested.

Regards,

	Hans


> 
> Regards,
> 
> 	Hans
> 
>> Cc: Russell King <linux@armlinux.org.uk>
>> Cc: stable@vger.kernel.org
>> Fixes: 256efaea1fdc ("gpiolib: fix up emulated open drain outputs")
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>>  drivers/gpio/gpiolib.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index b4b5792fe2ff..edd74ff31cea 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -4220,7 +4220,9 @@ int gpiochip_lock_as_irq(struct gpio_chip *gc, unsigned int offset)
>>  		}
>>  	}
>>  
>> -	if (test_bit(FLAG_IS_OUT, &desc->flags)) {
>> +	/* To be valid for IRQ the line needs to be input or open drain */
>> +	if (test_bit(FLAG_IS_OUT, &desc->flags) &&
>> +	    !test_bit(FLAG_OPEN_DRAIN, &desc->flags)) {
>>  		chip_err(gc,
>>  			 "%s: tried to flag a GPIO set as output for IRQ\n",
>>  			 __func__);
>>
> 

Re: [PATCH] gpio: fix locking open drain IRQ lines

[Linus Walleij]

On Fri, May 29, 2020 at 1:06 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:

> Close, but no cigar. I didn't check the kernel log and I now see this warning:

Oups.

> The fix seems to be to just add this change to the patch:
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Thanks a lot Hans, I folded your fix into my fix.

Yours,
Linus Walleij