netdev - Re: [PATCH] net: phy: return EPROBE_DEFER if PHY is not accessible

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a1dc47d6-fe07-2b13-ae53-ec6ea949333a@gmail.com>
Date:   Fri, 17 Mar 2023 10:05:14 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     arturo.buzarra@...i.com, netdev@...r.kernel.org,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Russell King <rmk+kernel@...linux.org.uk>
Subject: Re: [PATCH] net: phy: return EPROBE_DEFER if PHY is not accessible

+Andrew, Heiner, Vladimir and Russell,

(please always copy the maintainers of the piece of code you are modifying).

On 3/17/23 05:16, arturo.buzarra@...i.com wrote:
> From: Arturo Buzarra <arturo.buzarra@...i.com>
> 
> A PHY driver can dynamically determine the devices features, but in some
> circunstances, the PHY is not yet ready and the read capabilities does not fail
> but returns an undefined value, so incorrect capabilities are assumed and the
> initialization process fails. This commit postpones the PHY probe to ensure the
> PHY is accessible.

We need more specifics here, what type of PHY device are you seeing this 
with? Keying off all 0s or all 1s is problematic because while it could 
signal that the PHY is not ready in your particular case, it could also 
mean that you have an electrical issue whereby the MDIO data line is 
pulled down, respectively high too hard. In that case, we would rather 
error out earlier than later, because no amount of probe deferral will 
solve that.

If your PHY requires "some time" before it can be ready you have a 
number of ways to achieve that:

- implement phy_driver::probe which may load firmware, initialize 
internal state, etc.

- implement a phy_driver::get_features

Using deferred reads until MII_BMSR contains what you want is unlikely 
to be the recommended way by your vendor to ensure the PHY is ready. 
There has got to be some sort of vendor specific register that can be 
polled to indicate readiness.

> 
> Signed-off-by: Arturo Buzarra <arturo.buzarra@...i.com>
> ---
>   drivers/net/phy/phy_device.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 1785f1cead97..f8c31e741936 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2628,10 +2628,14 @@ int genphy_read_abilities(struct phy_device *phydev)
>   			       phydev->supported);
>   
>   	val = phy_read(phydev, MII_BMSR);
>   	if (val < 0)
>   		return val;
> +	if (val == 0x0000 || val == 0xffff) {
> +		phydev_err(phydev, "PHY is not accessible\n");
> +		return -EPROBE_DEFER;
> +	}

>   
>   	linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported,
>   			 val & BMSR_ANEGCAPABLE);
>   
>   	linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, phydev->supported,

-- 
Florian