Message-ID: <20210209185119.a4zptdw2xxufzkxp@skbuf>
Date:   Tue, 9 Feb 2021 20:51:19 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     George McCollister <george.mccollister@...il.com>
Cc:     Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jonathan Corbet <corbet@....net>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 3/4] net: dsa: add support for offloading HSR

On Tue, Feb 09, 2021 at 12:37:38PM -0600, George McCollister wrote:
> On Tue, Feb 9, 2021 at 11:20 AM Vladimir Oltean <olteanv@...il.com> wrote:
> >
> > On Mon, Feb 08, 2021 at 11:21:26AM -0600, George McCollister wrote:
> > > > If you return zero, the software fallback is never going to kick in.
> > >
> > > For join and leave? How is this not a problem for the bridge and lag
> > > functions? They work the same way don't they? I figured it would be
> > > safe to follow what they were doing.
> >
> > I didn't say that the bridge and LAG offloading logic does the right
> > thing, but it is on its way there...
> >
> > Those "XXX not offloaded" messages were tested with cases where the
> > .port_lag_join callback _is_ present, but fails (due to things like
> > incompatible xmit hashing policy). They were not tested with the case
> > where the driver does not implement .port_lag_join at all.
> >
> > Doesn't mean you shouldn't do the right thing. I'll send some patches
> > soon, hopefully, fixing that for LAG and the bridge, you can concentrate
> > on HSR. For the non-offload scenario where the port is basically
> > standalone, we also need to disable the other bridge functions such as
> > address learning, otherwise it won't work properly, and that's where
> > I've been focusing my attention lately. You can't offload the bridge in
> > software, or a LAG, if you have address learning enabled. For HSR it's
> > even more interesting, you need to have address learning disabled even
> > when you offload the DANH/DANP.
> 
> Do I just return -EOPNOTSUPP instead of 0 in dsa_switch_hsr_join and
> dsa_switch_hsr_leave?

Yes, return -EOPNOTSUPP if the callbacks are not implemented please.

> I'm not sure exactly what you're saying needs to be done wrt to
> address learning with HSR. The switch does address learning
> internally. Are you saying the DSA address learning needs to be
> disabled?

I'm saying that when you're doing any sort of redundancy protocol, the
switch will get confused by address learning if it performs it at
physical port level, because it will see the same source MAC address
coming in from 2 (or more) different physical ports. And when it sees a
packet coming in through a port that it had already learned it should be
the destination for the MAC address because an earlier packet came
having that MAC address as source, it will think it should do
hairpinning which it's configured not to => it'll drop the packet.

Now, your switch might have some sort of concept of address learning at
logical port level, where the "logical port" would roughly correspond to
the hsr0 upper (I haven't opened the XRS700x manual to know if it does
this, sorry). Basically if you support RedBox I expect that the switch
is able to learn at the level of "this MAC address came from the HSR
ring, aka from one or both of the individual ring ports". But for
configuring that in Linux, you'd need something like:

ip link set hsr0 master br0
ip link set hsr0 type bridge_slave learning on

and then catch from DSA the switchdev notification emitted for hsr0, and
use that to configure learning on the logical port of your switch
corresponding to hsr0, instead of learning on the physical ports that
offload it.

There are similar issues related to address learning for everything
except a bridge port, basically.

> If that's something I need for this patch some tips on what
> to do would be appreciated because I'm a bit lost.

I didn't say you need to change something related to learning for this
series, because you can't anyway - DSA doesn't give you the knobs to
configure address learning yet. The series where I try to add those are
here:
https://patchwork.kernel.org/project/netdevbpf/cover/20210209151936.97382-1-olteanv@gmail.com/

The take-away is that with those changes, a DSA driver should start its
ports in standalone mode with learning disabled and flooding of all
kinds enabled, and then treat the .port_bridge_flags callback which
should do the right thing and enable/disable address learning only when
necessary.

All I said is that address learning remaining enabled has been an issue
that prevented the non-offload scenarios from really working, but you
shouldn't be too hung up on that, and still return -EOPNOTSUPP, thereby
allowing the software fallback to kick in, even if it doesn't work.

Powered by blists - more mailing lists