Re: Bug report - gpio: improve error path in gpiolib

Re: Bug report - gpio: improve error path in gpiolib

[Alex Courbot]

Hi Nikolaus,

On 10/03/2013 01:22 AM, Dr. H. Nikolaus Schaller wrote:
> Hi Alexandre,
> i think have traced down one problem we have to the latest patch in 3.12-rc:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/gpio/gpiolib.c?id=be1a4b13089b1e18da83a549d49163ccad3c19ba
>
> The problem is that by this patch a desc == NULL now returns -EINVAL while earlier it did return an -EPROBE_DEFER, which I consider the better option.
>
> By this -EINVAL, now some dependent drivers are no longer initialized.
>
> So I't suggest to revert that.

Adding Linus since he authored the patch.

I suppose the behavior should not change indeed. But could you specify 
which use-case now returns -EINVAL instead of -EPROBE_DEFER? It is not 
obvious by just looking at the patch alone.

Thanks,
Alex.


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: Bug report - gpio: improve error path in gpiolib

[Dr. H. Nikolaus Schaller]

Hi all,

Am 04.10.2013 um 03:27 schrieb Alex Courbot:

> Hi Nikolaus,
> 
> On 10/03/2013 01:22 AM, Dr. H. Nikolaus Schaller wrote:
>> Hi Alexandre,
>> i think have traced down one problem we have to the latest patch in 3.12-rc:
>> 
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/gpio/gpiolib.c?id=be1a4b13089b1e18da83a549d49163ccad3c19ba
>> 
>> The problem is that by this patch a desc == NULL now returns -EINVAL while earlier it did return an -EPROBE_DEFER, which I consider the better option.
>> 
>> By this -EINVAL, now some dependent drivers are no longer initialized.
>> 
>> So I't suggest to revert that.
> 
> Adding Linus since he authored the patch.
> 
> I suppose the behavior should not change indeed. But could you specify which use-case now returns -EINVAL instead of -EPROBE_DEFER? It is not obvious by just looking at the patch alone.

To give some more background information and how we use the gpio system to run
into this issue:

We are currently in the middle of preparing patches for upstreaming for the ARM
based GTA04 board.

One of these additions needs a "reverse gpio-regulator" driver. It presents itself
as a virtual GPIO to other drivers that expect to control a GPIO but internally controls
a regulator. We need this to enable some external power e.g. if the DTR line of
an UART is asserted by the UART driver so that an external GPS chip&antenna
gets powered on.

The code of that driver is here:

https://github.com/goldelico/gta04-kernel/blob/master/drivers/gpio/gpio-reg.c

It was developed from 3.4 to 3.7 and I have started to forward-port it to 3.12-rc
to make it ready for future submission.

There I found that this GPIO is now being treated as invalid instead of
-EPROBE_DEFER and is therefore no longer deferring the probing of the
UART. I.e. the whole UART probing process fails.

This appears to have its root in the function gpiod_request() that the descriptor
exists but not the chip pointer.

If I understand the old code correctly it did return -EINVAL only for a NULL
descriptor but did return the default value of the variable 'status' if desc != NULL
but desc->chip == NULL. This default is -EPROBE_DEFER.

I don't know the exact interworking of gpiolib and gpiod_request() with our
driver and it's probe function, but adding this line at the beginning of gpiod_request
did make the whole deferring chain work again (after IMHO restoring the "older"
logic of gpiod_request up to 3.11):

	if (desc && !desc->chip)
		return -EPROBE_DEFER;

BR,
Nikolaus Schaller

Re: Bug report - gpio: improve error path in gpiolib

[Alexandre Courbot]

On Fri, Oct 4, 2013 at 3:21 PM, Dr. H. Nikolaus Schaller
<hns@goldelico.com> wrote:
> Hi all,
>
> Am 04.10.2013 um 03:27 schrieb Alex Courbot:
>
>> Hi Nikolaus,
>>
>> On 10/03/2013 01:22 AM, Dr. H. Nikolaus Schaller wrote:
>>> Hi Alexandre,
>>> i think have traced down one problem we have to the latest patch in 3.12-rc:
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/gpio/gpiolib.c?id=be1a4b13089b1e18da83a549d49163ccad3c19ba
>>>
>>> The problem is that by this patch a desc == NULL now returns -EINVAL while earlier it did return an -EPROBE_DEFER, which I consider the better option.
>>>
>>> By this -EINVAL, now some dependent drivers are no longer initialized.
>>>
>>> So I't suggest to revert that.
>>
>> Adding Linus since he authored the patch.
>>
>> I suppose the behavior should not change indeed. But could you specify which use-case now returns -EINVAL instead of -EPROBE_DEFER? It is not obvious by just looking at the patch alone.
>
> To give some more background information and how we use the gpio system to run
> into this issue:
>
> We are currently in the middle of preparing patches for upstreaming for the ARM
> based GTA04 board.
>
> One of these additions needs a "reverse gpio-regulator" driver. It presents itself
> as a virtual GPIO to other drivers that expect to control a GPIO but internally controls
> a regulator. We need this to enable some external power e.g. if the DTR line of
> an UART is asserted by the UART driver so that an external GPS chip&antenna
> gets powered on.
>
> The code of that driver is here:
>
> https://github.com/goldelico/gta04-kernel/blob/master/drivers/gpio/gpio-reg.c
>
> It was developed from 3.4 to 3.7 and I have started to forward-port it to 3.12-rc
> to make it ready for future submission.
>
> There I found that this GPIO is now being treated as invalid instead of
> -EPROBE_DEFER and is therefore no longer deferring the probing of the
> UART. I.e. the whole UART probing process fails.
>
> This appears to have its root in the function gpiod_request() that the descriptor
> exists but not the chip pointer.
>
> If I understand the old code correctly it did return -EINVAL only for a NULL
> descriptor but did return the default value of the variable 'status' if desc != NULL
> but desc->chip == NULL. This default is -EPROBE_DEFER.
>
> I don't know the exact interworking of gpiolib and gpiod_request() with our
> driver and it's probe function, but adding this line at the beginning of gpiod_request
> did make the whole deferring chain work again (after IMHO restoring the "older"
> logic of gpiod_request up to 3.11):
>
>         if (desc && !desc->chip)
>                 return -EPROBE_DEFER;

I see - that's fair enough, the return value of gpiod_request() should
not change, especially when it bears such useful information.

I have just sent 2 patches that should fix this issue. Could you test
them and give your Tested-by after confirming they work? Then I think
Linus can safely merge them (disclaimer: I have not tested them myself
yet).

Thanks,
Alex.

Re: Bug report - gpio: improve error path in gpiolib

[Dr. H. Nikolaus Schaller]

Hi Alexandre,

Am 04.10.2013 um 20:02 schrieb Alexandre Courbot:

> On Fri, Oct 4, 2013 at 3:21 PM, Dr. H. Nikolaus Schaller
> <hns@goldelico.com> wrote:
>> Hi all,
>> 
>> Am 04.10.2013 um 03:27 schrieb Alex Courbot:
>> 
>>> Hi Nikolaus,
>>> 
>>> On 10/03/2013 01:22 AM, Dr. H. Nikolaus Schaller wrote:
>>>> Hi Alexandre,
>>>> i think have traced down one problem we have to the latest patch in 3.12-rc:
>>>> 
>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/gpio/gpiolib.c?id=be1a4b13089b1e18da83a549d49163ccad3c19ba
>>>> 
>>>> The problem is that by this patch a desc == NULL now returns -EINVAL while earlier it did return an -EPROBE_DEFER, which I consider the better option.
>>>> 
>>>> By this -EINVAL, now some dependent drivers are no longer initialized.
>>>> 
>>>> So I't suggest to revert that.
>>> 
>>> Adding Linus since he authored the patch.
>>> 
>>> I suppose the behavior should not change indeed. But could you specify which use-case now returns -EINVAL instead of -EPROBE_DEFER? It is not obvious by just looking at the patch alone.
>> 
>> To give some more background information and how we use the gpio system to run
>> into this issue:
>> 
>> We are currently in the middle of preparing patches for upstreaming for the ARM
>> based GTA04 board.
>> 
>> One of these additions needs a "reverse gpio-regulator" driver. It presents itself
>> as a virtual GPIO to other drivers that expect to control a GPIO but internally controls
>> a regulator. We need this to enable some external power e.g. if the DTR line of
>> an UART is asserted by the UART driver so that an external GPS chip&antenna
>> gets powered on.
>> 
>> The code of that driver is here:
>> 
>> https://github.com/goldelico/gta04-kernel/blob/master/drivers/gpio/gpio-reg.c
>> 
>> It was developed from 3.4 to 3.7 and I have started to forward-port it to 3.12-rc
>> to make it ready for future submission.
>> 
>> There I found that this GPIO is now being treated as invalid instead of
>> -EPROBE_DEFER and is therefore no longer deferring the probing of the
>> UART. I.e. the whole UART probing process fails.
>> 
>> This appears to have its root in the function gpiod_request() that the descriptor
>> exists but not the chip pointer.
>> 
>> If I understand the old code correctly it did return -EINVAL only for a NULL
>> descriptor but did return the default value of the variable 'status' if desc != NULL
>> but desc->chip == NULL. This default is -EPROBE_DEFER.
>> 
>> I don't know the exact interworking of gpiolib and gpiod_request() with our
>> driver and it's probe function, but adding this line at the beginning of gpiod_request
>> did make the whole deferring chain work again (after IMHO restoring the "older"
>> logic of gpiod_request up to 3.11):
>> 
>>        if (desc && !desc->chip)
>>                return -EPROBE_DEFER;
> 
> I see - that's fair enough, the return value of gpiod_request() should
> not change, especially when it bears such useful information.
> 
> I have just sent 2 patches that should fix this issue.

Great.

> Could you test
> them and give your Tested-by after confirming they work?

Yes, these patches work for us as well and have the intended
effect (like my quick hack).

Tested-by: Dr. H. Nikolaus Schaller <hns@goldelico.com>

> 

> Then I think
> Linus can safely merge them (disclaimer: I have not tested them myself
> yet).
> 
> Thanks,
> Alex.

Thanks to you,
Nikolaus

Re: Bug report - gpio: improve error path in gpiolib

[Alexandre Courbot]

On Sat, Oct 5, 2013 at 1:50 PM, Dr. H. Nikolaus Schaller
<hns@goldelico.com> wrote:
>> I have just sent 2 patches that should fix this issue.
>
> Great.
>
>> Could you test
>> them and give your Tested-by after confirming they work?
>
> Yes, these patches work for us as well and have the intended
> effect (like my quick hack).
>
> Tested-by: Dr. H. Nikolaus Schaller <hns@goldelico.com>

Great! Linus, are you ok to take these into your tree? They should
probably go into 3.12 as the patch that causes the difference in
behavior is already merged.

Thanks,
Alex.

Re: Bug report - gpio: improve error path in gpiolib

[Linus Walleij]

On Sun, Oct 6, 2013 at 8:55 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Sat, Oct 5, 2013 at 1:50 PM, Dr. H. Nikolaus Schaller
> <hns@goldelico.com> wrote:
>>> I have just sent 2 patches that should fix this issue.
>>
>> Great.
>>
>>> Could you test
>>> them and give your Tested-by after confirming they work?
>>
>> Yes, these patches work for us as well and have the intended
>> effect (like my quick hack).
>>
>> Tested-by: Dr. H. Nikolaus Schaller <hns@goldelico.com>
>
> Great! Linus, are you ok to take these into your tree? They should
> probably go into 3.12 as the patch that causes the difference in
> behavior is already merged.

OK saw this now, move these two for fixes.

Yours,
Linus Walleij