gpio: don't WARN() on NULL descs if gpiolib is disabled

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

If gpiolib is disabled, we use the inline stubs from gpio/consumer.h
instead of regular definitions of GPIO API. The stubs for 'optional'
variants of gpiod_get routines return NULL in this case as if the
relevant GPIO wasn't found. This is correct so far.

Calling other (non-gpio_get) stubs from this header triggers a warning
because the GPIO descriptor couldn't have been requested. The warning
however is unconditional (WARN_ON(1)) and is emitted even if the passed
descriptor pointer is NULL.

We don't want to force the users of 'optional' gpio_get to check the
returned pointer before calling e.g. gpiod_set_value() so let's only
WARN on non-NULL descriptors.

Reported-by: Claus H. Stovgaard <cst@phaseone.com>
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 include/linux/gpio/consumer.h | 64 +++++++++++++++++------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

On Mon, 2019-07-08 at 10:23 +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> If gpiolib is disabled, we use the inline stubs from gpio/consumer.h
> instead of regular definitions of GPIO API. The stubs for 'optional'
> variants of gpiod_get routines return NULL in this case as if the
> relevant GPIO wasn't found. This is correct so far.
> 
> Calling other (non-gpio_get) stubs from this header triggers a
> warning
> because the GPIO descriptor couldn't have been requested. The warning
> however is unconditional (WARN_ON(1)) and is emitted even if the
> passed
> descriptor pointer is NULL.
> 
> We don't want to force the users of 'optional' gpio_get to check the
> returned pointer before calling e.g. gpiod_set_value() so let's only
> WARN on non-NULL descriptors.
> 
> Reported-by: Claus H. Stovgaard <cst@phaseone.com>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  include/linux/gpio/consumer.h | 64 +++++++++++++++++--------------
> ----
>  1 file changed, 32 insertions(+), 32 deletions(-)

Have tested it on my setup (ZynqMP with AT24 EEPROM), where it works
great. This have removed the warnings in the log regarding settting
wp_gpio for AT24.

Thanks
Claus

Hi Bartosz,

On Mon, Jul 8, 2019 at 10:25 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> If gpiolib is disabled, we use the inline stubs from gpio/consumer.h
> instead of regular definitions of GPIO API. The stubs for 'optional'
> variants of gpiod_get routines return NULL in this case as if the
> relevant GPIO wasn't found. This is correct so far.
>
> Calling other (non-gpio_get) stubs from this header triggers a warning
> because the GPIO descriptor couldn't have been requested. The warning
> however is unconditional (WARN_ON(1)) and is emitted even if the passed
> descriptor pointer is NULL.
>
> We don't want to force the users of 'optional' gpio_get to check the
> returned pointer before calling e.g. gpiod_set_value() so let's only
> WARN on non-NULL descriptors.
>
> Reported-by: Claus H. Stovgaard <cst@phaseone.com>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

I remember I had this discussion in the past, and I made a large
refactoring to make it possible for drivers that need gpiod_*
calls to simply do:

select GPIOLIB

in Kconfig.

This should solve also this problem I think.

However I do realize that there may be situations where people
simply want to make GPIO support entirely optional without
having to e.g. create custom stubs and encapsulate things
inside if IS_ENABLED(CONFIG_GPIOLIB).

I was thinking something like this in the stubs:

gpiod_get[_index]() {
    return POISON;
}

gpiod_get[_index]_optional() {
   return NULL;
}

This way all gpiod_get() and optional calls are properly
handled and the semantic that only _optional calls
can return NULL is preserved. (Your patch would
violate this.)

Then other stubs can do:

gpiod_set_value() {
  WARN_ON(desc);
}

As in your patch, and all will be smooth provided the
_optional calls have been used to obtain the desc.

Yours,
Linus Walleij

wt., 9 lip 2019 o 15:30 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> Hi Bartosz,
>
> On Mon, Jul 8, 2019 at 10:25 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > If gpiolib is disabled, we use the inline stubs from gpio/consumer.h
> > instead of regular definitions of GPIO API. The stubs for 'optional'
> > variants of gpiod_get routines return NULL in this case as if the
> > relevant GPIO wasn't found. This is correct so far.
> >
> > Calling other (non-gpio_get) stubs from this header triggers a warning
> > because the GPIO descriptor couldn't have been requested. The warning
> > however is unconditional (WARN_ON(1)) and is emitted even if the passed
> > descriptor pointer is NULL.
> >
> > We don't want to force the users of 'optional' gpio_get to check the
> > returned pointer before calling e.g. gpiod_set_value() so let's only
> > WARN on non-NULL descriptors.
> >
> > Reported-by: Claus H. Stovgaard <cst@phaseone.com>
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> I remember I had this discussion in the past, and I made a large
> refactoring to make it possible for drivers that need gpiod_*
> calls to simply do:
>
> select GPIOLIB
>
> in Kconfig.
>
> This should solve also this problem I think.
>
> However I do realize that there may be situations where people
> simply want to make GPIO support entirely optional without
> having to e.g. create custom stubs and encapsulate things
> inside if IS_ENABLED(CONFIG_GPIOLIB).
>

In this case the board doesn't provide any GPIO controller at all so
there's simply no need to select GPIOLIB - it would only add bloat.

> I was thinking something like this in the stubs:
>
> gpiod_get[_index]() {
>     return POISON;
> }
>
> gpiod_get[_index]_optional() {
>    return NULL;
> }

This is already being done.

>
> This way all gpiod_get() and optional calls are properly
> handled and the semantic that only _optional calls
> can return NULL is preserved. (Your patch would
> violate this.)
>

Maybe I'm missing something, but I don't quite see how my patch
violates this behavior. :(

> Then other stubs can do:
>
> gpiod_set_value() {
>   WARN_ON(desc);
> }
>
> As in your patch, and all will be smooth provided the
> _optional calls have been used to obtain the desc.
>
> Yours,
> Linus Walleij

Bart

On 09.07.19 15:30, Linus Walleij wrote:

Hi,

> I remember I had this discussion in the past, and I made a large> refactoring to make it possible for drivers that need gpiod_*> calls
to simply do:> > select GPIOLIB> > in Kconfig.
Would that allow enabling gpio consumers or drivers selectable w/o
having the gpio subsystem enabled first ?

--mtx

Hi Linus

On tir, 2019-07-09 at 15:30 +0200, Linus Walleij wrote:
> 
> I remember I had this discussion in the past, and I made a large
> refactoring to make it possible for drivers that need gpiod_*
> calls to simply do:
> 
> select GPIOLIB
> 
> in Kconfig.
> 
> This should solve also this problem I think.
> 
> However I do realize that there may be situations where people
> simply want to make GPIO support entirely optional without
> having to e.g. create custom stubs and encapsulate things
> inside if IS_ENABLED(CONFIG_GPIOLIB).

You properly saw the email in linux-i2c, but will just it for
completeness.
https://marc.info/?l=linux-i2c&m=156257442124566&w=2

The background for getting the warning, was upgrading a system on
Xilinx ZynqMP SoC from 4.14 to 4.19. As part of this upgrade wp (write
protect) has been introduced in the at24 driver, resulting in the
warning from comsumer.h as I don't have enabled GPIOLIB.

Regards
Claus

On Tue, Jul 9, 2019 at 4:20 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> wt., 9 lip 2019 o 15:30 Linus Walleij <linus.walleij@linaro.org> napisał(a):

> > I was thinking something like this in the stubs:
> >
> > gpiod_get[_index]() {
> >     return POISON;
> > }
> >
> > gpiod_get[_index]_optional() {
> >    return NULL;
> > }
>
> This is already being done.

Ah it is.

> > This way all gpiod_get() and optional calls are properly
> > handled and the semantic that only _optional calls
> > can return NULL is preserved. (Your patch would
> > violate this.)
> >
>
> Maybe I'm missing something, but I don't quite see how my patch
> violates this behavior. :(

I missed that we actually do pass a poison from the strict
*get functions, mea culpa.

Let's apply this, will you send me a pull request or shall I
just try to apply it?

Yours,
Linus Walleij

wt., 16 lip 2019 o 23:46 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Tue, Jul 9, 2019 at 4:20 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > wt., 9 lip 2019 o 15:30 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> > > I was thinking something like this in the stubs:
> > >
> > > gpiod_get[_index]() {
> > >     return POISON;
> > > }
> > >
> > > gpiod_get[_index]_optional() {
> > >    return NULL;
> > > }
> >
> > This is already being done.
>
> Ah it is.
>
> > > This way all gpiod_get() and optional calls are properly
> > > handled and the semantic that only _optional calls
> > > can return NULL is preserved. (Your patch would
> > > violate this.)
> > >
> >
> > Maybe I'm missing something, but I don't quite see how my patch
> > violates this behavior. :(
>
> I missed that we actually do pass a poison from the strict
> *get functions, mea culpa.
>
> Let's apply this, will you send me a pull request or shall I
> just try to apply it?
>
> Yours,
> Linus Walleij

I'll apply it to my local tree and send it for v5.3-rc2.

Bart

On Sat, Jul 20, 2019 at 8:03 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:

> I'll apply it to my local tree and send it for v5.3-rc2.

OK! Do you see it as bug fix so it should go in the rcs?

It pretty much needs to be a regression to go in there,
because this stub stuff blew up in my face before :/

Thanks,
Linus

sob., 20 lip 2019 o 21:45 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Sat, Jul 20, 2019 at 8:03 PM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
>
> > I'll apply it to my local tree and send it for v5.3-rc2.
>
> OK! Do you see it as bug fix so it should go in the rcs?
>
> It pretty much needs to be a regression to go in there,
> because this stub stuff blew up in my face before :/
>
> Thanks,
> Linus

It causes a warning every time someone writes to an at24 EEPROM
without GPIOLIB selected. To me it sounds like a bug that should go
into stable. Let me know if I'm wrong in thinking that, then I'll send
it for v5.4.

Bart