netdev - Re: [PATCH RFT 1/2] phylib: add device reset GPIO support

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:	Sun, 15 May 2016 00:14:29 +0300
From:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
	f.fainelli@...il.com
Cc:	grant.likely@...aro.org, robh+dt@...nel.org,
	devicetree@...r.kernel.org, netdev@...r.kernel.org,
	frowand.list@...il.com, pawel.moll@....com, mark.rutland@....com,
	ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
	linux-kernel@...r.kernel.org,
	Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH RFT 1/2] phylib: add device reset GPIO support

Hello.

On 05/13/2016 10:18 PM, Sergei Shtylyov wrote:

>>> [we already talked about this patch in #armlinux, I'm now just
>>> forwarding my comments on the list. Background was that I sent an easier
>>> and less complete patch with the same idea. See
>>> http://patchwork.ozlabs.org/patch/621418/]
>>>
>>> [added Linus Walleij to Cc, there is a question for you/him below]
>>>
>>> On Fri, Apr 29, 2016 at 01:12:54AM +0300, Sergei Shtylyov wrote:
>>>> --- net-next.orig/Documentation/devicetree/bindings/net/phy.txt
>>>> +++ net-next/Documentation/devicetree/bindings/net/phy.txt
>>>> @@ -35,6 +35,8 @@ Optional Properties:
>>>>  - broken-turn-around: If set, indicates the PHY device does not correctly
>>>>    release the turn around line low at the end of a MDIO transaction.
>>>>
>>>> +- reset-gpios: The GPIO phandle and specifier for the PHY reset signal.
>>>> +
>>>>  Example:
>>>>
>>>>  ethernet-phy@0 {
>>>
>>> This is great.
>>>
>>>> Index: net-next/drivers/net/phy/at803x.c
>>>> ===================================================================
>>>> --- net-next.orig/drivers/net/phy/at803x.c
>>>> +++ net-next/drivers/net/phy/at803x.c
>>>> @@ -65,7 +65,6 @@ MODULE_LICENSE("GPL");
>>>> [...]
>>>
>>> My patch breaks this driver. I wasn't aware of it.
>>
>>    I tried to be as careful as I could but still it looks that I didn't
>> succeed at that too...
>
>    Hm, I'm starting to forget the vital details about my patch...
>
>> [...]
>>>> Index: net-next/drivers/net/phy/mdio_device.c
>>>> ===================================================================
>>>> --- net-next.orig/drivers/net/phy/mdio_device.c
>>>> +++ net-next/drivers/net/phy/mdio_device.c
>> [...]
>>>> @@ -117,9 +126,16 @@ static int mdio_probe(struct device *dev
>>>>      struct mdio_driver *mdiodrv = to_mdio_driver(drv);
>>>>      int err = 0;
>>>>
>>>> -    if (mdiodrv->probe)
>>>> +    if (mdiodrv->probe) {
>>>> +        /* Deassert the reset signal */
>>>> +        mdio_device_reset(mdiodev, 0);
>>>> +
>>>>          err = mdiodrv->probe(mdiodev);
>>>>
>>>> +        /* Assert the reset signal */
>>>> +        mdio_device_reset(mdiodev, 1);
>>>
>>> I wonder if it's safe to do this in general. What if ->probe does
>>> something with the phy that is lost by resetting but that is relied on
>>> later?
>>
>>    Well, I thought that config_init() method is designed for that but indeed
>> the LXT driver writes to BMCR in its probe() method and hence is broken.
>> Thank you for noticing...
>
>    It's broken even without my patch. The phylib will cause a PHY soft reset

    Only iff the config_init() method exists in the PHY driver...

> when attaching to the PHY device, so all BMCR programming dpone in the probe()
> method will be lost. My patch does make sense as is.

    No, actually it doesn't. :-(

> Looks like I should alsolook into fixing lxt.c.

    It took me to actually do a patch to understand my fault. Sigh... :-/

> Florian, what do you think?

    Florian, is phy_init_hw() logic correct?

MBR, Sergei