netdev - Re: [PATCH net-next] net: phy: phylink: add fallback from SGMII to 1000BaseX

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: <4b7bf6b4-dbc8-d3d0-4b31-789fcdb8e6b7@sedsystems.ca>
Date:   Fri, 31 May 2019 18:17:51 -0600
From:   Robert Hancock <hancock@...systems.ca>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: phy: phylink: add fallback from SGMII to
 1000BaseX

On 2019-05-31 2:18 p.m., Russell King - ARM Linux admin wrote:
> On Fri, May 31, 2019 at 01:18:04PM -0600, Robert Hancock wrote:
>> Some copper SFP modules support both SGMII and 1000BaseX,
> 
> The situation is way worse than that.  Some copper SFP modules are
> programmed to support SGMII only.  Others are programmed to support
> 1000baseX only.  There is no way to tell from the EEPROM how they
> are configured, and there is no way to auto-probe the format of the
> control word (which is the difference between the two.)
> 
>> but some
>> drivers/devices only support the 1000BaseX mode. Currently SGMII mode is
>> always being selected as the desired mode for such modules, and this
>> fails if the controller doesn't support SGMII. Add a fallback for this
>> case by trying 1000BaseX instead if the controller rejects SGMII mode.
> 
> So, what happens when a controller supports both SGMII and 1000base-X
> modes (such as the Marvell devices) but the module is setup for
> 1000base-X mode?

My description is likely a bit incorrect.. rather than supporting both
1000BaseX and SGMII, a given module can support either 1000BaseX or
SGMII, but likely only one at a time (at least without magic
vendor-specific commands to switch modes).

The logic in sfp_select_interface always selects SGMII mode for copper
modules, which is the preferred mode of operation since 100 and 10 Mbps
modes won't work with 1000BaseX. If the controller and module actually
support SGMII, everything is fine. If the controller doesn't support
SGMII, it should fail validation and the link won't come up. If the
module doesn't support SGMII, it may try to come up but the link likely
won't work properly.

Our device is mainly intended for fiber modules, which is why 1000BaseX
is being used. The variant of fiber modules we are using (for example,
Finisar FCLF8520P2BTL) are set up for 1000BaseX, and seem like they are
kind of a hack to allow using copper on devices which only support
1000BaseX mode (in fact that particular one is extra hacky since you
have to disable 1000BaseX autonegotiation on the host side). This patch
is basically intended to allow that particular case to work.

It's kind of a dumb situation, but in the absence of a way to tell from
the EEPROM content what mode the module is actually in (and you would
likely know better than I if there was), it seems like the best we can do.

> 
>>
>> Signed-off-by: Robert Hancock <hancock@...systems.ca>
>> ---
>>  drivers/net/phy/phylink.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
>> index 68d0a89..4fd72c2 100644
>> --- a/drivers/net/phy/phylink.c
>> +++ b/drivers/net/phy/phylink.c
>> @@ -1626,6 +1626,7 @@ static int phylink_sfp_module_insert(void *upstream,
>>  {
>>  	struct phylink *pl = upstream;
>>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(support) = { 0, };
>> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(orig_support) = { 0, };
>>  	struct phylink_link_state config;
>>  	phy_interface_t iface;
>>  	int ret = 0;
>> @@ -1635,6 +1636,7 @@ static int phylink_sfp_module_insert(void *upstream,
>>  	ASSERT_RTNL();
>>  
>>  	sfp_parse_support(pl->sfp_bus, id, support);
>> +	linkmode_copy(orig_support, support);
>>  	port = sfp_parse_port(pl->sfp_bus, id, support);
>>  
>>  	memset(&config, 0, sizeof(config));
>> @@ -1663,6 +1665,25 @@ static int phylink_sfp_module_insert(void *upstream,
>>  
>>  	config.interface = iface;
>>  	ret = phylink_validate(pl, support, &config);
>> +
>> +	if (ret && iface == PHY_INTERFACE_MODE_SGMII &&
>> +	    phylink_test(orig_support, 1000baseX_Full)) {
>> +		/* Copper modules may select SGMII but the interface may not
>> +		 * support that mode, try 1000BaseX if supported.
>> +		 */
> 
> Here, you are talking about what the module itself supports, but this
> code is determining what it should do based on what the _network
> controller_ supports.

orig_support is from just after sfp_parse_support is called, so it's
reflecting everything we think the module supports. The "net: sfp: Set
1000BaseX support flag for 1000BaseT modules" patch adds 1000BaseX to
that list for copper modules, which allows this code to detect that
1000BaseX might be a possibility.

> 
> If the SFP module is programmed for SGMII, and the network controller
> supports 1000base-X, then it isn't going to work very well - the
> sender of the control word will be sending one format, and the
> receiver will be interpreting the bits wrongly.

Agreed, but as I mentioned above, it doesn't appear that there's any
sensible way to avoid that. Without this patch, both SGMII and 1000BaseX
copper modules would fail in a 1000BaseX-only controller. With this
patch in place, the 1000BaseX module will work.

-- 
Robert Hancock
Senior Software Developer
SED Systems, a division of Calian Ltd.
Email: hancock@...systems.ca