[patch] drivers: phy: tweaks to phy_create()

[patch] drivers: phy: tweaks to phy_create()

[Dan Carpenter]

If this was called with a NULL "dev" then it lead to a NULL dereference
when we called dev_WARN().  I have changed it to WARN_ON() so that we
get a stack dump and can fix the caller.

If ida_simple_get() failed then there was a missing call to kfree(phy).

The rest of this patch is just cleanup like returning directly instead
of having do-nothing gotos.  Using descriptive labels instead of
GW-BASIC style "err0" and "err1".  I also flipped the order of
put_device() and ida_remove() so they are a mirror reflection of the
order they were allocated.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 03cf8fb..18b9de7 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -437,23 +437,18 @@ struct phy *phy_create(struct device *dev, const struct phy_ops *ops,
 	int id;
 	struct phy *phy;
 
-	if (!dev) {
-		dev_WARN(dev, "no device provided for PHY\n");
-		ret = -EINVAL;
-		goto err0;
-	}
+	if (WARN_ON(!dev))
+		return ERR_PTR(-EINVAL);
 
 	phy = kzalloc(sizeof(*phy), GFP_KERNEL);
-	if (!phy) {
-		ret = -ENOMEM;
-		goto err0;
-	}
+	if (!phy)
+		return ERR_PTR(-ENOMEM);
 
 	id = ida_simple_get(&phy_ida, 0, 0, GFP_KERNEL);
 	if (id < 0) {
 		dev_err(dev, "unable to get id\n");
 		ret = id;
-		goto err0;
+		goto free_phy;
 	}
 
 	device_initialize(&phy->dev);
@@ -468,11 +463,11 @@ struct phy *phy_create(struct device *dev, const struct phy_ops *ops,
 
 	ret = dev_set_name(&phy->dev, "phy-%s.%d", dev_name(dev), id);
 	if (ret)
-		goto err1;
+		goto put_dev;
 
 	ret = device_add(&phy->dev);
 	if (ret)
-		goto err1;
+		goto put_dev;
 
 	if (pm_runtime_enabled(dev)) {
 		pm_runtime_enable(&phy->dev);
@@ -481,12 +476,12 @@ struct phy *phy_create(struct device *dev, const struct phy_ops *ops,
 
 	return phy;
 
-err1:
-	ida_remove(&phy_ida, phy->id);
+put_dev:
 	put_device(&phy->dev);
+	ida_remove(&phy_ida, phy->id);
+free_phy:
 	kfree(phy);
 
-err0:
 	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(phy_create);

Re: [patch] drivers: phy: tweaks to phy_create()

[Kishon Vijay Abraham I]

Hi,

On Wednesday 06 November 2013 01:24 PM, Dan Carpenter wrote:
> If this was called with a NULL "dev" then it lead to a NULL dereference
> when we called dev_WARN().  I have changed it to WARN_ON() so that we
> get a stack dump and can fix the caller.
>
> If ida_simple_get() failed then there was a missing call to kfree(phy).

There was already a patch fixing it.
git://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git fixes
>
> The rest of this patch is just cleanup like returning directly instead
> of having do-nothing gotos.  Using descriptive labels instead of

Grouping the err returns in the end looked a bit cleaner to me. It's 
just a matter of preference I guess.
> GW-BASIC style "err0" and "err1".  I also flipped the order of
> put_device() and ida_remove() so they are a mirror reflection of the
> order they were allocated.
>

Thanks
Kishon

Re: [patch] drivers: phy: tweaks to phy_create()

[Dan Carpenter]

On Wed, Nov 06, 2013 at 01:41:26PM +0530, Kishon Vijay Abraham I wrote:

> >The rest of this patch is just cleanup like returning directly instead
> >of having do-nothing gotos.  Using descriptive labels instead of
> 
> Grouping the err returns in the end looked a bit cleaner to me. It's
> just a matter of preference I guess.

Say if you are reading the code and it says:

	goto err0;

What you think is, "Huh, that's a crap name for a label.  I wonder what
it does?"  So you have interrupt what you are doing and scroll down and
then you see that it doesn't do anything.  Not a single thing.  It just
returns.  Then you have to scroll back and try remember where you were.

But if you use a clear label like:

	ret = -ENOMEM;
	goto return_ret;

Then I don't have to scroll down to find out what it is.

But it's even more clear to just say:

	return -ENOMEM;

The original code was not just messy, it was also buggy.  If you do the
error handling in the right way by unwinding in the correct order and
thinking about label names then your code will be less buggy.

regards,
dan carpenter

Re: [patch] drivers: phy: tweaks to phy_create()

[Kishon Vijay Abraham I]

Hi Dan,

On Wednesday 06 November 2013 01:24 PM, Dan Carpenter wrote:
> If this was called with a NULL "dev" then it lead to a NULL dereference
> when we called dev_WARN().  I have changed it to WARN_ON() so that we
> get a stack dump and can fix the caller.
> 
> If ida_simple_get() failed then there was a missing call to kfree(phy).
> 
> The rest of this patch is just cleanup like returning directly instead
> of having do-nothing gotos.  Using descriptive labels instead of
> GW-BASIC style "err0" and "err1".  I also flipped the order of
> put_device() and ida_remove() so they are a mirror reflection of the
> order they were allocated.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Can you create this patch on top of
git://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git fixes ?

Thanks
Kishon

> 
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index 03cf8fb..18b9de7 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -437,23 +437,18 @@ struct phy *phy_create(struct device *dev, const struct phy_ops *ops,
>  	int id;
>  	struct phy *phy;
>  
> -	if (!dev) {
> -		dev_WARN(dev, "no device provided for PHY\n");
> -		ret = -EINVAL;
> -		goto err0;
> -	}
> +	if (WARN_ON(!dev))
> +		return ERR_PTR(-EINVAL);
>  
>  	phy = kzalloc(sizeof(*phy), GFP_KERNEL);
> -	if (!phy) {
> -		ret = -ENOMEM;
> -		goto err0;
> -	}
> +	if (!phy)
> +		return ERR_PTR(-ENOMEM);
>  
>  	id = ida_simple_get(&phy_ida, 0, 0, GFP_KERNEL);
>  	if (id < 0) {
>  		dev_err(dev, "unable to get id\n");
>  		ret = id;
> -		goto err0;
> +		goto free_phy;
>  	}
>  
>  	device_initialize(&phy->dev);
> @@ -468,11 +463,11 @@ struct phy *phy_create(struct device *dev, const struct phy_ops *ops,
>  
>  	ret = dev_set_name(&phy->dev, "phy-%s.%d", dev_name(dev), id);
>  	if (ret)
> -		goto err1;
> +		goto put_dev;
>  
>  	ret = device_add(&phy->dev);
>  	if (ret)
> -		goto err1;
> +		goto put_dev;
>  
>  	if (pm_runtime_enabled(dev)) {
>  		pm_runtime_enable(&phy->dev);
> @@ -481,12 +476,12 @@ struct phy *phy_create(struct device *dev, const struct phy_ops *ops,
>  
>  	return phy;
>  
> -err1:
> -	ida_remove(&phy_ida, phy->id);
> +put_dev:
>  	put_device(&phy->dev);
> +	ida_remove(&phy_ida, phy->id);
> +free_phy:
>  	kfree(phy);
>  
> -err0:
>  	return ERR_PTR(ret);
>  }
>  EXPORT_SYMBOL_GPL(phy_create);
>