netdev - Re: [PATCH v2 net-next] net: phylink: improve phylink_sfp_config

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: <Z1nT-GlW24hgHkfx@shell.armlinux.org.uk>
Date: Wed, 11 Dec 2024 18:03:36 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH v2 net-next] net: phylink: improve
 phylink_sfp_config_phy() error message with empty supported

On Wed, Dec 11, 2024 at 07:25:36PM +0200, Vladimir Oltean wrote:
> It seems that phylink does not support driving PHYs in SFP modules using
> the Generic PHY or Generic Clause 45 PHY driver. I've come to this
> conclusion after analyzing these facts:
> 
> - sfp_sm_probe_phy(), who is our caller here, first calls
>   phy_device_register() and then sfp_add_phy() -> ... ->
>   phylink_sfp_connect_phy().
> 
> - phydev->supported is populated by phy_probe()
> 
> - phy_probe() is usually called synchronously from phy_device_register()
>   via phy_bus_match(), if a precise device driver is found for the PHY.
>   In that case, phydev->supported has a good chance of being set to a
>   non-zero mask.
> 
> - There is an exceptional case for the PHYs for which phy_bus_match()
>   didn't find a driver. Those devices sit for a while without a driver,
>   then phy_attach_direct() force-binds the genphy_c45_driver or
>   genphy_driver to them. Again, this triggers phy_probe() and renders
>   a good chance of phydev->supported being populated, assuming
>   compatibility with genphy_read_abilities() or
>   genphy_c45_pma_read_abilities().
> 
> - phylink_sfp_config_phy() does not support the exceptional case of
>   retrieving phydev->supported from the Generic PHY driver, due to its
>   code flow. It expects the phydev->supported mask to already be
>   non-empty, because it first calls phylink_validate() on it, and only
>   calls phylink_attach_phy() if that succeeds. Thus, phylink_attach_phy()
>   -> phy_attach_direct() has no chance of running.
> 
> It is not my wish to change the state of affairs by altering the code
> flow, but merely to document the limitation rather than have the current
> unspecific error:
> 
> [   61.800079] mv88e6085 d0032004.mdio-mii:12 sfp: validation with support 00,00000000,00000000,00000000 failed: -EINVAL
> [   61.820743] sfp sfp: sfp_add_phy failed: -EINVAL
> 
> On the premise that an empty phydev->supported is going to make
> phylink_validate() fail anyway, it would be more informative to single
> out that case, undercut the phylink_validate() call, and print a more
> specific message:
> 
> [   33.468000] mv88e6085 d0032004.mdio-mii:12 sfp: PHY i2c:sfp:16 (id 0x01410cc2) supports no link modes. Maybe its specific PHY driver not loaded?
> [   33.488187] mv88e6085 d0032004.mdio-mii:12 sfp: Common drivers for PHYs on SFP modules are CONFIG_BCM84881_PHY and CONFIG_MARVELL_PHY.
> [   33.518621] sfp sfp: sfp_add_phy failed: -EINVAL
> 
> Of course, there may be other reasons due to which phydev->supported is
> empty, thus the use of the word "maybe", but I think the lack of a
> driver would be the most common.
> 
> Link: https://lore.kernel.org/netdev/20241113144229.3ff4bgsalvj7spb7@skbuf/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> v1->v2: add one more informational line containing common Kconfig
> options, as per review feedback.
> 
> Link to v1:
> https://lore.kernel.org/netdev/20241114165348.2445021-1-vladimir.oltean@nxp.com/
> 
>  drivers/net/phy/phylink.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 95fbc363f9a6..b9dee09f4cfc 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -3436,6 +3436,12 @@ static int phylink_sfp_config_phy(struct phylink *pl, struct phy_device *phy)
>  	int ret;
>  
>  	linkmode_copy(support, phy->supported);
> +	if (linkmode_empty(support)) {
> +		phylink_err(pl, "PHY %s (id 0x%.8lx) supports no link modes. Maybe its specific PHY driver not loaded?\n",
> +			    phydev_name(phy), (unsigned long)phy->phy_id);
> +		phylink_err(pl, "Common drivers for PHYs on SFP modules are CONFIG_BCM84881_PHY and CONFIG_MARVELL_PHY.\n");
> +		return -EINVAL;
> +	}

Wouldn't checking phy->drv == NULL be a better to detect that there is
no PHY driver bound (and thus indicating that the specific driver is
not loaded?)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!