Re: [PATCH net-next v2 1/4] net: phy: fixed_phy: Fix return value check for fixed_phy_get_gpiod

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Thu, Aug 17, 2023 at 08:16:28PM +0800, Ruan Jinjie wrote:
> Since fixed_phy_get_gpiod() return NULL instead of ERR_PTR(),
> if it fails, the IS_ERR() can never return the error. So check NULL
> and return ERR_PTR(-EINVAL) if fails.

No, this is totally and utterly wrong, and this patch introduces a new
bug. The original code is _correct_.

> Fixes: 71bd106d2567 ("net: fixed-phy: Add fixed_phy_register_with_gpiod() API")
> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
> ---
>  drivers/net/phy/fixed_phy.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
> index aef739c20ac4..4e7406455b6e 100644
> --- a/drivers/net/phy/fixed_phy.c
> +++ b/drivers/net/phy/fixed_phy.c
> @@ -239,8 +239,8 @@ static struct phy_device *__fixed_phy_register(unsigned int irq,
>  	/* Check if we have a GPIO associated with this fixed phy */
>  	if (!gpiod) {
>  		gpiod = fixed_phy_get_gpiod(np);
> -		if (IS_ERR(gpiod))
> -			return ERR_CAST(gpiod);
> +		if (!gpiod)
> +			return ERR_PTR(-EINVAL);

Let's look at fixed_phy_get_gpiod():

        gpiod = fwnode_gpiod_get_index(of_fwnode_handle(fixed_link_node),
                                       "link", 0, GPIOD_IN, "mdio");
        if (IS_ERR(gpiod) && PTR_ERR(gpiod) != -EPROBE_DEFER) {
...
		gpiod = NULL;
	}
...
	return gpiod;

If fwnode_gpiod_get_index() returns -EPROBE_DEFER, _then_ we return an
error pointer. So it _does_ return an error pointer.

It also returns NULL when there is no device node passed to it, or
if there is no fixed-link specifier, or there is some other error
from fwnode_gpiod_get_index().

Otherwise, it returns a valid pointer to a gpio descriptor.

The gpio is optional. The device node is optional. When
fixed_phy_get_gpiod() returns NULL, it is _not_ an error, it means
that we don't have a GPIO. Just because something returns NULL does
_not_ mean it's an error - please get out of that thinking, because
if you don't your patches will introduce lots of new bugs.

Only when fwnode_gpiod_get_index() wants to defer probe do we return
an error.

So, sorry but NAK to this patch, it is incorrect.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!