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 for Android: free password hash cracker in your pocket

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

Message-Id: <20150331.125754.1434513759096073981.davem@davemloft.net>
Date:	Tue, 31 Mar 2015 12:57:54 -0400 (EDT)
From:	David Miller <davem@...emloft.net>
To:	u.kleine-koenig@...gutronix.de
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

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.

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.
--
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