| Message ID | 20241114165348.2445021-1-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phylink: improve phylink_sfp_config_phy() error message with empty supported | expand |
> [ 64.738270] mv88e6085 d0032004.mdio-mii:12 sfp: PHY i2c:sfp:16 (id 0x01410cc2) supports no link modes. Maybe its specific PHY driver not loaded? > [ 64.769731] 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. I think this is useful. I only have a minor nitpick, maybe in the commit message mention which PHY drivers are typically used by SFPs, to point somebody who gets this message in the right direction. The Marvell driver is one. at803x i think is also used. Are then any others? Andrew
On Thu, Nov 14, 2024 at 06:38:13PM +0100, Andrew Lunn wrote: > > [ 64.738270] mv88e6085 d0032004.mdio-mii:12 sfp: PHY i2c:sfp:16 (id 0x01410cc2) supports no link modes. Maybe its specific PHY driver not loaded? > > [ 64.769731] 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. > > I think this is useful. > > I only have a minor nitpick, maybe in the commit message mention which > PHY drivers are typically used by SFPs, to point somebody who gets > this message in the right direction. The Marvell driver is one. at803x > i think is also used. Are then any others? bcm84881 too. Not sure about at803x - the only SFP I know that uses that PHY doesn't make the PHY available to the host.
On Thu, Nov 14, 2024 at 06:53:48PM +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. Somewhat related, i wounder if we should add a phydev_info() message in genphy_probe() which prints a message that you probably want to swap to using a PHY driver specific to the hardware? The less genphy is used, the better. Andrew
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index b1e828a4286d..efeff8733a52 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -3219,6 +3219,11 @@ static int phylink_sfp_config_phy(struct phylink *pl, u8 mode, 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); + return -EINVAL; + } memset(&config, 0, sizeof(config)); linkmode_copy(config.advertising, phy->advertising);
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: [ 64.738270] mv88e6085 d0032004.mdio-mii:12 sfp: PHY i2c:sfp:16 (id 0x01410cc2) supports no link modes. Maybe its specific PHY driver not loaded? [ 64.769731] 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@nxp.com> --- drivers/net/phy/phylink.c | 5 +++++ 1 file changed, 5 insertions(+)