Re: [PATCH net-next v2 3/4] net: dsa: add support for offloading HSR

On Tue, Feb 9, 2021 at 12:51 PM Vladimir Oltean [off-list ref] wrote:

On Tue, Feb 09, 2021 at 12:37:38PM -0600, George McCollister wrote:

quoted

On Tue, Feb 9, 2021 at 11:20 AM Vladimir Oltean [off-list ref] wrote:

quoted

On Mon, Feb 08, 2021 at 11:21:26AM -0600, George McCollister wrote:

quoted

quoted

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.

Will do.

quoted

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:

Yup, the switch has provisions to deal with this.

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.

quoted

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.

Thanks for clearing that up and explaining how this will work in the future.