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]

Message-ID: <f1fae99f-2464-8a42-deb8-c27cccf8df12@cogentembedded.com>
Date:	Fri, 13 May 2016 00:35:50 +0300
From:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:	grant.likely@...aro.org, robh+dt@...nel.org,
	devicetree@...r.kernel.org, f.fainelli@...il.com,
	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/12/2016 09:42 PM, Uwe Kleine-König 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...

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

[...]
>> Index: net-next/drivers/of/of_mdio.c
>> ===================================================================
>> --- net-next.orig/drivers/of/of_mdio.c
>> +++ net-next/drivers/of/of_mdio.c
>> @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n
>>  static void of_mdiobus_register_phy(struct mii_bus *mdio,
>>  				    struct device_node *child, u32 addr)
>>  {
>> +	struct gpio_desc *gpiod;
>>  	struct phy_device *phy;
>>  	bool is_c45;
>>  	int rc;
>> @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru
>>  	is_c45 = of_device_is_compatible(child,
>>  					 "ethernet-phy-ieee802.3-c45");
>>
>> +	gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
>> +	/* Deassert the reset signal */
>> +	if (!IS_ERR(gpiod))
>> +		gpiod_direction_output(gpiod, 0);
>
> This is wrong I think. You must only ignore -ENODEV, all other error

    At least -ENOSYS should also be ignored (it's returned when gpiolib is not 
configured), right? When does -ENODEV gets returned (it's not easy to follow)?

> codes should be passed to the caller.

    The caller doesn't care anyway...

> (I see that's not trivial because
> of_mdiobus_register_phy returns void.)

    I've made this function *void* in net-next.

> In my patch I used devm_gpiod_get_array which has the nice property that
> I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime
> of the gpio to the device which is nice and IMHO the right direction for
> the phylib (i.e. better embracing of the device model).
>
> This cannot be used here easily however because there is no struct
> device yet and this is only created after the phy id is determined.

    Your last patch [1] didn't make use of the managed device API (devm) 
either, I didn't quite get to the bottom of that...

> The
> phy id is either read from the device tree or must be read from the phy
> which might fail if reset is not deasserted.

> Principally there is no reason however that the phy_id must be known
> before the struct device is created however.

    It's just that the code is cleaner that way...

[1] http://paste.debian.net/683630/

> Best regards
> Uwe

MBR, Sergei