Re: mv88e6240 configuration broken for B850v3

Vladimir Oltean
2021-12-07T14:37:11Z

On Tue, Dec 07, 2021 at 01:59:03PM +0000, Russell King (Oracle) wrote:
> The change that I have proposed is based on two patches:
>
> 1) Change mv88e6xxx_port_ppu_updates() to behave sensibly for the 6250
> family of DSA switches.
>
> 2) Un-force the link-down in mv88e6xxx_mac_config()
>
> This gives us more consistency. The checks become:
>
> a) mac_link_down():
>
> if ((!mv88e6xxx_port_ppu_updates(chip, port) ||
> mode == MLO_AN_FIXED) && ops->port_sync_link)
> err = ops->port_sync_link(chip, port, mode, false);
>
> b) mac_link_up():
>
> if (!mv88e6xxx_port_ppu_updates(chip, port) ||
> mode == MLO_AN_FIXED) {
> ...
> if (ops->port_sync_link)
> err = ops->port_sync_link(chip, port, mode, true);
> }
>
> c) mac_config():
>
> if (chip->info->ops->port_set_link &&
> ((mode == MLO_AN_INBAND && p->interface != state->interface) ||
> (mode == MLO_AN_PHY && mv88e6xxx_port_ppu_updates(chip, port))))
> chip->info->ops->port_set_link(chip, port, LINK_UNFORCED);
>
> The "(mode == MLO_AN_INBAND && p->interface != state->interface)"
> expression comes from the existing code, so isn't something that
> needs discussion.
>
> We want to preserve the state of the link-force for MLO_AN_FIXED,
> so the only case for discussion is the new MLO_AN_PHY. It can be
> seen that all three methods above end up using the same decision,
> and essentially if mv88e6xxx_port_ppu_updates() is true, meaning
> there is a non-software method to handle the status updates, then
> we (a) don't do any forcing in the mac_link_*() methods and turn
> off the forcing in mac_config().

Ok, so if we are in:
- MLO_AN_PHY (internal or external) and PPU enabled, we're letting the link unforced in mac_config
- MLO_AN_PHY (internal or external) and PPU disabled, we're forcing the link up in mac_link_up,
via the "!mv88e6xxx_port_ppu_updates()" check.
- MLO_AN_FIXED, we're also forcing the link up in mac_link_up, via the
"mode == MLO_AN_FIXED" check.

So in Martyn's case, a CPU port internal PHY link which is forced down
in mac_link_down would either be forced up or unforced, but never be
left the way in which mac_link_down put it. That is good and appears
robust.

Although it isn't strictly true that "[ if ] there is a non-software
method to handle the status updates, then we don't do any forcing in
the mac_link_*() methods", because technically we still do in
mac_link_down, due to the " || mode == MLO_AN_FIXED" condition plus the
way in which we are called, it's just that we undo it later.