[PATCH] Revert "gpio: bail out silently on NULL descriptors"

[PATCH] Revert "gpio: bail out silently on NULL descriptors"

[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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpio/gpiolib.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

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; \
 	} \
-- 
2.7.4

[PATCH] Revert "gpio: bail out silently on NULL descriptors"

[Maxime Ripard]

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

-- 
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>

[PATCH] Revert "gpio: bail out silently on NULL descriptors"

[Hans de Goede]

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

[PATCH] Revert "gpio: bail out silently on NULL descriptors"

[Linus Walleij]

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

[PATCH] Revert "gpio: bail out silently on NULL descriptors"

[Grygorii Strashko]

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.
> 


-- 
regards,
-grygorii

[PATCH] Revert "gpio: bail out silently on NULL descriptors"

[Linus Walleij]

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

[PATCH] Revert "gpio: bail out silently on NULL descriptors"

[Hans de Goede]

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.
>>
>
>

[PATCH] Revert "gpio: bail out silently on NULL descriptors"

[Hans de Goede]

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

[PATCH] Revert "gpio: bail out silently on NULL descriptors"

[Hans de Goede]

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