[PATCH 0/5] Don't return NULL from get_phy_device() anymore

[PATCH 0/5] Don't return NULL from get_phy_device() anymore

[Sergei Shtylyov]

Hello.

   Here's the set of 5 patches against DaveM's 'net-next.git' repo. The first
patch makes get_phy_device() return only error values on error, the rest of
the patches clean up the callers of that function...

[1/5] phylib-don-t-return-NULL-from-get_phy_device.patch
[2/5] xgene-get_phy_device-doesn-t-return-NULL-anymore.patch
[3/5] fixed_phy-get_phy_device-doesn-t-return-NULL-anymore.patch
[4/5] mdio_bus-get_phy_device-doesn-t-return-NULL-anymore.patch
[5/5] of_mdio-get_phy_device-doesn-t-return-NULL-anymore.patch

MBR, Sergei

[PATCH 1/5] phylib: don't return NULL from get_phy_device()

[Sergei Shtylyov]

Arnd Bergmann asked that get_phy_device() returns either NULL or the error
value,  not both on error.  Do as he said, return ERR_PTR(-ENODEV) instead
of NULL when the PHY ID registers read as  all ones.

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/net/phy/phy_device.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: net-next/drivers/net/phy/phy_device.c
===================================================================
--- net-next.orig/drivers/net/phy/phy_device.c
+++ net-next/drivers/net/phy/phy_device.c
@@ -529,7 +529,7 @@ struct phy_device *get_phy_device(struct
 
 	/* If the phy_id is mostly Fs, there is no device there */
 	if ((phy_id & 0x1fffffff) == 0x1fffffff)
-		return NULL;
+		return ERR_PTR(-ENODEV);
 
 	return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
 }

[PATCH 2/5] xgene: get_phy_device() doesn't return NULL anymore

[Sergei Shtylyov]

Now that get_phy_device() no longer returns NULL on error, we don't need
to check for it...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/net/ethernet/apm/xgene/xgene_enet_hw.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: net-next/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
===================================================================
--- net-next.orig/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
+++ net-next/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
@@ -824,7 +824,7 @@ static int xgene_mdiobus_register(struct
 		return -EINVAL;
 
 	phy = get_phy_device(mdio, phy_id, false);
-	if (!phy || IS_ERR(phy))
+	if (IS_ERR(phy))
 		return -EIO;
 
 	ret = phy_device_register(phy);

[PATCH 3/5] fixed_phy: get_phy_device() doesn't return NULL anymore

[Sergei Shtylyov]

Now that get_phy_device() no longer returns NULL on error, we don't need
to check for it...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/net/phy/fixed_phy.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: net-next/drivers/net/phy/fixed_phy.c
===================================================================
--- net-next.orig/drivers/net/phy/fixed_phy.c
+++ net-next/drivers/net/phy/fixed_phy.c
@@ -328,7 +328,7 @@ struct phy_device *fixed_phy_register(un
 		return ERR_PTR(ret);
 
 	phy = get_phy_device(fmb->mii_bus, phy_addr, false);
-	if (!phy || IS_ERR(phy)) {
+	if (IS_ERR(phy)) {
 		fixed_phy_del(phy_addr);
 		return ERR_PTR(-EINVAL);
 	}

[PATCH 4/5] mdio_bus: get_phy_device() doesn't return NULL anymore

[Sergei Shtylyov]

Now that get_phy_device() no longer returns NULL on error, we don't need
to check for it...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/net/phy/mdio_bus.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: net-next/drivers/net/phy/mdio_bus.c
===================================================================
--- net-next.orig/drivers/net/phy/mdio_bus.c
+++ net-next/drivers/net/phy/mdio_bus.c
@@ -419,7 +419,7 @@ struct phy_device *mdiobus_scan(struct m
 	int err;
 
 	phydev = get_phy_device(bus, addr, false);
-	if (IS_ERR(phydev) || phydev == NULL)
+	if (IS_ERR(phydev))
 		return phydev;
 
 	/*

[PATCH 5/5] of_mdio: get_phy_device() doesn't return NULL anymore

[Sergei Shtylyov]

Now that get_phy_device() no longer returns NULL on error, we don't need
to check for it...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/of/of_mdio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: net-next/drivers/of/of_mdio.c
===================================================================
--- net-next.orig/drivers/of/of_mdio.c
+++ net-next/drivers/of/of_mdio.c
@@ -56,7 +56,7 @@ static void of_mdiobus_register_phy(stru
 		phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
 	else
 		phy = get_phy_device(mdio, addr, is_c45);
-	if (IS_ERR_OR_NULL(phy))
+	if (IS_ERR(phy))
 		return;
 
 	rc = irq_of_parse_and_map(child, 0);

Re: [PATCH 5/5] of_mdio: get_phy_device() doesn't return NULL anymore

[Sergei Shtylyov]

On 04/24/2016 08:31 PM, Sergei Shtylyov wrote:

> Now that get_phy_device() no longer returns NULL on error, we don't need
> to check for it...
>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>

    Oops, forgot to CC: the DT people, doing that now...

> ---
>   drivers/of/of_mdio.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: net-next/drivers/of/of_mdio.c
> ===================================================================
> --- net-next.orig/drivers/of/of_mdio.c
> +++ net-next/drivers/of/of_mdio.c
> @@ -56,7 +56,7 @@ static void of_mdiobus_register_phy(stru
>   		phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
>   	else
>   		phy = get_phy_device(mdio, addr, is_c45);
> -	if (IS_ERR_OR_NULL(phy))
> +	if (IS_ERR(phy))
>   		return;
>
>   	rc = irq_of_parse_and_map(child, 0);

Re: [PATCH 1/5] phylib: don't return NULL from get_phy_device()

[Florian Fainelli]

On 24/04/16 10:25, Sergei Shtylyov wrote:
> Arnd Bergmann asked that get_phy_device() returns either NULL or the error
> value,  not both on error.  Do as he said, return ERR_PTR(-ENODEV) instead
> of NULL when the PHY ID registers read as  all ones.
> 
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH 2/5] xgene: get_phy_device() doesn't return NULL anymore

[Florian Fainelli]

On 24/04/16 10:27, Sergei Shtylyov wrote:
> Now that get_phy_device() no longer returns NULL on error, we don't need
> to check for it...
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH 3/5] fixed_phy: get_phy_device() doesn't return NULL anymore

[Florian Fainelli]

On 24/04/16 10:29, Sergei Shtylyov wrote:
> Now that get_phy_device() no longer returns NULL on error, we don't need
> to check for it...
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH 4/5] mdio_bus: get_phy_device() doesn't return NULL anymore

[Florian Fainelli]

On 24/04/16 10:30, Sergei Shtylyov wrote:
> Now that get_phy_device() no longer returns NULL on error, we don't need
> to check for it...
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH 5/5] of_mdio: get_phy_device() doesn't return NULL anymore

[Florian Fainelli]

On 24/04/16 10:31, Sergei Shtylyov wrote:
> Now that get_phy_device() no longer returns NULL on error, we don't need
> to check for it...
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH 0/5] Don't return NULL from get_phy_device() anymore

[David Miller]

From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Sun, 24 Apr 2016 20:23:03 +0300

>    Here's the set of 5 patches against DaveM's 'net-next.git' repo. The first
> patch makes get_phy_device() return only error values on error, the rest of
> the patches clean up the callers of that function...

Series applied, thanks.

Re: [PATCH 1/5] phylib: don't return NULL from get_phy_device()

[Andrew Lunn]

On Wed, Apr 27, 2016 at 03:30:57PM -0400, Vivien Didelot wrote:
> Hi David, All,
> 
> Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes:
> 
> > Arnd Bergmann asked that get_phy_device() returns either NULL or the error
> > value,  not both on error.  Do as he said, return ERR_PTR(-ENODEV) instead
> > of NULL when the PHY ID registers read as  all ones.
> >
> > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >
> > ---
> >  drivers/net/phy/phy_device.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > Index: net-next/drivers/net/phy/phy_device.c
> > ===================================================================
> > --- net-next.orig/drivers/net/phy/phy_device.c
> > +++ net-next/drivers/net/phy/phy_device.c
> > @@ -529,7 +529,7 @@ struct phy_device *get_phy_device(struct
> >  
> >  	/* If the phy_id is mostly Fs, there is no device there */
> >  	if ((phy_id & 0x1fffffff) == 0x1fffffff)
> > -		return NULL;
> > +		return ERR_PTR(-ENODEV);
> >  
> >  	return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
> >  }

This change is wrong, it needs reverting, or the call sights need
fixing to expect ENODEV.

The point is, the device not being there is not an error, with respect
to the code calling this function.

It gets called by mdiobus_scan()

struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr)
{
        struct phy_device *phydev;
        int err;

        phydev = get_phy_device(bus, addr, false);
        if (IS_ERR(phydev) || phydev == NULL)
                return phydev;

So before, we return NULL, if the device was not there. Now we return
ERR_PTR(-ENODEV).

This is being called by:

int __mdiobus_register(struct mii_bus *bus, struct module *owner)
{
        struct mdio_device *mdiodev;
...
        for (i = 0; i < PHY_MAX_ADDR; i++) {
                if ((bus->phy_mask & (1 << i)) == 0) {
                        struct phy_device *phydev;

                        phydev = mdiobus_scan(bus, i);
                        if (IS_ERR(phydev)) {
                                err = PTR_ERR(phydev);
                                goto error;
                        }
                }
        }

This is treating ERR_PTR(-ENODEV) as a fatal error, where as before
IS_ERR(NULL) would be false and it would continue scanning other
addresses on the bus.

Please revert this, or fix all the callsites such that ENODEV is not a
fatal error.

	     Andrew

Re: [PATCH 1/5] phylib: don't return NULL from get_phy_device()

[Sergei Shtylyov]

Hello.

On 04/27/2016 10:49 PM, Andrew Lunn wrote:

>> Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes:
>>
>>> Arnd Bergmann asked that get_phy_device() returns either NULL or the error
>>> value,  not both on error.  Do as he said, return ERR_PTR(-ENODEV) instead
>>> of NULL when the PHY ID registers read as  all ones.
>>>
>>> Suggested-by: Arnd Bergmann <arnd@arndb.de>
>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>
>>> ---
>>>   drivers/net/phy/phy_device.c |    2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> Index: net-next/drivers/net/phy/phy_device.c
>>> ===================================================================
>>> --- net-next.orig/drivers/net/phy/phy_device.c
>>> +++ net-next/drivers/net/phy/phy_device.c
>>> @@ -529,7 +529,7 @@ struct phy_device *get_phy_device(struct
>>>
>>>   	/* If the phy_id is mostly Fs, there is no device there */
>>>   	if ((phy_id & 0x1fffffff) == 0x1fffffff)
>>> -		return NULL;
>>> +		return ERR_PTR(-ENODEV);
>>>
>>>   	return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
>>>   }
>
> This change is wrong, it needs reverting, or the call sights need
> fixing to expect ENODEV.

    So this function had a good reason to return NULL, as it turned out... :-(

> The point is, the device not being there is not an error, with respect
> to the code calling this function.
>
> It gets called by mdiobus_scan()
>
> struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr)
> {
>          struct phy_device *phydev;
>          int err;
>
>          phydev = get_phy_device(bus, addr, false);
>          if (IS_ERR(phydev) || phydev == NULL)
>                  return phydev;
>
> So before, we return NULL, if the device was not there. Now we return
> ERR_PTR(-ENODEV).
>
> This is being called by:
>
> int __mdiobus_register(struct mii_bus *bus, struct module *owner)
> {
>          struct mdio_device *mdiodev;
> ...
>          for (i = 0; i < PHY_MAX_ADDR; i++) {
>                  if ((bus->phy_mask & (1 << i)) == 0) {
>                          struct phy_device *phydev;
>
>                          phydev = mdiobus_scan(bus, i);
>                          if (IS_ERR(phydev)) {
>                                  err = PTR_ERR(phydev);
>                                  goto error;
>                          }
>                  }
>          }
>
> This is treating ERR_PTR(-ENODEV) as a fatal error, where as before
> IS_ERR(NULL) would be false and it would continue scanning other
> addresses on the bus.

    Thank you for the detailed analysis! (And shame on me for the lack of it.)

> Please revert this, or fix all the callsites such that ENODEV is not a
> fatal error.

    OK, I'll do what DaveM decides.

> 	     Andrew

MBR, Sergei

Re: [PATCH 1/5] phylib: don't return NULL from get_phy_device()

[Florian Fainelli]

On 27/04/16 13:12, Sergei Shtylyov wrote:
> Hello.
> 
> On 04/27/2016 10:30 PM, Vivien Didelot wrote:
> 
>>> Arnd Bergmann asked that get_phy_device() returns either NULL or the
>>> error
>>> value,  not both on error.  Do as he said, return ERR_PTR(-ENODEV)
>>> instead
>>> of NULL when the PHY ID registers read as  all ones.
>>>
>>> Suggested-by: Arnd Bergmann <arnd@arndb.de>
>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>
>>> ---
>>>   drivers/net/phy/phy_device.c |    2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> Index: net-next/drivers/net/phy/phy_device.c
>>> ===================================================================
>>> --- net-next.orig/drivers/net/phy/phy_device.c
>>> +++ net-next/drivers/net/phy/phy_device.c
>>> @@ -529,7 +529,7 @@ struct phy_device *get_phy_device(struct
>>>
>>>       /* If the phy_id is mostly Fs, there is no device there */
>>>       if ((phy_id & 0x1fffffff) == 0x1fffffff)
>>> -        return NULL;
>>> +        return ERR_PTR(-ENODEV);
>>>
>>>       return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
>>>   }
>>
>> This particular commit, merged as:
>>
>>      b74766a0a0fe ("phylib: don't return NULL from get_phy_device()")
>>
>> breaks my 3-switch DSA setup with the following error:
>>
>>      fec: probe of 400d1000.ethernet failed with error -22
>>
>> Reverting c971c0e580a6 ("Merge branch 'get_phy_device-retval'") restores
>> a working setup.
> 
>    I think I was able to follow this to the get_phy_device() call in
> fixed_phy_register() but I'm unable to see why it fails now and didn't
> before. Are you using fixed_phy.c at all?

I am using fixed_phy.c and yes, this fails for me too, sorry for not
catching that when you posted the patches :/

-- 
Florian

Re: [PATCH 1/5] phylib: don't return NULL from get_phy_device()

[Sergei Shtylyov]

Hello.

On 04/27/2016 10:30 PM, Vivien Didelot wrote:

>> Arnd Bergmann asked that get_phy_device() returns either NULL or the error
>> value,  not both on error.  Do as he said, return ERR_PTR(-ENODEV) instead
>> of NULL when the PHY ID registers read as  all ones.
>>
>> Suggested-by: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> ---
>>   drivers/net/phy/phy_device.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Index: net-next/drivers/net/phy/phy_device.c
>> ===================================================================
>> --- net-next.orig/drivers/net/phy/phy_device.c
>> +++ net-next/drivers/net/phy/phy_device.c
>> @@ -529,7 +529,7 @@ struct phy_device *get_phy_device(struct
>>
>>   	/* If the phy_id is mostly Fs, there is no device there */
>>   	if ((phy_id & 0x1fffffff) == 0x1fffffff)
>> -		return NULL;
>> +		return ERR_PTR(-ENODEV);
>>
>>   	return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
>>   }
>
> This particular commit, merged as:
>
>      b74766a0a0fe ("phylib: don't return NULL from get_phy_device()")
>
> breaks my 3-switch DSA setup with the following error:
>
>      fec: probe of 400d1000.ethernet failed with error -22
>
> Reverting c971c0e580a6 ("Merge branch 'get_phy_device-retval'") restores
> a working setup.

    I think I was able to follow this to the get_phy_device() call in 
fixed_phy_register() but I'm unable to see why it fails now and didn't before. 
Are you using fixed_phy.c at all?

> Thanks,
>
>          Vivien

MBR, Sergei

Re: [PATCH 1/5] phylib: don't return NULL from get_phy_device()

[David Miller]

From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Wed, 27 Apr 2016 23:09:37 +0300

> On 04/27/2016 10:49 PM, Andrew Lunn wrote:
> 
>> Please revert this, or fix all the callsites such that ENODEV is not a
>> fatal error.
> 
>    OK, I'll do what DaveM decides.

If you feel confident getting all the ENODEV checks right, please just do
that.

Thanks.

Re: [PATCH 1/5] phylib: don't return NULL from get_phy_device()

[Florian Fainelli]

On 27/04/16 12:49, Andrew Lunn wrote:
> On Wed, Apr 27, 2016 at 03:30:57PM -0400, Vivien Didelot wrote:
>> Hi David, All,
>>
>> Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes:
>>
>>> Arnd Bergmann asked that get_phy_device() returns either NULL or the error
>>> value,  not both on error.  Do as he said, return ERR_PTR(-ENODEV) instead
>>> of NULL when the PHY ID registers read as  all ones.
>>>
>>> Suggested-by: Arnd Bergmann <arnd@arndb.de>
>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>
>>> ---
>>>  drivers/net/phy/phy_device.c |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> Index: net-next/drivers/net/phy/phy_device.c
>>> ===================================================================
>>> --- net-next.orig/drivers/net/phy/phy_device.c
>>> +++ net-next/drivers/net/phy/phy_device.c
>>> @@ -529,7 +529,7 @@ struct phy_device *get_phy_device(struct
>>>  
>>>  	/* If the phy_id is mostly Fs, there is no device there */
>>>  	if ((phy_id & 0x1fffffff) == 0x1fffffff)
>>> -		return NULL;
>>> +		return ERR_PTR(-ENODEV);
>>>  
>>>  	return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
>>>  }
> 
> This change is wrong, it needs reverting, or the call sights need
> fixing to expect ENODEV.
> 
> The point is, the device not being there is not an error, with respect
> to the code calling this function.
> 
> It gets called by mdiobus_scan()
> 
> struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr)
> {
>         struct phy_device *phydev;
>         int err;
> 
>         phydev = get_phy_device(bus, addr, false);
>         if (IS_ERR(phydev) || phydev == NULL)
>                 return phydev;
> 
> So before, we return NULL, if the device was not there. Now we return
> ERR_PTR(-ENODEV).
> 
> This is being called by:
> 
> int __mdiobus_register(struct mii_bus *bus, struct module *owner)
> {
>         struct mdio_device *mdiodev;
> ...
>         for (i = 0; i < PHY_MAX_ADDR; i++) {
>                 if ((bus->phy_mask & (1 << i)) == 0) {
>                         struct phy_device *phydev;
> 
>                         phydev = mdiobus_scan(bus, i);
>                         if (IS_ERR(phydev)) {
>                                 err = PTR_ERR(phydev);
>                                 goto error;
>                         }
>                 }
>         }
> 
> This is treating ERR_PTR(-ENODEV) as a fatal error, where as before
> IS_ERR(NULL) would be false and it would continue scanning other
> addresses on the bus.
> 
> Please revert this, or fix all the callsites such that ENODEV is not a
> fatal error.

So the one you pointed out in __mdiobus_register() is definitively
needed, though I did get a different issue than Vivien's (-EBUSY vs.
-EINVAL). The get_phy_device() in drivers/of/of_mdio.c probably needs
something similar too, here is what I locally have for the moment:

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 499003ee8055..94a27b028dd8 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -333,7 +333,7 @@ int __mdiobus_register(struct mii_bus *bus, struct
module *owner)
                        struct phy_device *phydev;

                        phydev = mdiobus_scan(bus, i);
-                       if (IS_ERR(phydev)) {
+                       if (IS_ERR(phydev) && PTR_ERR(phydev) != -ENODEV) {
                                err = PTR_ERR(phydev);
                                goto error;
                        }

-- 
Florian

Re: [PATCH 1/5] phylib: don't return NULL from get_phy_device()

[Arnd Bergmann]

On Wednesday 27 April 2016 23:09:37 Sergei Shtylyov wrote:
> Hello.
> 
> On 04/27/2016 10:49 PM, Andrew Lunn wrote:
> 
> >> Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes:
> >>
> >>> Arnd Bergmann asked that get_phy_device() returns either NULL or the error
> >>> value,  not both on error.  Do as he said, return ERR_PTR(-ENODEV) instead
> >>> of NULL when the PHY ID registers read as  all ones.
> >>>
> >>> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> >>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >>>
> >>> ---
> >>>   drivers/net/phy/phy_device.c |    2 +-
> >>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> Index: net-next/drivers/net/phy/phy_device.c
> >>> ===================================================================
> >>> --- net-next.orig/drivers/net/phy/phy_device.c
> >>> +++ net-next/drivers/net/phy/phy_device.c
> >>> @@ -529,7 +529,7 @@ struct phy_device *get_phy_device(struct
> >>>
> >>>   	/* If the phy_id is mostly Fs, there is no device there */
> >>>   	if ((phy_id & 0x1fffffff) == 0x1fffffff)
> >>> -		return NULL;
> >>> +		return ERR_PTR(-ENODEV);
> >>>
> >>>   	return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
> >>>   }
> >
> > This change is wrong, it needs reverting, or the call sights need
> > fixing to expect ENODEV.
> 
>     So this function had a good reason to return NULL, as it turned out... :-(
> 
> > The point is, the device not being there is not an error, with respect
> > to the code calling this function.
> >
> > It gets called by mdiobus_scan()
> >
> > struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr)
> > {
> >          struct phy_device *phydev;
> >          int err;
> >
> >          phydev = get_phy_device(bus, addr, false);
> >          if (IS_ERR(phydev) || phydev == NULL)
> >                  return phydev;
> >
> > So before, we return NULL, if the device was not there. Now we return
> > ERR_PTR(-ENODEV).
> >
> > This is being called by:
> >
> > int __mdiobus_register(struct mii_bus *bus, struct module *owner)
> > {
> >          struct mdio_device *mdiodev;
> > ...
> >          for (i = 0; i < PHY_MAX_ADDR; i++) {
> >                  if ((bus->phy_mask & (1 << i)) == 0) {
> >                          struct phy_device *phydev;
> >
> >                          phydev = mdiobus_scan(bus, i);
> >                          if (IS_ERR(phydev)) {
> >                                  err = PTR_ERR(phydev);
> >                                  goto error;
> >                          }
> >                  }
> >          }
> >
> > This is treating ERR_PTR(-ENODEV) as a fatal error, where as before
> > IS_ERR(NULL) would be false and it would continue scanning other
> > addresses on the bus.
> 
>     Thank you for the detailed analysis! (And shame on me for the lack of it.)
> 
> > Please revert this, or fix all the callsites such that ENODEV is not a
> > fatal error.
> 
>     OK, I'll do what DaveM decides.

I found one other user that remains broken: pxa168_init_phy() looks 
wrong before and after the patch:

        pep->phy = mdiobus_scan(pep->smi_bus, pep->phy_addr);
        if (!pep->phy)
                return -ENODEV;

        err = phy_connect_direct(dev, pep->phy, pxa168_eth_adjust_link,
                                 pep->phy_intf);

as phy_connect_direct() will go on and dereference an error pointer. This
should check for IS_ERR(), and with the patches applied, we can drop the
!pep->phy check.

	Arnd

Re: [PATCH 1/5] phylib: don't return NULL from get_phy_device()

[Arnd Bergmann]

On Wednesday 27 April 2016 14:47:29 Florian Fainelli wrote:
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 499003ee8055..94a27b028dd8 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -333,7 +333,7 @@ int __mdiobus_register(struct mii_bus *bus, struct
> module *owner)
>                         struct phy_device *phydev;
> 
>                         phydev = mdiobus_scan(bus, i);
> -                       if (IS_ERR(phydev)) {
> +                       if (IS_ERR(phydev) && PTR_ERR(phydev) != -ENODEV) {
>                                 err = PTR_ERR(phydev);
>                                 goto error;
>                         }
> 
> 

I think that is an improvement over the original code, and better than
reverting the series. Out of the three callers of mdiobus_scan, I already
commented on drivers/net/ethernet/marvell/pxa168_eth.c being wrong to
start with, and drivers/net/ethernet/cadence/macb.c seems to require
the same fix that you did here for mdio_bus.c

	Arnd