netdev - Re: mv88e6240 configuration broken for B850v3

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211207143705.46fd2zavniwb3gri@skbuf>
Date:   Tue, 7 Dec 2021 16:37:05 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Martyn Welch <martyn.welch@...labora.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, kernel@...labora.com
Subject: 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.