* [PATCH] gpio: fix locking open drain IRQ lines @ 2020-05-27 14:07 Linus Walleij 2020-05-27 14:18 ` Russell King - ARM Linux admin 2020-05-28 12:24 ` Hans Verkuil 0 siblings, 2 replies; 8+ messages in thread From: Linus Walleij @ 2020-05-27 14:07 UTC (permalink / raw) To: linux-gpio Cc: Bartosz Golaszewski, Linus Walleij, Hans Verkuil, Russell King, stable 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] gpio: fix locking open drain IRQ lines 2020-05-27 14:07 [PATCH] gpio: fix locking open drain IRQ lines Linus Walleij @ 2020-05-27 14:18 ` Russell King - ARM Linux admin 2020-05-28 13:46 ` Linus Walleij 2020-05-28 12:24 ` Hans Verkuil 1 sibling, 1 reply; 8+ messages in thread From: Russell King - ARM Linux admin @ 2020-05-27 14:18 UTC (permalink / raw) To: Linus Walleij; +Cc: linux-gpio, Bartosz Golaszewski, Hans Verkuil, stable 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] gpio: fix locking open drain IRQ lines 2020-05-27 14:18 ` Russell King - ARM Linux admin @ 2020-05-28 13:46 ` Linus Walleij 2020-05-28 13:58 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 8+ messages in thread From: Linus Walleij @ 2020-05-28 13:46 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Hans Verkuil, stable 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] gpio: fix locking open drain IRQ lines 2020-05-28 13:46 ` Linus Walleij @ 2020-05-28 13:58 ` Russell King - ARM Linux admin 2020-05-28 14:20 ` Hans Verkuil 0 siblings, 1 reply; 8+ messages in thread From: Russell King - ARM Linux admin @ 2020-05-28 13:58 UTC (permalink / raw) To: Linus Walleij Cc: open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Hans Verkuil, stable 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] gpio: fix locking open drain IRQ lines 2020-05-28 13:58 ` Russell King - ARM Linux admin @ 2020-05-28 14:20 ` Hans Verkuil 0 siblings, 0 replies; 8+ messages in thread From: Hans Verkuil @ 2020-05-28 14:20 UTC (permalink / raw) To: Russell King - ARM Linux admin, Linus Walleij Cc: open list:GPIO SUBSYSTEM, Bartosz Golaszewski, stable 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] gpio: fix locking open drain IRQ lines 2020-05-27 14:07 [PATCH] gpio: fix locking open drain IRQ lines Linus Walleij 2020-05-27 14:18 ` Russell King - ARM Linux admin @ 2020-05-28 12:24 ` Hans Verkuil 2020-05-29 11:06 ` Hans Verkuil 1 sibling, 1 reply; 8+ messages in thread From: Hans Verkuil @ 2020-05-28 12:24 UTC (permalink / raw) To: Linus Walleij, linux-gpio; +Cc: Bartosz Golaszewski, Russell King, stable 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__); > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] gpio: fix locking open drain IRQ lines 2020-05-28 12:24 ` Hans Verkuil @ 2020-05-29 11:06 ` Hans Verkuil 2020-05-29 12:03 ` Linus Walleij 0 siblings, 1 reply; 8+ messages in thread From: Hans Verkuil @ 2020-05-29 11:06 UTC (permalink / raw) To: Linus Walleij, linux-gpio; +Cc: Bartosz Golaszewski, Russell King, stable 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__); >> > ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] gpio: fix locking open drain IRQ lines 2020-05-29 11:06 ` Hans Verkuil @ 2020-05-29 12:03 ` Linus Walleij 0 siblings, 0 replies; 8+ messages in thread From: Linus Walleij @ 2020-05-29 12:03 UTC (permalink / raw) To: Hans Verkuil Cc: open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Russell King, stable 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-05-29 12:03 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-27 14:07 [PATCH] gpio: fix locking open drain IRQ lines Linus Walleij 2020-05-27 14:18 ` Russell King - ARM Linux admin 2020-05-28 13:46 ` Linus Walleij 2020-05-28 13:58 ` Russell King - ARM Linux admin 2020-05-28 14:20 ` Hans Verkuil 2020-05-28 12:24 ` Hans Verkuil 2020-05-29 11:06 ` Hans Verkuil 2020-05-29 12:03 ` Linus Walleij
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.