Re: [PATCH net-next v2 0/4] add HSR offloading support for DSA switches

On Tue, Feb 09, 2021 at 11:04, George McCollister [off-list ref] wrote:

On Tue, Feb 9, 2021 at 8:38 AM Tobias Waldekranz [off-list ref] wrote:

quoted

On Mon, Feb 08, 2021 at 15:09, George McCollister [off-list ref] wrote:

quoted

On Mon, Feb 8, 2021 at 2:16 PM Tobias Waldekranz [off-list ref] wrote:

quoted

On Thu, Feb 04, 2021 at 15:59, George McCollister [off-list ref] wrote:

quoted

Add support for offloading HSR/PRP (IEC 62439-3) tag insertion, tag
removal, forwarding and duplication on DSA switches.
This series adds offloading to the xrs700x DSA driver.

Changes since RFC:
 * Split hsr and dsa patches. (Florian Fainelli)

Changes since v1:
 * Fixed some typos/wording. (Vladimir Oltean)
 * eliminate IFF_HSR and use is_hsr_master instead. (Vladimir Oltean)
 * Make hsr_handle_sup_frame handle skb_std as well (required when offloading)
 * Don't add hsr tag for HSR v0 supervisory frames.
 * Fixed tag insertion offloading for PRP.

George McCollister (4):
  net: hsr: generate supervision frame without HSR/PRP tag
  net: hsr: add offloading support
  net: dsa: add support for offloading HSR
  net: dsa: xrs700x: add HSR offloading support

 Documentation/networking/netdev-features.rst |  21 ++++++
 drivers/net/dsa/xrs700x/xrs700x.c            | 106 +++++++++++++++++++++++++++
 drivers/net/dsa/xrs700x/xrs700x_reg.h        |   5 ++
 include/linux/if_hsr.h                       |  27 +++++++
 include/linux/netdev_features.h              |   9 +++
 include/net/dsa.h                            |  13 ++++
 net/dsa/dsa_priv.h                           |  11 +++
 net/dsa/port.c                               |  34 +++++++++
 net/dsa/slave.c                              |  14 ++++
 net/dsa/switch.c                             |  24 ++++++
 net/dsa/tag_xrs700x.c                        |   7 +-
 net/ethtool/common.c                         |   4 +
 net/hsr/hsr_device.c                         |  46 ++----------
 net/hsr/hsr_device.h                         |   1 -
 net/hsr/hsr_forward.c                        |  33 ++++++++-
 net/hsr/hsr_forward.h                        |   1 +
 net/hsr/hsr_framereg.c                       |   2 +
 net/hsr/hsr_main.c                           |  11 +++
 net/hsr/hsr_main.h                           |   8 +-
 net/hsr/hsr_slave.c                          |  10 ++-
 20 files changed, 331 insertions(+), 56 deletions(-)
 create mode 100644 include/linux/if_hsr.h

--
2.11.0

Hi George,

I will hopefully have some more time to look into this during the coming
weeks. What follows are some random thoughts so far, I hope you can
accept the windy road :)

Broadly speaking, I gather there are two common topologies that will be
used with the XRS chip: "End-device" and "RedBox".

End-device:    RedBox:
 .-----.       .-----.
 | CPU |       | CPU |
 '--+--'       '--+--'
    |             |
.---0---.     .---0---.
|  XRS  |     |  XRS  3--- Non-redundant network
'-1---2-'     '-1---2-'
  |   |         |   |
 HSR Ring      HSR Ring

There is also the HSR-HSR use case and HSR-PRP use case.

HSR-HSR is also known as a "QuadBox", yes? HSR-PRP is the same thing,
but having two PRP networks on one side and an HSR ring on the other?

Yes. I believe you are correct.
From the spec:
"QuadBox
Quadruple port device connecting two peer HSR rings, which behaves as
an HSR node in
each ring and is able to filter the traffic and forward it from ring to ring."

quoted

quoted

quoted

From the looks of it, this series only deals with the end-device
use-case. Is that right?

Correct. net/hsr doesn't support this use case right now. It will
stomp the outgoing source MAC with that of the interface for instance.

Good to know! When would that behavior be required? Presumably it is not
overriding the SA just for fun?

Over the last few weeks I've looked over that code for way longer than
I'd like to admit and I'm still not sure. As far as I can tell, the
original authors have disappeared. My guess is it has something to do
with a configuration in which they had each redundant interface set to
a different MAC address and wanted the frames to go out with the
associated MAC address. As far as I can tell this is a violation of
the spec.

Alright, so maybe a "Works for Me (TM)" solution initially, where any
devince stacking was out of scope.

quoted

quoted

It also doesn't implement a ProxyNodeTable (though that actually
wouldn't matter if you were offloading to the xrs700x I think). Try
commenting out the ether_addr_copy() line in hsr_xmit and see if it
makes your use case work.

So what is missing is basically to expand the current facility for
generating sequence numbers to maintain a table of such associations,
keyed by the SA?

For the software implementation it would also need to use the
ProxyNodeTable to prevent forwarding matching frames on the ring and
delivering them to the hsr master port. It's also supposed to drop
frames coming in on a redundant port if the source address is in the
ProxyNodeTable.

This whole thing sounds an awful lot like an FDB. I suppose an option
would be to implement the RedBox/QuadBox roles in the bridge, perhaps by
building on the work done for MRP? Feel free to tell me I'm crazy :)

quoted

Is the lack of that table the reason for enforcing that the SA match the
HSR netdev?

Could be.

quoted

quoted

quoted

I will be targeting a RedBox setup, and I believe that means that the
remaining port has to be configured as an "interlink". (HSR/PRP is still
pretty new to me). Is that equivalent to a Linux config like this:

Depends what you mean by configured as an interlink. I believe bit 9
of HSR_CFG in the switch is only supposed to be used for the HSR-HSR
and HSR-PRP use case, not HSR-SAN.

Interesting, section 6.4.1 of the XRS manual states: "The interlink port
can be either in HSR, PRP or normal (non-HSR, non-PRP) mode." Maybe the
term is overloaded?

Yeah I guess since it's the only port that can be used for QuadBox
they call it the interlink port even if it doesn't have the HSR/PRP
mode enabled with the interlink bit set.

Right, that makes sense.

quoted

quoted

quoted

      br0
     /   \
   hsr0   \
   /  \    \
swp1 swp2 swp3

Or are there some additional semantics involved in forwarding between
the redundant ports and the interlink?

That sounds right.

quoted

The chip is very rigid in the sense that most roles are statically
allocated to specific ports. I think we need to add checks for this.

Okay. I'll look into this. Though a lot of the restrictions have to do
with using the third gigabit port for an HSR/PRP interlink (not
HSR-SAN) which I'm not currently supporting anyway.

But nothing is stopping me from trying to setup an HSR ring between port
(2,3) or (1,3), right? And that is not supported by the chip as I
understand it from looking at table 25.

Yeah. That's why I said I'd look into it :). Wasn't an issue for my
board since port 0 isn't connected and port 3 is used as the CPU
facing port.

Fair enough :)

quoted

quoted

quoted

Looking at the packets being generated on the redundant ports, both
regular traffic and supervision frames seem to be HSR-tagged. Are
supervision frames not supposed to be sent with an outer ethertype of
0x88fb? The manual talks about the possibility of setting up a policy
entry to bypass HSR-tagging (section 6.1.5), is this what that is for?

This was changed between 62439-3:2010 and 62439-3:2012.
"Prefixing the supervision frames on HSR by an HSR tag to simplify the hardware
implementation and introduce a unique EtherType for HSR to simplify
processing."

Thank you, that would have taken me a long time to figure out :)

quoted

The Linux HSR driver calls the former HSR v0 and the later HSR v1. I'm
not sure what their intention was with this feature. The inbound
policies are pretty flexible so maybe they didn't have anything so
specific in mind.

Now that I think of it, maybe you want things like LLDP to still operate
hop-by-hop over the ring?

Not sure. Would need to look into it.

quoted

quoted

I don't think the xrs7000 series could offload HSR v0 anyway because
the tag ether type is different.

quoted

In the DSA layer (dsa_slave_changeupper), could we merge the two HSR
join/leave calls somehow? My guess is all drivers are going to end up
having to do the same dance of deferring configuration until both ports
are known.

Describe what you mean a bit more. Do you mean join and leave should
each only be called once with both hsr ports being passed in?

Exactly. Maybe we could use `netdev_for_each_lower_dev` to figure out if
the other port has already been switched over to the new upper or
something. I find it hard to believe that there is any hardware out
there that can do something useful with a single HSR/PRP port anyway.

If one port failed maybe it would still be useful to join one port if
the switch supported it? Maybe this couldn't ever happen anyway due
the way hsr is designed.

How were you thinking this would work? Would it just not use
dsa_port_notify() and call a switch op directly after the second
port's dsa_slave_changeupper() call? Or would we instead keep port

Yeah this is what I was thinking. But I understand where Vladimir is
coming from. Fundamentally, I am also on the library side of the
"library vs. framework" spectrum.

notifiers and calls to dsa_switch_hsr_join for each port and just make
dsa_switch_hsr_join() not call the switch op to create the HSR until
the second port called it? I'm not all that familiar with how these
dsa notifiers work and would prefer to stick with using a similar
mechanism to the bridge and lag support. It would be nice to get some
feedback from the DSA maintainers on how they would prefer it to work
if they indeed had a preference at all.