linux-kernel - 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]

Message-ID: <56F06E21.7010507@cogentembedded.com>
Date:	Tue, 22 Mar 2016 00:56:49 +0300
From:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:	Sebastian Frias <sf84@...oste.net>,
	"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

On 03/21/2016 11:41 PM, Uwe Kleine-König wrote:

>>> Commit 687908c2b649 ("net: phy: at803x: simplify using
>>> devm_gpiod_get_optional and its 4th argument") introduced a dependency
>>> on GPIOLIB that was not there before.
>>>
>>> This commit removes such dependency by checking the return code and
>>> comparing it against ENOSYS which is returned when GPIOLIB is not
>>> selected.
>>>
>>> Fixes: 687908c2b649 ("net: phy: at803x: simplify using
>>> devm_gpiod_get_optional and its 4th argument")
>>>
>>> Signed-off-by: Sebastian Frias <sf84@...oste.net>
>>
>>     Do you have the PHY that requires the GPIO reset workaround?
>> Askinjg because I have the patch adding the "reset-gpios" prop handling to
>> phylib and your patch made me aware that I'll have to modify this driver in
>> order to do that...
>>
>>> ---
>>>   drivers/net/phy/at803x.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
>>> index 2174ec9..88b7ff3 100644
>>> --- a/drivers/net/phy/at803x.c
>>> +++ b/drivers/net/phy/at803x.c
>>> @@ -252,7 +252,9 @@ static int at803x_probe(struct phy_device *phydev)
>>>   		return -ENOMEM;
>>>
>>>   	gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>>> -	if (IS_ERR(gpiod_reset))
>>> +	if (PTR_ERR(gpiod_reset) == -ENOSYS)
>>> +		gpiod_reset = NULL;
>>> +	else if (IS_ERR(gpiod_reset))
>>     		return PTR_ERR(gpiod_reset);
>>
>>     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).

    I know. :-)

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

    I wonder if that was done before GPIO_ACTIVE_* thing was introduced... 
there are precedents in other MAC drivers that want a separate 
"phy-reset-active-low" or even -"high" prop...

> Best regards
> Uwe

MBR, Sergei