[PATCH net-next v2 0/4] net: Fix return value check for fixed_phy_register()

[PATCH net-next v2 0/4] net: Fix return value check for fixed_phy_register()

[Ruan Jinjie]

Since fixed_phy_get_gpiod() return NULL instead of
ERR_PTR(), the IS_ERR() check is not correct to return the err.

The fixed_phy_register() function returns error pointers and never
returns NULL.

And fixed_phy_register() function also returns -EPROBE_DEFER, -EINVAL and
-EBUSY, etc, in addition to -ENODEV or -EIO, Use PTR_ERR instead.

Changes in v2:
- Remove redundant NULL check.
- Fix the return value for fixed_phy_register().
- Fix the return value check for fixed_phy_get_gpiod().
- Fix the return value also for lan743x.

Ruan Jinjie (4):
  net: phy: fixed_phy: Fix return value check for fixed_phy_get_gpiod
  net: bgmac: Fix return value check for fixed_phy_register()
  net: bcmgenet: Fix return value check for fixed_phy_register()
  net: lan743x: Fix return value check for fixed_phy_register()

 drivers/net/ethernet/broadcom/bgmac.c         | 4 ++--
 drivers/net/ethernet/broadcom/genet/bcmmii.c  | 4 ++--
 drivers/net/ethernet/microchip/lan743x_main.c | 2 +-
 drivers/net/phy/fixed_phy.c                   | 4 ++--
 4 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.34.1

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

[Ruan Jinjie]

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.

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);
 	}
 
 	/* Get the next available PHY address, up to PHY_MAX_ADDR */
-- 
2.34.1

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

[Andrew Lunn]

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.
> 
> Fixes: 71bd106d2567 ("net: fixed-phy: Add fixed_phy_register_with_gpiod() API")
> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Russell King (Oracle)]

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!

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

[Ruan Jinjie]

On 2023/8/17 21:10, Russell King (Oracle) wrote:
> 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.

Thank you, I understand what you mean, NULL is not an error here, so it
is not handled.

> 
> 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.
> 

[PATCH net-next v2 2/4] net: bgmac: Fix return value check for fixed_phy_register()

[Ruan Jinjie]

The fixed_phy_register() function returns error pointers and never
returns NULL. Update the checks accordingly.

And it also returns -EPROBE_DEFER, -EINVAL and -EBUSY, etc, in addition to
-ENODEV, just return -ENODEV is not sensible, use
PTR_ERR to fix the issue.

Fixes: c25b23b8a387 ("bgmac: register fixed PHY for ARM BCM470X / BCM5301X chipsets")
Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
---
v2:
- Remove redundant NULL check and fix the return value.
- Update the commit title and message.
- Add the fix tag.
---
 drivers/net/ethernet/broadcom/bgmac.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 10c7c232cc4e..448a1b90de5e 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1448,9 +1448,9 @@ int bgmac_phy_connect_direct(struct bgmac *bgmac)
 	int err;
 
 	phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
-	if (!phy_dev || IS_ERR(phy_dev)) {
+	if (IS_ERR(phy_dev)) {
 		dev_err(bgmac->dev, "Failed to register fixed PHY device\n");
-		return -ENODEV;
+		return PTR_ERR(phy_dev);
 	}
 
 	err = phy_connect_direct(bgmac->net_dev, phy_dev, bgmac_adjust_link,
-- 
2.34.1

Re: [PATCH net-next v2 2/4] net: bgmac: Fix return value check for fixed_phy_register()

[Andrew Lunn]

On Thu, Aug 17, 2023 at 08:16:29PM +0800, Ruan Jinjie wrote:
> The fixed_phy_register() function returns error pointers and never
> returns NULL. Update the checks accordingly.
> 
> And it also returns -EPROBE_DEFER, -EINVAL and -EBUSY, etc, in addition to
> -ENODEV, just return -ENODEV is not sensible, use
> PTR_ERR to fix the issue.

I would recommend changing not sensible to best practice, as i
suggested in one of your other patches.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

---
pw-bot: cr

Re: [PATCH net-next v2 2/4] net: bgmac: Fix return value check for fixed_phy_register()

[Ruan Jinjie]

On 2023/8/17 20:41, Andrew Lunn wrote:
> On Thu, Aug 17, 2023 at 08:16:29PM +0800, Ruan Jinjie wrote:
>> The fixed_phy_register() function returns error pointers and never
>> returns NULL. Update the checks accordingly.
>>
>> And it also returns -EPROBE_DEFER, -EINVAL and -EBUSY, etc, in addition to
>> -ENODEV, just return -ENODEV is not sensible, use
>> PTR_ERR to fix the issue.
> 
> I would recommend changing not sensible to best practice, as i
> suggested in one of your other patches.

Thank you again! I'll watch the wording next time.

> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> ---
> pw-bot: cr

[PATCH net-next v2 3/4] net: bcmgenet: Fix return value check for fixed_phy_register()

[Ruan Jinjie]

The fixed_phy_register() function returns error pointers and never
returns NULL. Update the checks accordingly.

And it also returns -EPROBE_DEFER, -EINVAL and -EBUSY, etc, in addition to
-ENODEV, just return -ENODEV is not sensible, use PTR_ERR to
fix the issue.

Fixes: b0ba512e25d7 ("net: bcmgenet: enable driver to work without a device tree")
Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
---
v2:
- Remove redundant NULL check and fix the return value.
- Update the commit title and message.
- Add the fix tag.
---
 drivers/net/ethernet/broadcom/genet/bcmmii.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 0092e46c46f8..97ea76d443ab 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -617,9 +617,9 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
 		};
 
 		phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
-		if (!phydev || IS_ERR(phydev)) {
+		if (IS_ERR(phydev)) {
 			dev_err(kdev, "failed to register fixed PHY device\n");
-			return -ENODEV;
+			return PTR_ERR(phydev);
 		}
 
 		/* Make sure we initialize MoCA PHYs with a link down */
-- 
2.34.1

Re: [PATCH net-next v2 3/4] net: bcmgenet: Fix return value check for fixed_phy_register()

[Heiner Kallweit]

On 17.08.2023 14:16, Ruan Jinjie wrote:
> The fixed_phy_register() function returns error pointers and never
> returns NULL. Update the checks accordingly.
> 
> And it also returns -EPROBE_DEFER, -EINVAL and -EBUSY, etc, in addition to
> -ENODEV, just return -ENODEV is not sensible, use PTR_ERR to
> fix the issue.
> 
It's right that by returning -ENODEV detail information about the
error cause is lost. However callers may rely on the function to
return -ENODEV in case of an error. Did you check for this?
Even if yes: This second part of the patch is an improvement,
and therefore should be a separate patch.

> Fixes: b0ba512e25d7 ("net: bcmgenet: enable driver to work without a device tree")
> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
> ---
> v2:
> - Remove redundant NULL check and fix the return value.
> - Update the commit title and message.
> - Add the fix tag.
> ---
>  drivers/net/ethernet/broadcom/genet/bcmmii.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> index 0092e46c46f8..97ea76d443ab 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> @@ -617,9 +617,9 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
>  		};
>  
>  		phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
> -		if (!phydev || IS_ERR(phydev)) {
> +		if (IS_ERR(phydev)) {
>  			dev_err(kdev, "failed to register fixed PHY device\n");
> -			return -ENODEV;
> +			return PTR_ERR(phydev);
>  		}
>  
>  		/* Make sure we initialize MoCA PHYs with a link down */

Re: [PATCH net-next v2 3/4] net: bcmgenet: Fix return value check for fixed_phy_register()

[Ruan Jinjie]

On 2023/8/17 20:36, Heiner Kallweit wrote:
> On 17.08.2023 14:16, Ruan Jinjie wrote:
>> The fixed_phy_register() function returns error pointers and never
>> returns NULL. Update the checks accordingly.
>>
>> And it also returns -EPROBE_DEFER, -EINVAL and -EBUSY, etc, in addition to
>> -ENODEV, just return -ENODEV is not sensible, use PTR_ERR to
>> fix the issue.
>>
> It's right that by returning -ENODEV detail information about the
> error cause is lost. However callers may rely on the function to
> return -ENODEV in case of an error. Did you check for this?

I have checked it again, there is no rely on the function to -ENODEV in
case of an error.

> Even if yes: This second part of the patch is an improvement,
> and therefore should be a separate patch.

Thank you! I'll split the two parts into 2 patches.

> 
>> Fixes: b0ba512e25d7 ("net: bcmgenet: enable driver to work without a device tree")
>> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
>> ---
>> v2:
>> - Remove redundant NULL check and fix the return value.
>> - Update the commit title and message.
>> - Add the fix tag.
>> ---
>>  drivers/net/ethernet/broadcom/genet/bcmmii.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> index 0092e46c46f8..97ea76d443ab 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> @@ -617,9 +617,9 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
>>  		};
>>  
>>  		phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
>> -		if (!phydev || IS_ERR(phydev)) {
>> +		if (IS_ERR(phydev)) {
>>  			dev_err(kdev, "failed to register fixed PHY device\n");
>> -			return -ENODEV;
>> +			return PTR_ERR(phydev);
>>  		}
>>  
>>  		/* Make sure we initialize MoCA PHYs with a link down */
> 

[PATCH net-next v2 4/4] net: lan743x: Fix return value check for fixed_phy_register()

[Ruan Jinjie]

fixed_phy_register() function returns -EPROBE_DEFER, -EINVAL and -EBUSY,
etc, but not return -EIO. use PTR_ERR to fix the issue.

Fixes: 624864fbff92 ("net: lan743x: add fixed phy support for LAN7431 device")
Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
---
 drivers/net/ethernet/microchip/lan743x_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index a36f6369f132..c81cdeb4d4e7 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -1515,7 +1515,7 @@ static int lan743x_phy_open(struct lan743x_adapter *adapter)
 							    &fphy_status, NULL);
 				if (IS_ERR(phydev)) {
 					netdev_err(netdev, "No PHY/fixed_PHY found\n");
-					return -EIO;
+					return PTR_ERR(phydev);
 				}
 			} else {
 				goto return_error;
-- 
2.34.1

Re: [PATCH net-next v2 4/4] net: lan743x: Fix return value check for fixed_phy_register()

[Heiner Kallweit]

On 17.08.2023 14:16, Ruan Jinjie wrote:
> fixed_phy_register() function returns -EPROBE_DEFER, -EINVAL and -EBUSY,
> etc, but not return -EIO. use PTR_ERR to fix the issue.
> 
> Fixes: 624864fbff92 ("net: lan743x: add fixed phy support for LAN7431 device")

This isn't a fix. Returning -EIO isn't wrong. Returning the original errno values
may be better, but that's an improvement.
Also combining "net-next" with a Fixes tag is wrong, except the functionality
was added very recently.

> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
> ---
>  drivers/net/ethernet/microchip/lan743x_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index a36f6369f132..c81cdeb4d4e7 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -1515,7 +1515,7 @@ static int lan743x_phy_open(struct lan743x_adapter *adapter)
>  							    &fphy_status, NULL);
>  				if (IS_ERR(phydev)) {
>  					netdev_err(netdev, "No PHY/fixed_PHY found\n");
> -					return -EIO;
> +					return PTR_ERR(phydev);
>  				}
>  			} else {
>  				goto return_error;

Re: [PATCH net-next v2 4/4] net: lan743x: Fix return value check for fixed_phy_register()

[Ruan Jinjie]

On 2023/8/17 20:43, Heiner Kallweit wrote:
> On 17.08.2023 14:16, Ruan Jinjie wrote:
>> fixed_phy_register() function returns -EPROBE_DEFER, -EINVAL and -EBUSY,
>> etc, but not return -EIO. use PTR_ERR to fix the issue.
>>
>> Fixes: 624864fbff92 ("net: lan743x: add fixed phy support for LAN7431 device")
> 
> This isn't a fix. Returning -EIO isn't wrong. Returning the original errno values
> may be better, but that's an improvement.

Sorry! I'll remove this fix tag in the next version.

> Also combining "net-next" with a Fixes tag is wrong, except the functionality
> was added very recently.

Thank you! I'll keep an eye on these in the future.

> 
>> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
>> ---
>>  drivers/net/ethernet/microchip/lan743x_main.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
>> index a36f6369f132..c81cdeb4d4e7 100644
>> --- a/drivers/net/ethernet/microchip/lan743x_main.c
>> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
>> @@ -1515,7 +1515,7 @@ static int lan743x_phy_open(struct lan743x_adapter *adapter)
>>  							    &fphy_status, NULL);
>>  				if (IS_ERR(phydev)) {
>>  					netdev_err(netdev, "No PHY/fixed_PHY found\n");
>> -					return -EIO;
>> +					return PTR_ERR(phydev);
>>  				}
>>  			} else {
>>  				goto return_error;
>