| Message ID | 20200527140758.162280-1-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: fix locking open drain IRQ lines | expand |
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 > >
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__); >
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
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?)
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
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__); >> >
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
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__);
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(-)