Re: mv88e6240 configuration broken for B850v3

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.