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.shtyl...@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 <a...@arndb.de>
> > Signed-off-by: Sergei Shtylyov <sergei.shtyl...@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