| Message ID | 20210204215926.64377-1-george.mccollister@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | add HSR offloading support for DSA switches | expand |
On Thu, Feb 04, 2021 at 15:59, George McCollister <george.mccollister@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 From the looks of it, this series only deals with the end-device use-case. Is that right? 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: br0 / \ hsr0 \ / \ \ swp1 swp2 swp3 Or are there some additional semantics involved in forwarding between the redundant ports and the interlink? 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. 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? 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. Thanks for working on this!
On Mon, Feb 8, 2021 at 2:16 PM Tobias Waldekranz <tobias@waldekranz.com> wrote: > > On Thu, Feb 04, 2021 at 15:59, George McCollister <george.mccollister@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. > > 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. 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. > > 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. > > 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. > > 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." 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. 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? > > Thanks for working on this!
On Mon, Feb 08, 2021 at 15:09, George McCollister <george.mccollister@gmail.com> wrote: > On Mon, Feb 8, 2021 at 2:16 PM Tobias Waldekranz <tobias@waldekranz.com> wrote: >> >> On Thu, Feb 04, 2021 at 15:59, George McCollister <george.mccollister@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? >> 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? > 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? Is the lack of that table the reason for enforcing that the SA match the HSR netdev? >> 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? >> 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. >> 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? > 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.
On Tue, Feb 9, 2021 at 8:38 AM Tobias Waldekranz <tobias@waldekranz.com> wrote: > > On Mon, Feb 08, 2021 at 15:09, George McCollister <george.mccollister@gmail.com> wrote: > > On Mon, Feb 8, 2021 at 2:16 PM Tobias Waldekranz <tobias@waldekranz.com> wrote: > >> > >> On Thu, Feb 04, 2021 at 15:59, George McCollister <george.mccollister@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. > > > 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. > > 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. > > >> 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. > > >> 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 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.
On Tue, Feb 09, 2021 at 11:04:08AM -0600, George McCollister wrote: > > >> 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 > 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. Even though I understand where this is coming from, I have grown to dislike overengineered solutions. DSA is there to standardize how an Ethernet-connected switch interacts with the network stack, not to be the middle man that tries to offer a lending hand everywhere. If there is a 50/50 chance that this extra logic in the DSA mid layer will not be needed for the second switch that offloads HSR/PRP, then I'd go with "don't do it". Just emit a hsr_join for both ports and let the driver deal with it.
On Tue, Feb 09, 2021 at 11:04, George McCollister <george.mccollister@gmail.com> wrote: > On Tue, Feb 9, 2021 at 8:38 AM Tobias Waldekranz <tobias@waldekranz.com> wrote: >> >> On Mon, Feb 08, 2021 at 15:09, George McCollister <george.mccollister@gmail.com> wrote: >> > On Mon, Feb 8, 2021 at 2:16 PM Tobias Waldekranz <tobias@waldekranz.com> wrote: >> >> >> >> On Thu, Feb 04, 2021 at 15:59, George McCollister <george.mccollister@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.
On Wed, Feb 10, 2021 at 10:10:14PM +0100, Tobias Waldekranz wrote: > On Tue, Feb 09, 2021 at 11:04, George McCollister <george.mccollister@gmail.com> wrote: > >> > 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 :) As far as I understand, the VDAN needs to generate supervision frames on behalf of all nodes that it proxies. Therefore, implementing the RedBox/QuadBox in the bridge is probably not practical. What I was discussing with George though is that maybe we can make hsr a consumer of SWITCHDEV_FDB_ADD_TO_DEVICE events, similar to DSA with its assisted_learning_on_cpu_port functionality, and that would be how it populates its proxy node table. A RedBox becomes a bridge with one HSR interface and one or more standalone interfaces, and a QuadBox becomes a bridge with two HSR interfaces. How does that sound?
On Wed, Feb 10, 2021 at 23:55, Vladimir Oltean <olteanv@gmail.com> wrote: > On Wed, Feb 10, 2021 at 10:10:14PM +0100, Tobias Waldekranz wrote: >> On Tue, Feb 09, 2021 at 11:04, George McCollister <george.mccollister@gmail.com> wrote: >> >> > 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 :) > > As far as I understand, the VDAN needs to generate supervision frames on > behalf of all nodes that it proxies. Therefore, implementing the > RedBox/QuadBox in the bridge is probably not practical. What I was > discussing with George though is that maybe we can make hsr a consumer > of SWITCHDEV_FDB_ADD_TO_DEVICE events, similar to DSA with its > assisted_learning_on_cpu_port functionality, and that would be how it > populates its proxy node table. Is it not easier to just implement learning in the HSR layer? Seeing as you need to look up the table for each packet anyway, you might as well add a new entry on a miss. Otherwise you run the risk of filling up your proxy table with entries that never egress the HSR device. Perhaps not likely on this particular device, but on a 48-port switch with HSR offloading it might be. This should also work for more exotic configs with multiple macvlans for example: macvlan0 macvlan1 \ / hsr0 / \ swp1 swp2 > A RedBox becomes a bridge with one HSR > interface and one or more standalone interfaces, and a QuadBox becomes a > bridge with two HSR interfaces. How does that sound? Yeah that is the straight forward solution, and what I tried to describe earlier in the thread with this illustration: >> >> br0 >> >> / \ >> >> hsr0 \ >> >> / \ \ >> >> swp1 swp2 swp3 I just wanted to double check that we had not overlooked a better solution outside of the existing HSR code.
On Sat, Feb 13, 2021 at 12:52:36AM +0100, Tobias Waldekranz wrote: > On Wed, Feb 10, 2021 at 23:55, Vladimir Oltean <olteanv@gmail.com> wrote: > > On Wed, Feb 10, 2021 at 10:10:14PM +0100, Tobias Waldekranz wrote: > >> 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 :) > > > > As far as I understand, the VDAN needs to generate supervision frames on > > behalf of all nodes that it proxies. Therefore, implementing the > > RedBox/QuadBox in the bridge is probably not practical. What I was > > discussing with George though is that maybe we can make hsr a consumer > > of SWITCHDEV_FDB_ADD_TO_DEVICE events, similar to DSA with its > > assisted_learning_on_cpu_port functionality, and that would be how it > > populates its proxy node table. > > Is it not easier to just implement learning in the HSR layer? Seeing as > you need to look up the table for each packet anyway, you might as well > add a new entry on a miss. Otherwise you run the risk of filling up your > proxy table with entries that never egress the HSR device. Perhaps not > likely on this particular device, but on a 48-port switch with HSR > offloading it might be. In the HSR layer, sure, I didn't mean to suggest otherwise, I thought you wanted to, when you said "I suppose an option would be to implement the RedBox/QuadBox roles in the bridge". So then the SWITCHDEV_FDB_ADD_TO_DEVICE events might be too much. Learning / populating the proxy node table can be probably done from the xmit function, with the only potential issue that the first packets will probably be lost, since no supervision frames have yet been transmitted for those proxied nodes. > This should also work for more exotic configs with multiple macvlans for > example: > > macvlan0 macvlan1 > \ / > hsr0 > / \ > swp1 swp2 Yes, I don't think macvlan uses switchdev. > > A RedBox becomes a bridge with one HSR > > interface and one or more standalone interfaces, and a QuadBox becomes a > > bridge with two HSR interfaces. How does that sound? > > Yeah that is the straight forward solution, and what I tried to describe > earlier in the thread with this illustration: > > >> >> br0 > >> >> / \ > >> >> hsr0 \ > >> >> / \ \ > >> >> swp1 swp2 swp3 > > I just wanted to double check that we had not overlooked a better > solution outside of the existing HSR code. I'm not aware of a better solution, but I'm also interested if there is one.