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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <Ya9op2QJwWRyqmDY@shell.armlinux.org.uk>
Date:   Tue, 7 Dec 2021 13:59:03 +0000
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Vladimir Oltean <olteanv@...il.com>
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 03:24:13PM +0200, Vladimir Oltean wrote:
> On Tue, Dec 07, 2021 at 12:58:20AM +0000, Russell King (Oracle) wrote:
> > > Anyway, so be it. Essentially, what is the most frustrating is that I
> > > haven't been doing anything else for the past 4 hours, and I still
> > > haven't understood enough of what you're saying to even understand why
> > > it is _relevant_ to Martyn's report. All I understand is that you're
> > > also looking in that area of the code for a completely unrelated reason
> > > which you've found adequate to mention here and now.
> > 
> > I hope you realise that _your_ attitude here is frustrating and
> > inflamatory. This is _not_ a "completely unrelated reason" - this
> > is about getting the right solution to Martyn's problem. I thought
> > about doing another detailed reply, but when I got to the part
> > about you wanting to check 6390X, I discarded that reply and
> > wrote this one instead. You clearly have a total distrust for
> > everything that I write in any email, so I just won't bother.
> 
> I think getting the right solution to Martyn's problem is the most
> important part. I'd be very happy if I understood it too, although that
> isn't mandatory, since it may be beyond me but still correct, and I hope
> I'm not standing in the way if that is the case.
> I've explained my current state of understanding, which is that I saw in
> the manual for 6097 that forcing the link is unnecessary for internal
> PHY ports regardless of whether the PPU is enabled or not. Yet, DSA will
> still force these ports down with your proposed change (because DSA lies
> that it is a MLO_AN_FIXED mode despite what's in the device tree), and
> it relies on unforcing them in mv88e6xxx_mac_config(), which in itself
> makes sense considering the other discussion about kexec and such. But
> it appears that it may not unforce them again - because that condition
> depends on mv88e6xxx_port_ppu_updates() which may be false. So if it is
> false, things are still broken.
> It isn't a matter of distrust and I'm sorry that you take it personal,
> but your proposed solution doesn't appear to me to have a clear
> correlation to the patch that introduced a regression.

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().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!