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: <20160512184233.GJ30822@pengutronix.de>
Date:	Thu, 12 May 2016 20:42:33 +0200
From:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
To:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
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 Sergei,

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

> [...]
> 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
> [...]
> @@ -103,6 +105,13 @@ void mdio_device_remove(struct mdio_devi
>  }
>  EXPORT_SYMBOL(mdio_device_remove);
>  
> +void mdio_device_reset(struct mdio_device *mdiodev, int value)
> +{
> +	if (mdiodev->reset)
> +		gpiod_set_value(mdiodev->reset, value);

Before v4.6-rc1~108^2~91 it was not necessary to check for the first
parameter being non-NULL before calling gpiod_set_value. Linus, did you
change this on purpose?

> +}
> +EXPORT_SYMBOL(mdio_device_reset);
> +
>  /**
>   * mdio_probe - probe an MDIO device
>   * @dev: device to probe
> @@ -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?

> +	}
> +
>  	return err;
>  }
> [...]
> 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
codes should be passed to the caller. (I see that's not trivial because
of_mdiobus_register_phy returns void.)

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

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |