netdev - Re: [PATCH] net: phy: at803x: simplify using devm_gpiod_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150331175347.GL17728@pengutronix.de>
Date:	Tue, 31 Mar 2015 19:53:47 +0200
From:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
To:	David Miller <davem@...emloft.net>
Cc:	f.fainelli@...il.com, netdev@...r.kernel.org,
	linus.walleij@...aro.org, acourbot@...dia.com
Subject: Re: [PATCH] net: phy: at803x: simplify using devm_gpiod_get_optional
 and its 4th argument

On Tue, Mar 31, 2015 at 12:57:54PM -0400, David Miller wrote:
> From: Uwe Kleine-König <u.kleine-koenig@...gutronix.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/  |
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html