[patch] of: check for IS_ERR()

[patch] of: check for IS_ERR()

[Dan Carpenter]

get_phy_device() can return an ERR_PTR()

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
I don't have a cross compile environment set up so I can't even compile 
test this.  :/  But err.h is included so it should be OK.

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 18ecae4..b474833 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -69,7 +69,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 		}
 
 		phy = get_phy_device(mdio, be32_to_cpup(addr));
-		if (!phy) {
+		if (!phy || IS_ERR(phy)) {
 			dev_err(&mdio->dev, "error probing PHY at address %i\n",
 				*addr);
 			continue;

Re: [patch] of: check for IS_ERR()

[David Miller]

From: Dan Carpenter <error27@gmail.com>
Date: Fri, 26 Feb 2010 12:49:41 +0300

> get_phy_device() can return an ERR_PTR()
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> I don't have a cross compile environment set up so I can't even compile 
> test this.  :/  But err.h is included so it should be OK.

It should return ERR_PTR() consistently.  Checking for both
NULL and ERR_PTR() is undesirable.

Re: [patch] of: check for IS_ERR()

[Grant Likely]

On Fri, Feb 26, 2010 at 2:54 AM, David Miller <davem@davemloft.net> wrote:
> From: Dan Carpenter <error27@gmail.com>
> Date: Fri, 26 Feb 2010 12:49:41 +0300
>
>> get_phy_device() can return an ERR_PTR()
>>
>> Signed-off-by: Dan Carpenter <error27@gmail.com>
>> ---
>> I don't have a cross compile environment set up so I can't even compile
>> test this.  :/  But err.h is included so it should be OK.
>
> It should return ERR_PTR() consistently.  Checking for both
> NULL and ERR_PTR() is undesirable.

Ugh.  This is why I dislike the ERR_PTR() pattern so much.  The
compiler cannot do any type checking and it is implemented
inconsistently.  You have to go look at the calling function to find
out what you're allowed to do with the return value.  ie. which test
do I use? (!ptr) or IS_ERR(ptr)?

It would be better if ERR_PTR() returned a structure or a union.  At
least that way the compiler would yell at you if the an ERR_PTR was
being returned.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [patch] of: check for IS_ERR()

[Dan Carpenter]

On Fri, Feb 26, 2010 at 01:54:20AM -0800, David Miller wrote:
> From: Dan Carpenter <error27@gmail.com>
> Date: Fri, 26 Feb 2010 12:49:41 +0300
> 
> > get_phy_device() can return an ERR_PTR()
> > 
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > ---
> > I don't have a cross compile environment set up so I can't even compile 
> > test this.  :/  But err.h is included so it should be OK.
> 
> It should return ERR_PTR() consistently.  Checking for both
> NULL and ERR_PTR() is undesirable.

Hi Giulio,

get_phy_device() currently returns NULL because of: 3ee82383f0098a2 "phy: 
fix phy address bug".  If I change it to return ERR_PTR(-ENODEV) that
will mean we break out of the loop with an error in mdiobus_register()
where before we would just continue on.

drivers/net/phy/mdio_bus.c
   119                          phydev = mdiobus_scan(bus, i);
   120                          if (IS_ERR(phydev)) {
   121                                  err = PTR_ERR(phydev);
   122                                  goto error;
   123                          }

Is that OK?

I'm not really familiar with this hardware at all, I'm just going based
on static analysis. :/

regards,
dan carpenter