Re: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Linus Walleij]

On Thu, Sep 7, 2017 at 11:39 PM, Florian Fainelli  wrote:

> I think it means CONFIG_GPIOLIB=n in the kernel because it's not needed,
> yet you run code (like drivers/net/phy/mdio_bus.c) that unconditionally
> calls into GPIOLIB and attempts to configure a given GPIO if available.
> This thread is actually what prompted me to write this email:

Yeah I think GPIOLIB should simply be selected in KConfig then.

Sent an inline patch to do that in the other reply.

Maybe I should simply delete the stubs if they confuse
people.

It is possible to select GPIOLIB on any platform these days
and get the right APIs, this used to not be the case but I fixed
it a while back.

Alternatively we can depend on GPIOLIB but it's simpler to just
select it I think, it's like a library this code uses and so that's
it.

Yours,
Linus Walleij

RE: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Woojung.Huh]

> >> I think it means CONFIG_GPIOLIB=n in the kernel because it's not needed,
> >> yet you run code (like drivers/net/phy/mdio_bus.c) that unconditionally
> >> calls into GPIOLIB and attempts to configure a given GPIO if available.
> > Yes. I'm facing issue on PC which won't need GPIOLIB as default.
> > Warning message goes away when GPIOLIB is enabled, and fortunately,
> > Ubuntu default config has it.
> > So, it may not be seen by many users when with full/default configuration.
> 
> Woojung, I suppose you are also getting a warning from
> gpiod_set_value_cansleep() done in mdiobus_unregister() right? With
> CONFIG_GPIOLIB=n devm_gpiod_get_optional() returns NULL, which we
> don't
> check as an error, on purpose however we still call
> gpiod_set_value_cansleep() on a NULL GPIO descriptor, so the following
> should do:
> 
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index b6f9fa670168..67dbb7c26840 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -424,7 +424,8 @@ void mdiobus_unregister(struct mii_bus *bus)
> }
> 
> /* Put PHYs in RESET to save power */
> -   gpiod_set_value_cansleep(bus->reset_gpiod, 1);
> +   if (!IS_ERR_OR_NULL(bus->reset_gpiod))
> +   gpiod_set_value_cansleep(bus->reset_gpiod, 1);
> 
> device_del(>dev);
>  }
Hi Florian,

Thanks for the patch. I'm avoiding warning with CONFIG_GPIOLIB=y for now.
I'm curious that there is a final conclusion to resolve this issue,
either with CONFIG_GPIOLIB=y or something else.

- Woojung

Re: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Florian Fainelli]

On 09/07/2017 02:51 PM, woojung@microchip.com wrote:
> If someone is using GPIO descriptors with GPIO disabled, i.e. calling
> gpiod_get() and friends, this is very likely to be a bug, and what
> the driver wants to do is:
>
>  depends on GPIOLIB
>
> or
>
>  select GPIOLIB
>
> in Kconfig. The whole optional thing is mainly a leftover from when it
> was possible to have a local implementation of the GPIOLIB API in
> some custom header file, noone sane should be doing that anymore,
> and if they do, they can very well face the warnings.
>
> If someone is facing a lot of WARN_ON() messages to this, it is a clear
> indication that they need to fix their Kconfig and in that case it is 
> proper.
 Linus & Andrew,

 I knew that it is already in David's pulling request.
 Configuring GPIOLIB is the right solution  even if platform doesn't use it?
>>>
>>> I guess?
>>>
>>> "Platform doesn't use it" what does that mean?
>>>
>>> Does it mean it does not call the
>>> APIs of the GPIOLIB, does it mean it doesn't have a GPIO driver
>>> at probe (but may have one by having it probed from a module)
>>> or does it mean the platform can never have it?
>>
>> I think it means CONFIG_GPIOLIB=n in the kernel because it's not needed,
>> yet you run code (like drivers/net/phy/mdio_bus.c) that unconditionally
>> calls into GPIOLIB and attempts to configure a given GPIO if available.
> Yes. I'm facing issue on PC which won't need GPIOLIB as default.
> Warning message goes away when GPIOLIB is enabled, and fortunately,
> Ubuntu default config has it.
> So, it may not be seen by many users when with full/default configuration.

Woojung, I suppose you are also getting a warning from
gpiod_set_value_cansleep() done in mdiobus_unregister() right? With
CONFIG_GPIOLIB=n devm_gpiod_get_optional() returns NULL, which we don't
check as an error, on purpose however we still call
gpiod_set_value_cansleep() on a NULL GPIO descriptor, so the following
should do:

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index b6f9fa670168..67dbb7c26840 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -424,7 +424,8 @@ void mdiobus_unregister(struct mii_bus *bus)
}

/* Put PHYs in RESET to save power */
-   gpiod_set_value_cansleep(bus->reset_gpiod, 1);
+   if (!IS_ERR_OR_NULL(bus->reset_gpiod))
+   gpiod_set_value_cansleep(bus->reset_gpiod, 1);

device_del(>dev);
 }


> 
>> This thread is actually what prompted me to write this email:
>>
>> https://lkml.org/lkml/2017/9/2/3
> Thanks for the link.
> 
> 


-- 
Florian

RE: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Woojung.Huh]

> >>> If someone is using GPIO descriptors with GPIO disabled, i.e. calling
> >>> gpiod_get() and friends, this is very likely to be a bug, and what
> >>> the driver wants to do is:
> >>>
> >>>  depends on GPIOLIB
> >>>
> >>> or
> >>>
> >>>  select GPIOLIB
> >>>
> >>> in Kconfig. The whole optional thing is mainly a leftover from when it
> >>> was possible to have a local implementation of the GPIOLIB API in
> >>> some custom header file, noone sane should be doing that anymore,
> >>> and if they do, they can very well face the warnings.
> >>>
> >>> If someone is facing a lot of WARN_ON() messages to this, it is a clear
> >>> indication that they need to fix their Kconfig and in that case it is 
> >>> proper.
> >> Linus & Andrew,
> >>
> >> I knew that it is already in David's pulling request.
> >> Configuring GPIOLIB is the right solution  even if platform doesn't use it?
> >
> > I guess?
> >
> > "Platform doesn't use it" what does that mean?
> >
> > Does it mean it does not call the
> > APIs of the GPIOLIB, does it mean it doesn't have a GPIO driver
> > at probe (but may have one by having it probed from a module)
> > or does it mean the platform can never have it?
> 
> I think it means CONFIG_GPIOLIB=n in the kernel because it's not needed,
> yet you run code (like drivers/net/phy/mdio_bus.c) that unconditionally
> calls into GPIOLIB and attempts to configure a given GPIO if available.
Yes. I'm facing issue on PC which won't need GPIOLIB as default.
Warning message goes away when GPIOLIB is enabled, and fortunately,
Ubuntu default config has it.
So, it may not be seen by many users when with full/default configuration.

> This thread is actually what prompted me to write this email:
>
> https://lkml.org/lkml/2017/9/2/3
Thanks for the link.

Re: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Florian Fainelli]

On 09/07/2017 02:33 PM, Linus Walleij wrote:
> On Thu, Sep 7, 2017 at 11:30 PM,   wrote:
>>> If someone is using GPIO descriptors with GPIO disabled, i.e. calling
>>> gpiod_get() and friends, this is very likely to be a bug, and what
>>> the driver wants to do is:
>>>
>>>  depends on GPIOLIB
>>>
>>> or
>>>
>>>  select GPIOLIB
>>>
>>> in Kconfig. The whole optional thing is mainly a leftover from when it
>>> was possible to have a local implementation of the GPIOLIB API in
>>> some custom header file, noone sane should be doing that anymore,
>>> and if they do, they can very well face the warnings.
>>>
>>> If someone is facing a lot of WARN_ON() messages to this, it is a clear
>>> indication that they need to fix their Kconfig and in that case it is 
>>> proper.
>> Linus & Andrew,
>>
>> I knew that it is already in David's pulling request.
>> Configuring GPIOLIB is the right solution  even if platform doesn't use it?
> 
> I guess?
> 
> "Platform doesn't use it" what does that mean?
> 
> Does it mean it does not call the
> APIs of the GPIOLIB, does it mean it doesn't have a GPIO driver
> at probe (but may have one by having it probed from a module)
> or does it mean the platform can never have it?

I think it means CONFIG_GPIOLIB=n in the kernel because it's not needed,
yet you run code (like drivers/net/phy/mdio_bus.c) that unconditionally
calls into GPIOLIB and attempts to configure a given GPIO if available.
This thread is actually what prompted me to write this email:

https://lkml.org/lkml/2017/9/2/3

So that's the takeway should we just conditionalize all the GPIOLIB
calls from drivers/net/phy/mdio_bus.c under an #if
IS_ENABLED(CONFIG_GPIOLIB) of some sort?

> 
> If it calls the APIs, it is using it.
> 

It's more subtle than that, the GPIO resource may not be available just
like you have disabled support for GPIOLIB in the kernel, in which case,
the calls are still there, they default to their inline stubs, you get
warnings, everyone panics and screams.
-- 
Florian

Re: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Linus Walleij]

On Thu, Sep 7, 2017 at 11:30 PM,   wrote:
>> If someone is using GPIO descriptors with GPIO disabled, i.e. calling
>> gpiod_get() and friends, this is very likely to be a bug, and what
>> the driver wants to do is:
>>
>>  depends on GPIOLIB
>>
>> or
>>
>>  select GPIOLIB
>>
>> in Kconfig. The whole optional thing is mainly a leftover from when it
>> was possible to have a local implementation of the GPIOLIB API in
>> some custom header file, noone sane should be doing that anymore,
>> and if they do, they can very well face the warnings.
>>
>> If someone is facing a lot of WARN_ON() messages to this, it is a clear
>> indication that they need to fix their Kconfig and in that case it is proper.
> Linus & Andrew,
>
> I knew that it is already in David's pulling request.
> Configuring GPIOLIB is the right solution  even if platform doesn't use it?

I guess?

"Platform doesn't use it" what does that mean?

Does it mean it does not call the
APIs of the GPIOLIB, does it mean it doesn't have a GPIO driver
at probe (but may have one by having it probed from a module)
or does it mean the platform can never have it?

If it calls the APIs, it is using it.

Yours,
Linus Walleij

RE: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Woojung.Huh]

> If someone is using GPIO descriptors with GPIO disabled, i.e. calling
> gpiod_get() and friends, this is very likely to be a bug, and what
> the driver wants to do is:
> 
>  depends on GPIOLIB
> 
> or
> 
>  select GPIOLIB
> 
> in Kconfig. The whole optional thing is mainly a leftover from when it
> was possible to have a local implementation of the GPIOLIB API in
> some custom header file, noone sane should be doing that anymore,
> and if they do, they can very well face the warnings.
> 
> If someone is facing a lot of WARN_ON() messages to this, it is a clear
> indication that they need to fix their Kconfig and in that case it is proper.
Linus & Andrew,

I knew that it is already in David's pulling request.
Configuring GPIOLIB is the right solution  even if platform doesn't use it?

Did I miss any new patch?

Thanks.
Woojung

Re: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Linus Walleij]

On Tue, Jul 18, 2017 at 3:32 PM, Andrew Lunn  wrote:
> On Tue, Jul 18, 2017 at 09:52:51AM -0300, Fabio Estevam wrote:
>> On Tue, Jul 18, 2017 at 9:48 AM, Sergei Shtylyov
>>  wrote:
>> > On 07/18/2017 03:39 PM, Fabio Estevam wrote:
>> >
>> >>>Won't this result in kernel WARNING when GPIO is disabled?
>> >
>> >
>> >GPIO support, I was going to type...
>> >
>> >> Not sure if I understood your point, but gpiod_set_value_cansleep() is
>> >> a no-op when the gpiod is NULL.
>> >
>> >
>> >Look at the stub in , it has WARN_ON(1).
>>
>> This patch does not alter the behavior of the driver with respect to
>> GPIO being disabled, so I still do not understand your concern.
>
> http://elixir.free-electrons.com/linux/latest/source/include/linux/gpio/consumer.h#L345
> static inline void gpiod_set_value_cansleep(struct gpio_desc *desc, int value)
> {
> /* GPIO can never have been requested */
> WARN_ON(1);
> }
>
> But i would say this is a gpio problem. If GPIO enabled does not care,
> GPIO disabled should also not care.

If someone is using GPIO descriptors with GPIO disabled, i.e. calling
gpiod_get() and friends, this is very likely to be a bug, and what
the driver wants to do is:

 depends on GPIOLIB

or

 select GPIOLIB

in Kconfig. The whole optional thing is mainly a leftover from when it
was possible to have a local implementation of the GPIOLIB API in
some custom header file, noone sane should be doing that anymore,
and if they do, they can very well face the warnings.

If someone is facing a lot of WARN_ON() messages to this, it is a clear
indication that they need to fix their Kconfig and in that case it is proper.

Yours,
Linus Walleij

Re: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Fabio Estevam]

Hi Andrew,

On Tue, Jul 18, 2017 at 10:32 AM, Andrew Lunn  wrote:

> http://elixir.free-electrons.com/linux/latest/source/include/linux/gpio/consumer.h#L345
> static inline void gpiod_set_value_cansleep(struct gpio_desc *desc, int value)
> {
> /* GPIO can never have been requested */
> WARN_ON(1);
> }
>
> But i would say this is a gpio problem. If GPIO enabled does not care,
> GPIO disabled should also not care.

Agreed.

Sergei, sorry for taking so long to understand your point.

> Adding Linus Walleij.

Just sent a RFC patch to linux-gpio.

Thanks

Re: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Andrew Lunn]

On Tue, Jul 18, 2017 at 09:52:51AM -0300, Fabio Estevam wrote:
> On Tue, Jul 18, 2017 at 9:48 AM, Sergei Shtylyov
>  wrote:
> > On 07/18/2017 03:39 PM, Fabio Estevam wrote:
> >
> >>>Won't this result in kernel WARNING when GPIO is disabled?
> >
> >
> >GPIO support, I was going to type...
> >
> >> Not sure if I understood your point, but gpiod_set_value_cansleep() is
> >> a no-op when the gpiod is NULL.
> >
> >
> >Look at the stub in , it has WARN_ON(1).
> 
> This patch does not alter the behavior of the driver with respect to
> GPIO being disabled, so I still do not understand your concern.

http://elixir.free-electrons.com/linux/latest/source/include/linux/gpio/consumer.h#L345
static inline void gpiod_set_value_cansleep(struct gpio_desc *desc, int value)
{
/* GPIO can never have been requested */
WARN_ON(1);
}

But i would say this is a gpio problem. If GPIO enabled does not care,
GPIO disabled should also not care.

Adding Linus Walleij.

 Andrew

Re: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Sergei Shtylyov]

On 07/18/2017 04:09 PM, Fabio Estevam wrote:


   No, it does -- devm_gpiod_get_optinal() will return NULL in that case,
bus->reset_gpio will remanin NULL, and you're removing the NULL checks
around the gpiod_set_value_cansleep() calls. Perhaps it's the problem in the
GPIO support though...


It is perfectly fine to call gpiod_set_value_cansleep() with a NULL
gpio descriptor.


   Depends on whether CONFIG_HPIOLIB is enabled or not.


Please take a look at drivers/gpio/gpiolib.c:


   If CONF(G_GPIOLIB=n, the stub from 
gpiod_set_value_cansleep() calls VALIDATE_DESC_VOID

Then if you look at the definition of VALIDATE_DESC_VOID you will see
that it does a NULL check on desc and returns immediately if it is
NULL.


   Sre, I did see that.


This means we are safe here :-)


   Sigh...

MBR, Sergei

Re: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Fabio Estevam]

On Tue, Jul 18, 2017 at 10:02 AM, Sergei Shtylyov
 wrote:

>No, it does -- devm_gpiod_get_optinal() will return NULL in that case,
> bus->reset_gpio will remanin NULL, and you're removing the NULL checks
> around the gpiod_set_value_cansleep() calls. Perhaps it's the problem in the
> GPIO support though...

It is perfectly fine to call gpiod_set_value_cansleep() with a NULL
gpio descriptor.

Please take a look at drivers/gpio/gpiolib.c:

gpiod_set_value_cansleep() calls VALIDATE_DESC_VOID

Then if you look at the definition of VALIDATE_DESC_VOID you will see
that it does a NULL check on desc and returns immediately if it is
NULL.

This means we are safe here :-)

Re: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Sergei Shtylyov]

On 07/18/2017 03:52 PM, Fabio Estevam wrote:


   Won't this result in kernel WARNING when GPIO is disabled?



   GPIO support, I was going to type...


Not sure if I understood your point, but gpiod_set_value_cansleep() is
a no-op when the gpiod is NULL.



   Look at the stub in , it has WARN_ON(1).


This patch does not alter the behavior of the driver with respect to
GPIO being disabled,


   No, it does -- devm_gpiod_get_optinal() will return NULL in that case, 
bus->reset_gpio will remanin NULL, and you're removing the NULL checks around 
the gpiod_set_value_cansleep() calls. Perhaps it's the problem in the GPIO 
support though...



so I still do not understand your concern.


   And now?

MBR, Sergei

Re: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Fabio Estevam]

On Tue, Jul 18, 2017 at 9:48 AM, Sergei Shtylyov
 wrote:
> On 07/18/2017 03:39 PM, Fabio Estevam wrote:
>
>>>Won't this result in kernel WARNING when GPIO is disabled?
>
>
>GPIO support, I was going to type...
>
>> Not sure if I understood your point, but gpiod_set_value_cansleep() is
>> a no-op when the gpiod is NULL.
>
>
>Look at the stub in , it has WARN_ON(1).

This patch does not alter the behavior of the driver with respect to
GPIO being disabled, so I still do not understand your concern.

Please be more specific.

Re: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Sergei Shtylyov]

On 07/18/2017 03:39 PM, Fabio Estevam wrote:


   Won't this result in kernel WARNING when GPIO is disabled?


   GPIO support, I was going to type...


Not sure if I understood your point, but gpiod_set_value_cansleep() is
a no-op when the gpiod is NULL.


   Look at the stub in , it has WARN_ON(1).

MBR, Sergei

Re: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Fabio Estevam]

On Tue, Jul 18, 2017 at 6:56 AM, Sergei Shtylyov
 wrote:

>Won't this result in kernel WARNING when GPIO is disabled?

Not sure if I understood your point, but gpiod_set_value_cansleep() is
a no-op when the gpiod is NULL.

Re: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Sergei Shtylyov]

Hello!

On 7/18/2017 12:09 AM, Fabio Estevam wrote:


From: Fabio Estevam 

The gpiod API checks for NULL descriptors, so there is no need to
duplicate the check in the driver.


   Won't this result in kernel WARNING when GPIO is disabled?


Signed-off-by: Fabio Estevam 

[...]

MBR, Sergei

Re: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[David Miller]

From: Fabio Estevam 
Date: Mon, 17 Jul 2017 18:09:09 -0300

> From: Fabio Estevam 
> 
> The gpiod API checks for NULL descriptors, so there is no need to
> duplicate the check in the driver.
> 
> Signed-off-by: Fabio Estevam 

Applied, thanks.

Re: [PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Andrew Lunn]

On Mon, Jul 17, 2017 at 06:09:09PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam 
> 
> The gpiod API checks for NULL descriptors, so there is no need to
> duplicate the check in the driver.
> Signed-off-by: Fabio Estevam 

VALIDATE_DESC is ugly. But:

Reviewed-by: Andrew Lunn 

Andrew

[PATCH net-next] mdio_bus: Remove unneeded gpiod NULL check

[Fabio Estevam]

From: Fabio Estevam 

The gpiod API checks for NULL descriptors, so there is no need to
duplicate the check in the driver.

Signed-off-by: Fabio Estevam 
---
 drivers/net/phy/mdio_bus.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 2df7b62c..b6f9fa6 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -399,8 +399,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module 
*owner)
}
 
/* Put PHYs in RESET to save power */
-   if (bus->reset_gpiod)
-   gpiod_set_value_cansleep(bus->reset_gpiod, 1);
+   gpiod_set_value_cansleep(bus->reset_gpiod, 1);
 
device_del(>dev);
return err;
@@ -425,8 +424,7 @@ void mdiobus_unregister(struct mii_bus *bus)
}
 
/* Put PHYs in RESET to save power */
-   if (bus->reset_gpiod)
-   gpiod_set_value_cansleep(bus->reset_gpiod, 1);
+   gpiod_set_value_cansleep(bus->reset_gpiod, 1);
 
device_del(>dev);
 }
-- 
2.7.4