netdev - Re: net: phylink: supported modes set to 0 with genphy sfp module

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: <20200512122844.GA1551@shell.armlinux.org.uk>
Date:   Tue, 12 May 2020 13:28:44 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Julien Beraud <julien.beraud@...lia.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Antoine Tenart <antoine.tenart@...tlin.com>
Subject: Re: net: phylink: supported modes set to 0 with genphy sfp module

On Tue, May 12, 2020 at 11:28:40AM +0200, Julien Beraud wrote:
> 
> 
> On 11/05/2020 21:06, Florian Fainelli wrote:
> > 
> > 
> > On 5/11/2020 11:29 AM, Russell King - ARM Linux admin wrote:
> > > On Mon, May 11, 2020 at 05:45:02PM +0200, Julien Beraud wrote:
> > > > Following commit:
> > > > 
> > > > commit 52c956003a9d5bcae1f445f9dfd42b624adb6e87
> > > > Author: Russell King <rmk+kernel@...linux.org.uk>
> > > > Date:   Wed Dec 11 10:56:45 2019 +0000
> > > > 
> > > >      net: phylink: delay MAC configuration for copper SFP modules
> > > > 
> > > > 
> > > > In function phylink_sfp_connect_phy, phylink_sfp_config is called before
> > > > phylink_attach_phy.
> > > > 
> > > > In the case of a genphy, the "supported" field of the phy_device is filled
> > > > by:
> > > > phylink_attach_phy->phy_attach_direct->phy_probe->genphy_read_abilities.
> > > > 
> > > > It means that:
> > > > 
> > > > ret = phylink_sfp_config(pl, mode, phy->supported, phy->advertising);
> > > > will have phy->supported with no bits set, and then the first call to
> > > > phylink_validate in phylink_sfp_config will return an error:
> > > > 
> > > > return phylink_is_empty_linkmode(supported) ? -EINVAL : 0;
> > > > 
> > > > this results in putting the sfp driver in "failed" state.
> > > 
> > > Which PHY is this?
> 
> The phy seems to be Marvell 88E1111, so the simple solution is to just add the driver for this PHY to my config.
> That said, if for some reason someone plugs a module for which no phy driver is found the issue will happen again.

Right, please use the specific PHY driver for modules that contain the
88E1111 to avoid any surprises, otherwise things can end up being
misconfigured - for example, the PHY using 1000base-X and the host
using SGMII, which may work or may lead to problems.

> > Using the generic PHY with a copper SFP module does not sound like a
> > great idea because without a specialized PHY driver (that is, not the
> > Generic PHY driver) there is not usually much that can happen.
> Thanks for the info. I don't have an advice on whether it is a good idea to use a copper sfp without a specialized driver,
> but before commit 52c956003a9d5bcae1f445f9dfd42b624adb6e87, it used to work and I could at least get a network connection.
> 
> Moreover, this commit didn't explicitely intend to forbid this behavior and the error is not explicit either.
> 
> If phylink+sfp still supports using genphy as a fallback, It may be good to fix the current behavior.
> If not, maybe adding an explicit warning or error would be preferrable.

The commit is designed to increase the range of modules that can be
used, especially when the module supports higher rates than the MAC.

The downside is that we _must_ know the PHYs capabilities before
attaching to it, so that we can choose an appropriate interface to
_attach_ to it with.  It's a chicken and egg problem.

For this to work reliably in cases such as those you've identified,
I need phylib to always give me that information earlier than it
currently seems to for the genphy fallback - but the problem is the
genphy fallback only happens after attaching to it.  So again,
another chicken and egg problem.

Subsituting the SFP modules capabilities seems like a workaround,
but those are also a guess from poor information in the SFP EEPROM.
It is what we were doing before however...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up