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__);
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)) {-- 2.25.4
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 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)) {
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)) {
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
linux-stable-mirror@lists.linaro.org
-
Hans Verkuil -
Linus Walleij -
Russell King - ARM Linux admin