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] [day] [month] [year] [list]

Message-ID: <b7b371e1-da07-29f9-44cb-f184377774c4@orolia.com>
Date:   Wed, 13 May 2020 09:14:24 +0200
From:   Julien Beraud <julien.beraud@...lia.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
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 12/05/2020 14:28, Russell King - ARM Linux admin wrote:
> 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.
> 
I'll do that, thanks.

>>> 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...
> 

Thank you very much for the explanations.
I'll try this patch for now.

Regards,
Julien