netdev - Re: [RFC net-next 2/2] net: stmmac: Convert to phylink

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: <34db7462-4f5a-155a-d230-e4c90cee1ce3@gmail.com>
Date:   Fri, 7 Jun 2019 20:58:02 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Jose Abreu <Jose.Abreu@...opsys.com>, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org
Cc:     Joao Pinto <Joao.Pinto@...opsys.com>,
        "David S . Miller" <davem@...emloft.net>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Russell King <linux@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: [RFC net-next 2/2] net: stmmac: Convert to phylink



On 6/6/2019 4:26 AM, Jose Abreu wrote:
> Convert stmmac driver to phylink.
> 
> Signed-off-by: Jose Abreu <joabreu@...opsys.com>
> Cc: Joao Pinto <jpinto@...opsys.com>
> Cc: David S. Miller <davem@...emloft.net>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@...com>
> Cc: Alexandre Torgue <alexandre.torgue@...com>
> Cc: Russell King <linux@...linux.org.uk>
> Cc: Andrew Lunn <andrew@...n.ch>
> Cc: Florian Fainelli <f.fainelli@...il.com>
> Cc: Heiner Kallweit <hkallweit1@...il.com>
> ---
[snip]

			     interface);
> -	}
> +	if (node) {
> +		ret = phylink_of_phy_connect(priv->phylink, node, 0);
> +	} else {
> +		int addr = priv->plat->phy_addr;
> +		struct phy_device *phydev;
>  
> -	if (IS_ERR_OR_NULL(phydev)) {
> -		netdev_err(priv->dev, "Could not attach to PHY\n");
> -		if (!phydev)
> +		phydev = mdiobus_get_phy(priv->mii, addr);
> +		if (!phydev) {
> +			netdev_err(priv->dev, "no phy at addr %d\n", addr);
>  			return -ENODEV;
> +		}

I am not exactly sure removing this is strictly equivalent here, but the
diff makes it hard to review.

For the sake of reviewing the code, could you structure the patches like
you did with an intermediate step being:

- prepare for PHYLINK (patch #1)
- add support for PHYLINK (parts of this patch) but without plugging it
into the probe/connect path just yet
- get rid of all PHYLIB related code (parts of this patch), which would
be a new patch #3

AFAIR, there are several cases that stmmac supports today:

- fixed PHY (covered by PHYLINK)
- PHY designated via phy-handle (covered by PHYLINK)
- PHY address via platform data (not covered by PHYLINK unless we add
support for that), with no Device Tree node
- when a MDIO bus Device Tree node is present, register the stmmac MDIO
bus (which you don't change).

I believe I have convinced myself that the third case is covered with
the change above :)

This looks great, thanks!
-- 
Florian