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: <20210401211721.hsrqn3gerz4nlwgk@skbuf>
Date:   Fri, 2 Apr 2021 00:17:21 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     George McCollister <george.mccollister@...il.com>
Cc:     netdev@...r.kernel.org, Russell King <rmk+kernel@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: net: phylink: phylink_helper_basex_speed issues with 1000base-x

Hi George,

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

I think these are good and valid questions. If I had a good justification
for the current phylink behavior I would have answered them, but I don't.