On Tue, Feb 09, 2021 at 11:04, George McCollister <george.mccollis...@gmail.com> wrote: > On Tue, Feb 9, 2021 at 8:38 AM Tobias Waldekranz <tob...@waldekranz.com> > wrote: >> >> On Mon, Feb 08, 2021 at 15:09, George McCollister >> <george.mccollis...@gmail.com> wrote: >> > On Mon, Feb 8, 2021 at 2:16 PM Tobias Waldekranz <tob...@waldekranz.com> >> > wrote: >> >> >> >> On Thu, Feb 04, 2021 at 15:59, George McCollister >> >> <george.mccollis...@gmail.com> wrote: >> >> > 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." > >> >> >> 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. >> >> > 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 :) >> >> Is the lack of that table the reason for enforcing that the SA match the >> HSR netdev? > > Could be. > >> >> >> 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. >> >> >> br0 >> >> / \ >> >> hsr0 \ >> >> / \ \ >> >> swp1 swp2 swp3 >> >> >> >> Or are there some additional semantics involved in forwarding between >> >> the redundant ports and the interlink? >> > >> > That sounds right. >> > >> >> >> >> 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 :) >> >> >> 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 :) >> >> > 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. > >> >> > I don't think the xrs7000 series could offload HSR v0 anyway because >> > the tag ether type is different. >> > >> >> >> >> 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.
