[PATCH] net: phy: at803x: simplify using devm_gpiod_get_optional and its 4th argument

[PATCH] net: phy: at803x: simplify using devm_gpiod_get_optional and its 4th argument

[Uwe Kleine-König]

Since 39b2bbe3d715 (gpio: add flags argument to gpiod_get*() functions)
which appeared in v3.17-rc1, the gpiod_get* functions take an additional
parameter that allows to specify direction and initial value for output.
Moreover use devm_gpiod_get_index_optional instead of ignoring all
errors returned by devm_gpiod_get. Simplify accordingly.

The result is more strict error handling which is good.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/phy/at803x.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index f80e19ac6704..87dd633a8d1f 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -197,15 +197,12 @@ static int at803x_probe(struct phy_device *phydev)
 	if (!priv)
 		return -ENOMEM;
 
-	priv->gpiod_reset = devm_gpiod_get(dev, "reset");
-	if (IS_ERR(priv->gpiod_reset))
-		priv->gpiod_reset = NULL;
-	else
-		gpiod_direction_output(priv->gpiod_reset, 1);
+	priv->gpiod_reset = devm_gpiod_get_optional(dev, "reset",
+						    GPIOD_OUT_HIGH);
 
 	phydev->priv = priv;
 
-	return 0;
+	return IS_ERR(priv->gpiod_reset);
 }
 
 static int at803x_config_init(struct phy_device *phydev)
-- 
2.1.4

Re: [PATCH] net: phy: at803x: simplify using devm_gpiod_get_optional and its 4th argument

[Alexandre Courbot]

On 03/28/2015 04:25 AM, Uwe Kleine-König wrote:
> Since 39b2bbe3d715 (gpio: add flags argument to gpiod_get*() functions)
> which appeared in v3.17-rc1, the gpiod_get* functions take an additional
> parameter that allows to specify direction and initial value for output.
> Moreover use devm_gpiod_get_index_optional instead of ignoring all
> errors returned by devm_gpiod_get. Simplify accordingly.
>
> The result is more strict error handling which is good.

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>   drivers/net/phy/at803x.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index f80e19ac6704..87dd633a8d1f 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -197,15 +197,12 @@ static int at803x_probe(struct phy_device *phydev)
>   	if (!priv)
>   		return -ENOMEM;
>
> -	priv->gpiod_reset = devm_gpiod_get(dev, "reset");
> -	if (IS_ERR(priv->gpiod_reset))
> -		priv->gpiod_reset = NULL;
> -	else
> -		gpiod_direction_output(priv->gpiod_reset, 1);
> +	priv->gpiod_reset = devm_gpiod_get_optional(dev, "reset",
> +						    GPIOD_OUT_HIGH);
>
>   	phydev->priv = priv;
>
> -	return 0;
> +	return IS_ERR(priv->gpiod_reset);
>   }
>
>   static int at803x_config_init(struct phy_device *phydev)
>
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH] net: phy: at803x: simplify using devm_gpiod_get_optional and its 4th argument

[David Miller]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Date: Fri, 27 Mar 2015 20:25:02 +0100

> @@ -197,15 +197,12 @@ static int at803x_probe(struct phy_device *phydev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> -	priv->gpiod_reset = devm_gpiod_get(dev, "reset");
> -	if (IS_ERR(priv->gpiod_reset))
> -		priv->gpiod_reset = NULL;
> -	else
> -		gpiod_direction_output(priv->gpiod_reset, 1);
> +	priv->gpiod_reset = devm_gpiod_get_optional(dev, "reset",
> +						    GPIOD_OUT_HIGH);
>  
>  	phydev->priv = priv;
>  
> -	return 0;
> +	return IS_ERR(priv->gpiod_reset);
>  }
>  
>  static int at803x_config_init(struct phy_device *phydev)

This isn't right.

The current code is necessary, don't change it.

Your "simplification" adds three new bugs:

1) It potentially leaves an error pointer in priv->gpiod_reset
   and I explicitly tell people to NEVER do this as it tests as
   non-NULL by cleanup code and therefore might be mistakenly
   used.

2) It returns the wrong error.  IS_ERR() is either true or
   false, but if you wanted to do this right you would
   return PTR_ERR() if IS_ERR() were true or zero.

3) Clearly this code intended to continue trying and succeed
   the probe even if getting "reset" failed, your changes
   no longer do this.

I really hate changes like this, don't try to be too cute unless
you fully understand the full repurcussions and the reasons why
someone did things one way or another.

Leaving error pointers live in datastructures is never to be done, and
if I see patches adding code doing it I will reject it on the spot.

Re: [PATCH] net: phy: at803x: simplify using devm_gpiod_get_optional and its 4th argument

[Uwe Kleine-König]

On Tue, Mar 31, 2015 at 12:57:54PM -0400, David Miller wrote:
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Date: Fri, 27 Mar 2015 20:25:02 +0100
> 
> > @@ -197,15 +197,12 @@ static int at803x_probe(struct phy_device *phydev)
> >  	if (!priv)
> >  		return -ENOMEM;
> >  
> > -	priv->gpiod_reset = devm_gpiod_get(dev, "reset");
> > -	if (IS_ERR(priv->gpiod_reset))
> > -		priv->gpiod_reset = NULL;
> > -	else
> > -		gpiod_direction_output(priv->gpiod_reset, 1);
> > +	priv->gpiod_reset = devm_gpiod_get_optional(dev, "reset",
> > +						    GPIOD_OUT_HIGH);
> >  
> >  	phydev->priv = priv;
> >  
> > -	return 0;
> > +	return IS_ERR(priv->gpiod_reset);
> >  }
> >  
> >  static int at803x_config_init(struct phy_device *phydev)
> 
> This isn't right.
> 
> The current code is necessary, don't change it.
> 
> Your "simplification" adds three new bugs:
> 
> 1) It potentially leaves an error pointer in priv->gpiod_reset
>    and I explicitly tell people to NEVER do this as it tests as
>    non-NULL by cleanup code and therefore might be mistakenly
>    used.
If priv->gpiod_reset is an error value it makes the probe routine return
this error (after point 2 is addressed), which should end the lifetime
of the structure containing the value.

> 2) It returns the wrong error.  IS_ERR() is either true or
>    false, but if you wanted to do this right you would
>    return PTR_ERR() if IS_ERR() were true or zero.
Ah right, I should use PTR_ERR_OR_ZERO.

> 3) Clearly this code intended to continue trying and succeed
>    the probe even if getting "reset" failed, your changes
>    no longer do this.
It uses the _optional variant of devm_gpiod_get now, which returns NULL if
devm_gpiod_get would return -ENOENT. For all other errors returned by
devm_gpiod_get the code that is currently in place is wrong to ignore
them.

> I really hate changes like this, don't try to be too cute unless
> you fully understand the full repurcussions and the reasons why
> someone did things one way or another.
I think I did understand the code, so I will resend with
PTR_ERR_OR_ZERO assuming it was you who didn't understood my change
regarding the other two issues you pointed out.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] net: phy: at803x: simplify using devm_gpiod_get_optional and its 4th argument

[David Miller]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Date: Tue, 31 Mar 2015 19:53:47 +0200

> On Tue, Mar 31, 2015 at 12:57:54PM -0400, David Miller wrote:
>> 1) It potentially leaves an error pointer in priv->gpiod_reset
>>    and I explicitly tell people to NEVER do this as it tests as
>>    non-NULL by cleanup code and therefore might be mistakenly
>>    used.
> If priv->gpiod_reset is an error value it makes the probe routine return
> this error (after point 2 is addressed), which should end the lifetime
> of the structure containing the value.

It doesn't matter, it's a dangerous practice and not doing so makes
sure that no matter how this code is ever changed nobody can run into
potential problems due to this.

>> I really hate changes like this, don't try to be too cute unless
>> you fully understand the full repurcussions and the reasons why
>> someone did things one way or another.
> I think I did understand the code, so I will resend with
> PTR_ERR_OR_ZERO assuming it was you who didn't understood my change
> regarding the other two issues you pointed out.

I'm not applying this patch if you keep putting error pointers into
the structure member.

Please do not fight me on this, I do not consider your changes an
improvement at all.