| Message ID | 1466014954-17956-1-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wed, Jun 15, 2016 at 08:22:34PM +0200, Hans de Goede wrote: > This reverts commit 54d77198fdfb("gpio: bail out silently on NULL > descriptors"). > > This commit causes the following code to fail: > > gpio_desc = devm_gpiod_get_optional(dev, ...); > gpio_irq = gpiod_to_irq(gpio_desc); > if (gpio_irq >= 0) { > ret = devm_request_irq(dev, gpio_irq, ...); > > And now ret is an error causing the probe function in question to bail. > > The problem here is that gpiod_to_irq now returns 0 for a NULL > gpio_desc while 0 is a valid irq-nr. Also see: > commit 4c37ce8608a8("gpio: make gpiod_to_irq() return negative for NO_IRQ") > which specifically avoids returning 0. 0 is not a valid interrupt number. irq_find_mapping returns 0 in case of an error: http://lxr.free-electrons.com/source/kernel/irq/irqdomain.c#L657 and see that mail from Linus: http://yarchive.net/comp/linux/zero.html " (On a PC, hardware irq 0 is a real irq too, but it's a _special_ irq, and it is set up by architecture-specific code. So as far as the generic kernel and all devices are concerned, "!dev->irq" means that the irq doesn't exist or hasn't been mapped for that device yet). " The proper way to solve this is to fix the driver, like Quentin did in that patch: http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/435881.html Maxime
Hi, On 15-06-16 20:46, Maxime Ripard wrote: > Hi, > > On Wed, Jun 15, 2016 at 08:22:34PM +0200, Hans de Goede wrote: >> This reverts commit 54d77198fdfb("gpio: bail out silently on NULL >> descriptors"). >> >> This commit causes the following code to fail: >> >> gpio_desc = devm_gpiod_get_optional(dev, ...); >> gpio_irq = gpiod_to_irq(gpio_desc); >> if (gpio_irq >= 0) { >> ret = devm_request_irq(dev, gpio_irq, ...); >> >> And now ret is an error causing the probe function in question to bail. >> >> The problem here is that gpiod_to_irq now returns 0 for a NULL >> gpio_desc while 0 is a valid irq-nr. Also see: >> commit 4c37ce8608a8("gpio: make gpiod_to_irq() return negative for NO_IRQ") >> which specifically avoids returning 0. > > 0 is not a valid interrupt number. Ok, so lets decouple the discussion a bit from whether or not 0 is a valid interrupt number. > irq_find_mapping returns 0 in case of an error: > http://lxr.free-electrons.com/source/kernel/irq/irqdomain.c#L657 Yes and in that case gpiod_to_irq() will explicitly return -ENXIO so as to not confuse callers. Which is the right thing to do, since almost all kernel functions have the semantic ret < 0 means error >= 0 means success. The patch I'm suggestion to revert however now has gpiod_to_irq() return 0 when it gets passed a NULL gpio_desc pointer, so this really has nothing to do with irq_find_mapping() at all (that never gets called in this case) and has everything to do with the patch I suggest we revert changing the behavior for gpiod_to_irq(NULL). Also not that that patch has a Cc: stable, so fix the driver is really not a good answer, stable patches should not change (internal) api behavior and break other code. Regards, Hans
On Wed, Jun 15, 2016 at 8:22 PM, Hans de Goede <hdegoede@redhat.com> wrote: > This reverts commit 54d77198fdfb("gpio: bail out silently on NULL > descriptors"). > > This commit causes the following code to fail: > > gpio_desc = devm_gpiod_get_optional(dev, ...); > gpio_irq = gpiod_to_irq(gpio_desc); > if (gpio_irq >= 0) { > ret = devm_request_irq(dev, gpio_irq, ...); > > And now ret is an error causing the probe function in question to bail. > > The problem here is that gpiod_to_irq now returns 0 for a NULL > gpio_desc while 0 is a valid irq-nr. Also see: > commit 4c37ce8608a8("gpio: make gpiod_to_irq() return negative for NO_IRQ") > which specifically avoids returning 0. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> The first commit was done to fix another regression so if I revert it I get back the first regression. I guess what we need to do is simply make gpiod_to_irq() a special case and have it behave the way expected for now? Hans: I sent a patch like that if it works for you could you give me your Tested-by? Yours, Linus Walleij
On 06/15/2016 10:08 PM, Hans de Goede wrote: > Hi, > > On 15-06-16 20:46, Maxime Ripard wrote: >> Hi, >> >> On Wed, Jun 15, 2016 at 08:22:34PM +0200, Hans de Goede wrote: >>> This reverts commit 54d77198fdfb("gpio: bail out silently on NULL >>> descriptors"). >>> >>> This commit causes the following code to fail: >>> >>> gpio_desc = devm_gpiod_get_optional(dev, ...); May be I missed smth., but in this example gpio_desc may contain err code. >>> gpio_irq = gpiod_to_irq(gpio_desc); which, most probably will cause gpiod_to_irq() to crash if (!desc) \ return 0; \ if (!desc->gdev) { \ ^^^^^^^^^^^^^ here >>> if (gpio_irq >= 0) { >>> ret = devm_request_irq(dev, gpio_irq, ...); >>> >>> And now ret is an error causing the probe function in question to bail. >>> >>> The problem here is that gpiod_to_irq now returns 0 for a NULL >>> gpio_desc while 0 is a valid irq-nr. Also see: >>> commit 4c37ce8608a8("gpio: make gpiod_to_irq() return negative for >>> NO_IRQ") >>> which specifically avoids returning 0. >> >> 0 is not a valid interrupt number. > > Ok, so lets decouple the discussion a bit from whether or not 0 > is a valid interrupt number. > >> irq_find_mapping returns 0 in case of an error: >> http://lxr.free-electrons.com/source/kernel/irq/irqdomain.c#L657 > > Yes and in that case gpiod_to_irq() will explicitly return -ENXIO > so as to not confuse callers. > > Which is the right thing to do, since almost all kernel functions > have the semantic ret < 0 means error >= 0 means success. > > The patch I'm suggestion to revert however now has gpiod_to_irq() > return 0 when it gets passed a NULL gpio_desc pointer, so this > really has nothing to do with irq_find_mapping() at all (that never > gets called in this case) and has everything to do with the > patch I suggest we revert changing the behavior for > gpiod_to_irq(NULL). > > Also not that that patch has a Cc: stable, so fix the driver is > really not a good answer, stable patches should not change > (internal) api behavior and break other code. >
On Thu, Jun 16, 2016 at 11:23 AM, Grygorii Strashko <grygorii.strashko@ti.com> wrote: > On 06/15/2016 10:08 PM, Hans de Goede wrote: >>>> This commit causes the following code to fail: >>>> >>>> gpio_desc = devm_gpiod_get_optional(dev, ...); > > May be I missed smth., but in this example gpio_desc may contain err code. > >>>> gpio_irq = gpiod_to_irq(gpio_desc); > which, most probably will cause gpiod_to_irq() to crash > if (!desc) \ > return 0; \ > if (!desc->gdev) { \ > ^^^^^^^^^^^^^ here Hm good catch. These optional GPIOs may never have worked properly, good that they are so rare :/ I'll send a separate patch making some IS_ERR() checks. Yours, Linus Walleij
Hi, On 16-06-16 11:23, Grygorii Strashko wrote: > On 06/15/2016 10:08 PM, Hans de Goede wrote: >> Hi, >> >> On 15-06-16 20:46, Maxime Ripard wrote: >>> Hi, >>> >>> On Wed, Jun 15, 2016 at 08:22:34PM +0200, Hans de Goede wrote: >>>> This reverts commit 54d77198fdfb("gpio: bail out silently on NULL >>>> descriptors"). >>>> >>>> This commit causes the following code to fail: >>>> >>>> gpio_desc = devm_gpiod_get_optional(dev, ...); > > May be I missed smth., but in this example gpio_desc may contain err code. Right, and the actual driver code does contain an error check for this, this was just simplified code to explain the problem. Also note the ... which would not really compile. Regards, Hans > >>>> gpio_irq = gpiod_to_irq(gpio_desc); > which, most probably will cause gpiod_to_irq() to crash > if (!desc) \ > return 0; \ > if (!desc->gdev) { \ > ^^^^^^^^^^^^^ here > >>>> if (gpio_irq >= 0) { >>>> ret = devm_request_irq(dev, gpio_irq, ...); >>>> >>>> And now ret is an error causing the probe function in question to bail. >>>> >>>> The problem here is that gpiod_to_irq now returns 0 for a NULL >>>> gpio_desc while 0 is a valid irq-nr. Also see: >>>> commit 4c37ce8608a8("gpio: make gpiod_to_irq() return negative for >>>> NO_IRQ") >>>> which specifically avoids returning 0. >>> >>> 0 is not a valid interrupt number. >> >> Ok, so lets decouple the discussion a bit from whether or not 0 >> is a valid interrupt number. >> >>> irq_find_mapping returns 0 in case of an error: >>> http://lxr.free-electrons.com/source/kernel/irq/irqdomain.c#L657 >> >> Yes and in that case gpiod_to_irq() will explicitly return -ENXIO >> so as to not confuse callers. >> >> Which is the right thing to do, since almost all kernel functions >> have the semantic ret < 0 means error >= 0 means success. >> >> The patch I'm suggestion to revert however now has gpiod_to_irq() >> return 0 when it gets passed a NULL gpio_desc pointer, so this >> really has nothing to do with irq_find_mapping() at all (that never >> gets called in this case) and has everything to do with the >> patch I suggest we revert changing the behavior for >> gpiod_to_irq(NULL). >> >> Also not that that patch has a Cc: stable, so fix the driver is >> really not a good answer, stable patches should not change >> (internal) api behavior and break other code. >> > >
Hi, On 16-06-16 11:53, Linus Walleij wrote: > On Thu, Jun 16, 2016 at 11:23 AM, Grygorii Strashko > <grygorii.strashko@ti.com> wrote: >> On 06/15/2016 10:08 PM, Hans de Goede wrote: > >>>>> This commit causes the following code to fail: >>>>> >>>>> gpio_desc = devm_gpiod_get_optional(dev, ...); >> >> May be I missed smth., but in this example gpio_desc may contain err code. >> >>>>> gpio_irq = gpiod_to_irq(gpio_desc); >> which, most probably will cause gpiod_to_irq() to crash >> if (!desc) \ >> return 0; \ >> if (!desc->gdev) { \ >> ^^^^^^^^^^^^^ here > > Hm good catch. These optional GPIOs may never have worked properly, > good that they are so rare :/ > > I'll send a separate patch making some IS_ERR() checks. IMHO that is really something which the driver should be doing (and in the case of phy-sun4i-usb.c where I was hitting the NULL problem, the driver does contain these checks), but I guess it cannot hurt. Regards, Hans
Hi, On 15-06-16 23:04, Linus Walleij wrote: > On Wed, Jun 15, 2016 at 8:22 PM, Hans de Goede <hdegoede@redhat.com> wrote: > >> This reverts commit 54d77198fdfb("gpio: bail out silently on NULL >> descriptors"). >> >> This commit causes the following code to fail: >> >> gpio_desc = devm_gpiod_get_optional(dev, ...); >> gpio_irq = gpiod_to_irq(gpio_desc); >> if (gpio_irq >= 0) { >> ret = devm_request_irq(dev, gpio_irq, ...); >> >> And now ret is an error causing the probe function in question to bail. >> >> The problem here is that gpiod_to_irq now returns 0 for a NULL >> gpio_desc while 0 is a valid irq-nr. Also see: >> commit 4c37ce8608a8("gpio: make gpiod_to_irq() return negative for NO_IRQ") >> which specifically avoids returning 0. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > The first commit was done to fix another regression so if I revert it I get > back the first regression. Ok. > I guess what we need to do is simply make gpiod_to_irq() a > special case and have it behave the way expected for now? That is fine with me. > Hans: I sent a patch like that if it works for you could you give > me your Tested-by? I've just checked the code and code-wise it looks good. I'll test it on actual hw later today. Regards, Hans
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 24f60d2..6c33a07 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1367,13 +1367,10 @@ done: /* * This descriptor validation needs to be inserted verbatim into each * function taking a descriptor, so we need to use a preprocessor - * macro to avoid endless duplication. If the desc is NULL it is an - * optional GPIO and calls should just bail out. + * macro to avoid endless duplication. */ #define VALIDATE_DESC(desc) do { \ - if (!desc) \ - return 0; \ - if (!desc->gdev) { \ + if (!desc || !desc->gdev) { \ pr_warn("%s: invalid GPIO\n", __func__); \ return -EINVAL; \ } \ @@ -1384,9 +1381,7 @@ done: } } while (0) #define VALIDATE_DESC_VOID(desc) do { \ - if (!desc) \ - return; \ - if (!desc->gdev) { \ + if (!desc || !desc->gdev) { \ pr_warn("%s: invalid GPIO\n", __func__); \ return; \ } \
This reverts commit 54d77198fdfb("gpio: bail out silently on NULL descriptors"). This commit causes the following code to fail: gpio_desc = devm_gpiod_get_optional(dev, ...); gpio_irq = gpiod_to_irq(gpio_desc); if (gpio_irq >= 0) { ret = devm_request_irq(dev, gpio_irq, ...); And now ret is an error causing the probe function in question to bail. The problem here is that gpiod_to_irq now returns 0 for a NULL gpio_desc while 0 is a valid irq-nr. Also see: commit 4c37ce8608a8("gpio: make gpiod_to_irq() return negative for NO_IRQ") which specifically avoids returning 0. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/gpio/gpiolib.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)