netdev - Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB

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]

Date:	Tue, 22 Mar 2016 15:53:51 +0100
From:	Sebastian Frias <sf84@...oste.net>
To:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>,
	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
CC:	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	lkml <linux-kernel@...r.kernel.org>, mason <slash.tmp@...e.fr>,
	Daniel Mack <zonque@...il.com>
Subject: Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB

Hi,

On 03/21/2016 09:41 PM, Uwe Kleine-König wrote:
>>    My patch basically gets rid of all this code. The thing that worries me
>> is that the driver assumes that the reset singal is active low, despite what
>> the GPIO specifier in the device tree has for the GPIO polarity. In fact, it
>> will only work correctly if the specified has GPIO_ACTIVE_HIGH -- which is
>> wrong because the reset signal is active low!
> 
> Note that gpio descriptors handle the polarity just fine (i.e. the pin
> is set to 0 after doing gpiod_set_value(1) if the gpio is active low).
> 

Isn't that source of bugs?
What about using some #define (or probably better, an enum)?, something
like:

    gpiod_set_value(gpiod, GPIO_SET_VALUE_ACTIVE)
    gpiod_set_value(gpiod, GPIO_SET_VALUE_INACTIVE)
    gpiod_set_value(gpiod, GPIO_SET_VALUE_TRISTATE)

then, somebody reading the code would have to stop and think what do
these mean.
IIUC, currently the "0" or "1" can easily be confused with the actual
logical value of the GPIO.

gpiod_set_value() could also return an int with the actual value it
applied to the GPIO.
For example: if gpiod is active low, gpiod_set_value(gpiod,
GPIO_SET_VALUE_ACTIVE) would return 0;
Conversely, if gpiod is active high, gpiod_set_value(gpiod,
GPIO_SET_VALUE_ACTIVE) would return 1;

Best regards,

Sebastian

> But having said that, the driver gets it wrong.
> 
> The right sequence to reset a device using a gpio is:
> 
> 	gpiod_set_value(priv->gpiod_reset, 1);
> 	msleep(some_time);
> 	gpiod_set_value(priv->gpiod_reset, 0);
> 
> and if the gpio is active low, this should be specified in the device
> tree. This was done wrong in 13a56b449325 (net: phy: at803x: Add support
> for hardware reset).
> 
> Best regards
> Uwe
>