[PATCH] pinctrl: sh-pfc: Let gpio_chip.to_irq() return -ENXIO on error

[PATCH] pinctrl: sh-pfc: Let gpio_chip.to_irq() return -ENXIO on error

[Geert Uytterhoeven]

Currrently the gpio_chip.to_irq() callback returns -ENOSYS on error,
which causes bad interactions with the serial_mctrl_gpio helpers.

mctrl_gpio_init() returns -ENOSYS if GPIOLIB is not enabled, which is
intended to be ignored by its callers. However, ignoring -ENOSYS when it
was caused by a gpiod_to_irq() failure will lead to a crash later:

    Unable to handle kernel paging request at virtual address ffffffde
    ...
    PC is at mctrl_gpio_set+0x14/0x78

Fix this by returning -ENXIO instead.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Is -ENXIO the right error code?

  - Most drivers seem to return -ENXIO on failure, just like
    gpiod_to_irq() does when no .to_irq() callback is provided by the
    driver,
  - Some drivers use -EINVAL,
  - Drivers that call irq_find_mapping(), irq_create_mapping(), or
    irq_create_fwspec_mapping() return zero!  This also applies to the
    core helper gpiochip_to_irq().
---
 drivers/pinctrl/sh-pfc/gpio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
index a6681b8b17c3b30c..2fffb9c32231adb3 100644
--- a/drivers/pinctrl/sh-pfc/gpio.c
+++ b/drivers/pinctrl/sh-pfc/gpio.c
@@ -212,7 +212,7 @@ static int gpio_pin_to_irq(struct gpio_chip *gc, unsigned offset)
 		}
 	}
 
-	return -ENOSYS;
+	return -ENXIO;
 
 found:
 	return pfc->irqs[i];
-- 
1.9.1

Re: [PATCH] pinctrl: sh-pfc: Let gpio_chip.to_irq() return -ENXIO on error

[Linus Walleij]

On Fri, Apr 29, 2016 at 9:24 AM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> Currrently the gpio_chip.to_irq() callback returns -ENOSYS on error,
> which causes bad interactions with the serial_mctrl_gpio helpers.
>
> mctrl_gpio_init() returns -ENOSYS if GPIOLIB is not enabled, which is
> intended to be ignored by its callers. However, ignoring -ENOSYS when it
> was caused by a gpiod_to_irq() failure will lead to a crash later:
>
>     Unable to handle kernel paging request at virtual address ffffffde
>     ...
>     PC is at mctrl_gpio_set+0x14/0x78
>
> Fix this by returning -ENXIO instead.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Is -ENXIO the right error code?

I would say that whatever the generic helpers in drivers/gpio/gpiolib.c
returns should be the norm. However the guy who wrote them (me)
isn't very smart either. It looks like so:

static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
{
        return irq_find_mapping(chip->irqdomain, offset);
}

That returns 0 (NO_IRQ) on failure.

And as you say:

>   - Drivers that call irq_find_mapping(), irq_create_mapping(), or
>     irq_create_fwspec_mapping() return zero!  This also applies to the
>     core helper gpiochip_to_irq().

Zero means NO_IRQ.

Reminder:
http://lwn.net/Articles/470820/

What we should do is patch all drivers to return 0 on failure, and
patch any consumers like mctrl_gpio_init() to handle that correctly.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: sh-pfc: Let gpio_chip.to_irq() return -ENXIO on error

[Geert Uytterhoeven]

Hi Linus,

On Sun, May 1, 2016 at 10:48 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> Currrently the gpio_chip.to_irq() callback returns -ENOSYS on error,
>> which causes bad interactions with the serial_mctrl_gpio helpers.
>>
>> mctrl_gpio_init() returns -ENOSYS if GPIOLIB is not enabled, which is
>> intended to be ignored by its callers. However, ignoring -ENOSYS when it
>> was caused by a gpiod_to_irq() failure will lead to a crash later:
>>
>>     Unable to handle kernel paging request at virtual address ffffffde
>>     ...
>>     PC is at mctrl_gpio_set+0x14/0x78
>>
>> Fix this by returning -ENXIO instead.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> Is -ENXIO the right error code?
>
> I would say that whatever the generic helpers in drivers/gpio/gpiolib.c
> returns should be the norm. However the guy who wrote them (me)
> isn't very smart either. It looks like so:
>
> static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
> {
>         return irq_find_mapping(chip->irqdomain, offset);
> }
>
> That returns 0 (NO_IRQ) on failure.
>
> And as you say:
>
>>   - Drivers that call irq_find_mapping(), irq_create_mapping(), or
>>     irq_create_fwspec_mapping() return zero!  This also applies to the
>>     core helper gpiochip_to_irq().
>
> Zero means NO_IRQ.
>
> Reminder:
> http://lwn.net/Articles/470820/
>
> What we should do is patch all drivers to return 0 on failure, and
> patch any consumers like mctrl_gpio_init() to handle that correctly.

That's the Long Term Plan. There are still too many non-zero NO_IRQ
definitions in use...

Is -ENXIO acceptable for the short term?

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] pinctrl: sh-pfc: Let gpio_chip.to_irq() return -ENXIO on error

[Linus Walleij]

On Mon, May 2, 2016 at 10:03 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Sun, May 1, 2016 at 10:48 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

>>>   - Drivers that call irq_find_mapping(), irq_create_mapping(), or
>>>     irq_create_fwspec_mapping() return zero!  This also applies to the
>>>     core helper gpiochip_to_irq().
>>
>> Zero means NO_IRQ.
>>
>> Reminder:
>> http://lwn.net/Articles/470820/
>>
>> What we should do is patch all drivers to return 0 on failure, and
>> patch any consumers like mctrl_gpio_init() to handle that correctly.
>
> That's the Long Term Plan. There are still too many non-zero NO_IRQ
> definitions in use...
>
> Is -ENXIO acceptable for the short term?

I don't understand. You say you have a problem  with
mctrl_gpio_init() which looks like this:

        ret = gpiod_to_irq(gpios->gpio[i]);
        if (ret <= 0) {
            dev_err(port->dev, (...)

This function is already *correctly* handling zero as NO_IRQ
i.e. an error.

Just make your driver return 0/NO_IRQ and it is fixed.

Or are there other problems that you're not telling about?

Yours,
Linus Walleij

Re: [PATCH] pinctrl: sh-pfc: Let gpio_chip.to_irq() return -ENXIO on error

[Geert Uytterhoeven]

Hi Linus,

On Mon, May 2, 2016 at 10:30 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, May 2, 2016 at 10:03 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Sun, May 1, 2016 at 10:48 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>   - Drivers that call irq_find_mapping(), irq_create_mapping(), or
>>>>     irq_create_fwspec_mapping() return zero!  This also applies to the
>>>>     core helper gpiochip_to_irq().
>>>
>>> Zero means NO_IRQ.
>>>
>>> Reminder:
>>> http://lwn.net/Articles/470820/
>>>
>>> What we should do is patch all drivers to return 0 on failure, and
>>> patch any consumers like mctrl_gpio_init() to handle that correctly.
>>
>> That's the Long Term Plan. There are still too many non-zero NO_IRQ
>> definitions in use...
>>
>> Is -ENXIO acceptable for the short term?
>
> I don't understand. You say you have a problem  with
> mctrl_gpio_init() which looks like this:
>
>         ret = gpiod_to_irq(gpios->gpio[i]);
>         if (ret <= 0) {
>             dev_err(port->dev, (...)
>
> This function is already *correctly* handling zero as NO_IRQ
> i.e. an error.
>
> Just make your driver return 0/NO_IRQ and it is fixed.
>
> Or are there other problems that you're not telling about?

"mctrl_gpio_init() returns -ENOSYS if GPIOLIB is not enabled, which is
 intended to be ignored by its callers. However, ignoring -ENOSYS when it
 was caused by a gpiod_to_irq() failure will lead to a crash later:"

Please see the !CONFIG_GPIOLIB stubs in drivers/tty/serial/serial_mctrl_gpio.h.

Hence -ENOSYS is to be ignored by the driver, and it's safe to call
any of the mctrl_gpio_*() helpers if !CONFIG_GPIOLIB. That was done so
drivers don't have to check for !CONFIG_GPIOLIB theirselves.

However, if CONFIG_GPIOLIB=y, and -ENOSYS was a real error, calling
any of the mctrl_gpio_*() helpers will cause a crash. Without testing for
CONFIG_GPIOLIB, there's no way the driver can know -ENOSYS was a
real error.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] pinctrl: sh-pfc: Let gpio_chip.to_irq() return -ENXIO on error

[Geert Uytterhoeven]

Hi Linus,

On Mon, May 2, 2016 at 10:53 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Mon, May 2, 2016 at 10:30 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Mon, May 2, 2016 at 10:03 AM, Geert Uytterhoeven
>> <geert@linux-m68k.org> wrote:
>>> On Sun, May 1, 2016 at 10:48 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>   - Drivers that call irq_find_mapping(), irq_create_mapping(), or
>>>>>     irq_create_fwspec_mapping() return zero!  This also applies to the
>>>>>     core helper gpiochip_to_irq().
>>>>
>>>> Zero means NO_IRQ.
>>>>
>>>> Reminder:
>>>> http://lwn.net/Articles/470820/
>>>>
>>>> What we should do is patch all drivers to return 0 on failure, and
>>>> patch any consumers like mctrl_gpio_init() to handle that correctly.
>>>
>>> That's the Long Term Plan. There are still too many non-zero NO_IRQ
>>> definitions in use...
>>>
>>> Is -ENXIO acceptable for the short term?
>>
>> I don't understand. You say you have a problem  with
>> mctrl_gpio_init() which looks like this:
>>
>>         ret = gpiod_to_irq(gpios->gpio[i]);
>>         if (ret <= 0) {
>>             dev_err(port->dev, (...)
>>
>> This function is already *correctly* handling zero as NO_IRQ
>> i.e. an error.
>>
>> Just make your driver return 0/NO_IRQ and it is fixed.
>>
>> Or are there other problems that you're not telling about?

[silly response deleted]

Scrap it.

The only annoying thing is that 0 cannot easily be propagated upstream as
an error code, so it has to be tested for explicitly.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] pinctrl: sh-pfc: Let gpio_chip.to_irq() return -ENXIO on error

[Linus Walleij]

On Mon, May 2, 2016 at 11:04 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:

> [silly response deleted]
>
> Scrap it.

:D

> The only annoying thing is that 0 cannot easily be propagated upstream as
> an error code, so it has to be tested for explicitly.

Well with the Big Penguin's clear opinion on the matter there is not
much we can do.

What about this?

diff --git a/drivers/tty/serial/serial_mctrl_gpio.c
b/drivers/tty/serial/serial_mctrl_gpio.c
index 02147361eaa9..2297ec781681 100644
--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -172,6 +172,13 @@ struct mctrl_gpios *mctrl_gpio_init(struct
uart_port *port, unsigned int idx)
                        dev_err(port->dev,
                                "failed to find corresponding irq for
%s (idx=%d, err=%d)\n",
                                mctrl_gpios_desc[i].name, idx, ret);
+                       /*
+                        * Satisfy the error code semantics for a missing IRQ,
+                        * 0 means NO_IRQ, but the framework needs to return
+                        * a negative to deal with the error.
+                        */
+                       if (!ret)
+                               ret = -ENOSYS;
                        return ERR_PTR(ret);
                }
                gpios->irq[i] = ret;

Yours,
Linus Walleij

Re: [PATCH] pinctrl: sh-pfc: Let gpio_chip.to_irq() return -ENXIO on error

[Geert Uytterhoeven]

Hi Linus,

On Mon, May 2, 2016 at 11:06 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, May 2, 2016 at 11:04 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>
>> [silly response deleted]
>>
>> Scrap it.
>
> :D
>
>> The only annoying thing is that 0 cannot easily be propagated upstream as
>> an error code, so it has to be tested for explicitly.
>
> Well with the Big Penguin's clear opinion on the matter there is not
> much we can do.
>
> What about this?
>
> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c
> b/drivers/tty/serial/serial_mctrl_gpio.c
> index 02147361eaa9..2297ec781681 100644
> --- a/drivers/tty/serial/serial_mctrl_gpio.c
> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
> @@ -172,6 +172,13 @@ struct mctrl_gpios *mctrl_gpio_init(struct
> uart_port *port, unsigned int idx)
>                         dev_err(port->dev,
>                                 "failed to find corresponding irq for
> %s (idx=%d, err=%d)\n",
>                                 mctrl_gpios_desc[i].name, idx, ret);
> +                       /*
> +                        * Satisfy the error code semantics for a missing IRQ,
> +                        * 0 means NO_IRQ, but the framework needs to return
> +                        * a negative to deal with the error.
> +                        */
> +                       if (!ret)
> +                               ret = -ENOSYS;

No, not -ENOSYS, as that triggers my initial problem again (please read the
deleted silly response ;-)
Checkpatch says: "ENOSYS means 'invalid syscall nr' and nothing else".

-EINVAL?

>                         return ERR_PTR(ret);
>                 }
>                 gpios->irq[i] = ret;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] pinctrl: sh-pfc: Let gpio_chip.to_irq() return -ENXIO on error

[Linus Walleij]

On Mon, May 2, 2016 at 11:10 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Mon, May 2, 2016 at 11:06 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Mon, May 2, 2016 at 11:04 AM, Geert Uytterhoeven
>> <geert@linux-m68k.org> wrote:
>>
>>> [silly response deleted]
>>>
>>> Scrap it.
>>
>> :D
>>
>>> The only annoying thing is that 0 cannot easily be propagated upstream as
>>> an error code, so it has to be tested for explicitly.
>>
>> Well with the Big Penguin's clear opinion on the matter there is not
>> much we can do.
>>
>> What about this?
>>
>> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c
>> b/drivers/tty/serial/serial_mctrl_gpio.c
>> index 02147361eaa9..2297ec781681 100644
>> --- a/drivers/tty/serial/serial_mctrl_gpio.c
>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
>> @@ -172,6 +172,13 @@ struct mctrl_gpios *mctrl_gpio_init(struct
>> uart_port *port, unsigned int idx)
>>                         dev_err(port->dev,
>>                                 "failed to find corresponding irq for
>> %s (idx=%d, err=%d)\n",
>>                                 mctrl_gpios_desc[i].name, idx, ret);
>> +                       /*
>> +                        * Satisfy the error code semantics for a missing IRQ,
>> +                        * 0 means NO_IRQ, but the framework needs to return
>> +                        * a negative to deal with the error.
>> +                        */
>> +                       if (!ret)
>> +                               ret = -ENOSYS;
>
> No, not -ENOSYS, as that triggers my initial problem again (please read the
> deleted silly response ;-)
> Checkpatch says: "ENOSYS means 'invalid syscall nr' and nothing else".
>
> -EINVAL?

Sure, whatever works with your semantics.

Will you test & send a patch?

Yours,
Linus Walleij

Re: [PATCH] pinctrl: sh-pfc: Let gpio_chip.to_irq() return -ENXIO on error

[Geert Uytterhoeven]

Hi Linus,

On Mon, May 2, 2016 at 11:25 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, May 2, 2016 at 11:10 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Mon, May 2, 2016 at 11:06 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Mon, May 2, 2016 at 11:04 AM, Geert Uytterhoeven
>>> <geert@linux-m68k.org> wrote:
>>>
>>>> [silly response deleted]
>>>>
>>>> Scrap it.
>>>
>>> :D
>>>
>>>> The only annoying thing is that 0 cannot easily be propagated upstream as
>>>> an error code, so it has to be tested for explicitly.
>>>
>>> Well with the Big Penguin's clear opinion on the matter there is not
>>> much we can do.
>>>
>>> What about this?
>>>
>>> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c
>>> b/drivers/tty/serial/serial_mctrl_gpio.c
>>> index 02147361eaa9..2297ec781681 100644
>>> --- a/drivers/tty/serial/serial_mctrl_gpio.c
>>> +++ b/drivers/tty/serial/serial_mctrl_gpio.c
>>> @@ -172,6 +172,13 @@ struct mctrl_gpios *mctrl_gpio_init(struct
>>> uart_port *port, unsigned int idx)
>>>                         dev_err(port->dev,
>>>                                 "failed to find corresponding irq for
>>> %s (idx=%d, err=%d)\n",
>>>                                 mctrl_gpios_desc[i].name, idx, ret);
>>> +                       /*
>>> +                        * Satisfy the error code semantics for a missing IRQ,
>>> +                        * 0 means NO_IRQ, but the framework needs to return
>>> +                        * a negative to deal with the error.
>>> +                        */
>>> +                       if (!ret)
>>> +                               ret = -ENOSYS;
>>
>> No, not -ENOSYS, as that triggers my initial problem again (please read the
>> deleted silly response ;-)
>> Checkpatch says: "ENOSYS means 'invalid syscall nr' and nothing else".
>>
>> -EINVAL?

Upon reconsideration, -ENXIO.

> Sure, whatever works with your semantics.
>
> Will you test & send a patch?

This works fine, but I'm a bit reluctant to send out the patches...

gpiod_to_irq() is documented to return a negative error code on failure,
cfr. Documentation/gpio/consumer.txt and drivers/gpio/gpiolib.c.
In fact the check in mctrl_gpio_init() is the only caller that
considers zero as an
error.

Perhaps gpiod_to_irq() should handle .to_irq() returning zero instead?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] pinctrl: sh-pfc: Let gpio_chip.to_irq() return -ENXIO on error

[Linus Walleij]

On Mon, May 2, 2016 at 12:05 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:

> gpiod_to_irq() is documented to return a negative error code on failure,
> cfr. Documentation/gpio/consumer.txt and drivers/gpio/gpiolib.c.
> In fact the check in mctrl_gpio_init() is the only caller that
> considers zero as an
> error.
>
> Perhaps gpiod_to_irq() should handle .to_irq() returning zero instead?

OK good point, I sent a patch like so, let's see what people say.

I'm a bit torn: part of me feel that since the function has *_to_irq()
in it it should use zero for NO_IRQ if there is no translation.

But this works too, whatever, rough consensus and running code.

Yours,
Linus Walleij