netdev - Re: net: phylink: phylink_helper_basex

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: <20210401223355.GA1463@shell.armlinux.org.uk>
Date:   Thu, 1 Apr 2021 23:33:55 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     George McCollister <george.mccollister@...il.com>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>
Subject: Re: net: phylink: phylink_helper_basex_speed issues with 1000base-x

Hi,

I hadn't responded earlier because I wanted to think about it more,
but then forgot about this email.

On Thu, Mar 25, 2021 at 11:36:26AM -0500, George McCollister wrote:
> When I set port 9 on an mv88e6390, a cpu facing port to use 1000base-x
> (it also supports 2500base-x) in device-tree I find that
> phylink_helper_basex_speed() changes interface to
> PHY_INTERFACE_MODE_2500BASEX.

If both 2500base-X and 1000base-X are reported as being advertised,
then yes, it will. This is to support SFPs that can operate in either
mode. The key thing here is that both speeds are being advertised
and we're in either 2500base-X or 1000base-X mode.

This gives userspace a way to switch between those two supported modes
on the SFP.

> The Ethernet adapter connecting to this
> switch port doesn't support 2500BASEX so it never establishes a link.

You mean the remote side only supports 1000base-X?

> If I hack up the code to force PHY_INTERFACE_MODE_1000BASEX it works
> fine.
> 
> state->an_enabled is true when phylink_helper_basex_speed() is called
> even when configured with fixed-link. This causes it to change the
> interface to PHY_INTERFACE_MODE_2500BASEX if 2500BaseX_Full is in
> state->advertising which it always is on the first call because
> phylink_create calls bitmap_fill(pl->supported,
> __ETHTOOL_LINK_MODE_MASK_NBITS) beforehand. Should state->an_enabled
> be true with MLO_AN_FIXED?

Historically, it has been (by the original fixed-phy implementation)
and I don't wish to change that as that would be a user-visible
change. Turning off state->an_enabled will make the interface depend
on state->speed instead.

> I've also noticed that phylink_validate (which ends up calling
> phylink_helper_basex_speed) is called before phylink_parse_mode in
> phylink_create. If phylink_helper_basex_speed changes the interface
> mode this influences whether phylink_parse_mode (for MLO_AN_INBAND)
> sets 1000baseX_Full or 2500baseX_Full in pl->supported (which is then
> copied to pl->advertising). phylink_helper_basex_speed is then called
> again (via phylink_validate) which uses advertising to decide how to
> set interface. This seems like circular logic.

I'm wondering if we should postpone the initial call to
phylink_validate() to just before the "pl->cur_link_an_mode =
pl->cfg_link_an_mode;" in there, and only if we're still in MLO_AN_PHY
mode - it will already have been called via the other methods. Would
that help to solve the problem?

> To make matters even more confusing I see that
> mv88e6xxx_serdes_dcs_get_state uses state->interface to decide whether
> to set state->speed to SPEED_1000 or SPEED_2500.

There is no real report from the hardware to indicate the speed -
2500base-X looks like 1000base-X except for the different interface
mode.

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