netdev - net: phylink: phylink_helper_basex_speed issues with 1000base-x

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

Date:   Thu, 25 Mar 2021 11:36:26 -0500
From:   George McCollister <george.mccollister@...il.com>
To:     netdev@...r.kernel.org
Cc:     Russell King <rmk+kernel@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>
Subject: net: phylink: phylink_helper_basex_speed issues with 1000base-x

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. The Ethernet adapter connecting to this
switch port doesn't support 2500BASEX so it never establishes a link.
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?

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.

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.

I've been thinking through how to get the desired behavior but I'm not
even sure what the desired behavior is. If you set phy-mode to
"1000base-x" in device-tree do you ever want interface to be set to
PHY_INTERFACE_MODE_2500BASEX? If so just for MLO_AN_INBAND or also for
ML_AN_FIXED? Do we want phylink_validate called in phylink_create even
though it gets called anyway for MLO_AN_INBAND and ML_AN_FIXED later?

Regards,
George McCollister