Revert "gpio: bail out silently on NULL descriptors"

PATCH New Hans de Goede 4 participants 9 messages Jun 15, 2016 – Jun 17, 2016 GPIO

lore

Share & tools

lore

Hans de Goede

author

2016-06-15 18:22 UTC #0 lore raw

PATCH 1/1 Revert "gpio: bail out silently on NULL descriptors"

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 <[email protected]>

---
 drivers/gpio/gpiolib.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

drivers/gpio/gpiolib.c +3 -8

index 24f60d2..6c33a07 100644
@@ -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; \
 	} \

Signature · 1 line

2.7.4

Maxime Ripard

2016-06-15 18:46 UTC #1 lore raw

PATCH 1/1 Revert "gpio: bail out silently on NULL descriptors"

Hi,

Quoted 10 lines · Hans de Goede

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, ...);

Quoted 6 lines

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

Signature · 10 lines

Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160615/f23634f1/attachment.sig>

Hans de Goede

author

2016-06-15 19:08 UTC #2 lore raw

PATCH 1/1 Revert "gpio: bail out silently on NULL descriptors"

Hi,

Quoted 13 lines · Maxime Ripard

Hi,
Quoted 10 lines · Hans de Goede
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, ...);

Quoted 8 lines

Quoted 6 lines
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.

Quoted 2 lines

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

Grygorii Strashko

2016-06-16 09:23 UTC #3 lore raw

PATCH 1/1 Revert "gpio: bail out silently on NULL descriptors"

Quoted 12 lines · Hans de Goede

Hi,
Quoted 9 lines · Maxime Ripard
Hi,
Quoted 6 lines · Hans de Goede
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.

Quoted 1 line

Quoted 1 line
Quoted 1 line
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

Quoted 36 lines

Quoted 5 lines
Quoted 5 lines
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.
Quoted 7 lines
Quoted 5 lines
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.
Quoted 2 lines
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.

Signature · 2 lines

regards,
-grygorii

Linus Walleij

2016-06-16 09:53 UTC #4 lore raw

PATCH 1/1 Revert "gpio: bail out silently on NULL descriptors"

Quoted 6 lines · Grygorii Strashko

Quoted 3 lines · Hans de Goede
Quoted 3 lines
Quoted 3 lines
This commit causes the following code to fail:
gpio_desc = devm_gpiod_get_optional(dev, ...);

Quoted 8 lines

May be I missed smth., but in this example gpio_desc may contain err code.
Quoted 1 line
Quoted 1 line
Quoted 1 line
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

Hans de Goede

author

2016-06-17 08:05 UTC #5 lore raw

PATCH 1/1 Revert "gpio: bail out silently on NULL descriptors"

Hi,

Quoted 17 lines · Linus Walleij

Quoted 6 lines · Grygorii Strashko
Quoted 3 lines · Hans de Goede
Quoted 3 lines
Quoted 3 lines
This commit causes the following code to fail:
gpio_desc = devm_gpiod_get_optional(dev, ...);
Quoted 8 lines
May be I missed smth., but in this example gpio_desc may contain err code.
Quoted 1 line
Quoted 1 line
Quoted 1 line
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

Quoted 4 lines

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

Hans de Goede

author

2016-06-17 08:04 UTC #6 lore raw

PATCH 1/1 Revert "gpio: bail out silently on NULL descriptors"

In reply to Grygorii Strashko

Hi,

Quoted 7 lines · Grygorii Strashko

Quoted 6 lines · Hans de Goede
Hi,
Quoted 3 lines · Maxime Ripard
Hi,
On Wed, Jun 15, 2016 at 08:22:34PM +0200, Hans de Goede wrote:

Quoted 8 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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

Quoted 46 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
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
Quoted 36 lines
Quoted 5 lines
Quoted 5 lines
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.
Quoted 7 lines
Quoted 5 lines
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.
Quoted 2 lines
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.

Linus Walleij

2016-06-15 21:04 UTC #7 lore raw

PATCH 1/1 Revert "gpio: bail out silently on NULL descriptors"

In reply to Hans de Goede

Quoted 10 lines · Hans de Goede

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, ...);

Quoted 8 lines

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 <[email protected]>

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

Hans de Goede

author

2016-06-17 08:07 UTC #8 lore raw

PATCH 1/1 Revert "gpio: bail out silently on NULL descriptors"

Hi,

Quoted 19 lines · Linus Walleij

Quoted 17 lines · Hans de Goede
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.

Quoted 4 lines

Quoted 1 line
Signed-off-by Hans de Goede <[email protected]>
The first commit was done to fix another regression so if I revert it I get back the first regression.

Ok.

Quoted 2 lines

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.

Quoted 2 lines

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