net: phylink: phylink_helper_basex_speed issues with 1000base-x

net: phylink: phylink_helper_basex_speed issues with 1000base-x

[George McCollister]

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

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

[Vladimir Oltean]

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.

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

[Russell King - ARM Linux admin]

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!

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

[George McCollister]

On Thu, Apr 1, 2021 at 5:33 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> 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?

Yes, the Ethernet controller on the board that connects to port 9 on
the mv88e6390 doesn't support 2500base-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?

I had wondered if precisely this would fix it. I tested it and it
does. The question I can't answer is will it break anything else?
Should I send a patch?

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