Message-ID: <20210117123744.erw2i34oap5xkapo@skbuf>
Date:   Sun, 17 Jan 2021 14:37:44 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     "David S . Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Tobias Waldekranz <tobias@...dekranz.com>,
        Maxim Kochetkov <fido_max@...ox.ru>
Subject: Re: [PATCH v2 net-next 01/14] net: mscc: ocelot: allow offloading of
 bridge on top of LAG

On Sat, Jan 16, 2021 at 05:26:23PM -0800, Jakub Kicinski wrote:
> On Sat, 16 Jan 2021 02:59:30 +0200 Vladimir Oltean wrote:
> > From: Vladimir Oltean <vladimir.oltean@....com>
> >
> > Commit 7afb3e575e5a ("net: mscc: ocelot: don't handle netdev events for
> > other netdevs") was too aggressive, and it made ocelot_netdevice_event
> > react only to network interface events emitted for the ocelot switch
> > ports.
> >
> > In fact, only the PRECHANGEUPPER should have had that check.
> >
> > When we ignore all events that are not for us, we miss the fact that the
> > upper of the LAG changes, and the bonding interface gets enslaved to a
> > bridge. This is an operation we could offload under certain conditions.
>
> I see the commit in question is in net, perhaps worth spelling out why
> this is not a fix? Perhaps add some "in the future" to the last
> sentence if it's the case that this will only matter with the following
> patches applied?

It is a fix. However, so is patch 13/14 "net: mscc: ocelot: rebalance
LAGs on link up/down events", but I didn't see an easy way to backport
that. Honestly the reasons why I did not attempt to split this series
into a part for "net" and one for "net-next" are:
(a) It would unnecessarily complicate my work for felix DSA, where this
    is considered a new feature as opposed to ocelot switchdev where it
    was supposedly already working (although.. not quite, due to the
    lack of rebalancing, a link down would throw off the LAG).
    I don't really think that anybody was seriously using LAG offload on
    ocelot so far.
(b) Even if I were to split this patch, it can only be trivially
    backported as far as commit 9c90eea310f8 ("net: mscc: ocelot: move
    net_device related functions to ocelot_net.c") from June 2020
    anyway.
(c) I cannot test the mscc_ocelot.ko switchdev driver with traffic,
    since I don't have the hardware (I just have a local patch that
    I keep rebasing on top of net-next which makes me able to at least
    probe it and access its registers on a different switch revision,
    but the traffic I/O procedure there is completely different). So I
    can not really confirm what is the state I'm leaving the mscc_ocelot
    driver in, for stable kernels. At least now, I've made the entry
    points into the control code path very similar to those of DSA, and
    I've exercised the switchdev driver in blind (without traffic), so I
    have a bit more confidence that it should work.
(d) Had the AUTOSEL guys picked up this patch, I would have probably had
    no objection (since my belief is that there's nothing to break and
    nothing to fix in stable kernels).

That being said, if we want to engage in a rigid demonstration of
procedures, sure we can do that. I have other patches anyway to fill the
pipeline until "net" is merged back into "net-next" :)

Powered by blists - more mailing lists