Re: [net-next PATCH v5 1/4] net: hsr: Provide RedBox support (HSR-SAN)

[Lukasz Majewski <lukma@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 11055 bytes --]

Hi Casper,

> On 2024-04-19 12:42 +0200, Lukasz Majewski wrote:
> > Hi Casper,
> >  
> >> On 2024-04-18 17:37 +0200, Lukasz Majewski wrote:
> >> Hi Lukasz,
> >>   
> >> > Hi Casper,
> >> >    
> >> >> Hi,
> >> >> 
> >> >> Sorry for the late reply, I was awaiting confirmation on what I
> >> >> can say about the hardware I have access to. They won't let me
> >> >> say the name :( but I can give some details.    
> >> >
> >> > Ok, good :-)
> >> >
> >> > At least I'm not alone and there is another person who can
> >> > validate the code (or behaviour) on another HSR HW.
> >> >
> >> > (Some parts of the specification could be double checked on
> >> > another HW as well).
> >> >    
> >> >> 
> >> >> On 2024-04-16 15:03 +0200, Lukasz Majewski wrote:    
> >> >> >> On 2024-04-02 10:58 +0200, Lukasz Majewski wrote:      
> >> >> >> > Changes for v3:
> >> >> >> >
> >> >> >> > - Modify frame passed Port C (Interlink) to have RedBox's
> >> >> >> > source address (SA) This fixes issue with connecting L2
> >> >> >> > switch to Interlink Port as switches drop frames with SA
> >> >> >> > other than one registered in their (internal) routing
> >> >> >> > tables. 
> >> >> >>       
> >> >> >> > +	/* When HSR node is used as RedBox - the frame
> >> >> >> > received from HSR ring
> >> >> >> > +	 * requires source MAC address (SA) replacement to
> >> >> >> > one which can be
> >> >> >> > +	 * recognized by SAN devices (otherwise, frames
> >> >> >> > are dropped by switch)
> >> >> >> > +	 */
> >> >> >> > +	if (port->type == HSR_PT_INTERLINK)
> >> >> >> > +		ether_addr_copy(eth_hdr(skb)->h_source,
> >> >> >> > +
> >> >> >> > port->hsr->macaddress_redbox);   
> >> >> >> 
> >> >> >> I'm not really understanding the reason for this change. Can
> >> >> >> you explain it in more detail?      
> >> >> >
> >> >> > According to the HSR standard [1] the RedBox device shall work
> >> >> > as a "proxy" [*] between HSR network and SAN (i.e. "normal"
> >> >> > ethernet) devices.
> >> >> >
> >> >> > This particular snippet handles the situation when frame from
> >> >> > HSR node is supposed to be sent to SAN network. In that case
> >> >> > the SA of HSR (SA_A) is replaced with SA of RedBox (SA_RB) as
> >> >> > the MAC address of RedBox is known and used by SAN devices.
> >> >> >
> >> >> >
> >> >> > Node A  hsr1  |======| hsr1 Node Redbox |   |
> >> >> > (SA_A) [**]   |	     |           eth3   |---| ethX SAN
> >> >> > 	      |      |        	 (SA_RB)|   |  (e.g
> >> >> > switch)
> >> >> >
> >> >> >
> >> >> > (the ====== represents duplicate link - like lan1,lan2)
> >> >> >
> >> >> > If the SA_A would be passed to SAN (e.g. switch) the switch
> >> >> > could get confused as also RedBox MAC address would be used.
> >> >> > Hence, all the frames going out from "Node Redbox" have SA
> >> >> > set to SA_RB.
> >> >> >
> >> >> > According to [1] - RedBox shall have the MAC address.
> >> >> > This is similar to problem from [2].      
> >> >> 
> >> >> Thanks for the explanation, but I still don't quite follow in
> >> >> what way the SAN gets confused. "also RedBox MAC address would
> >> >> be used", when does this happen? Do you mean that some frames
> >> >> from Node A end up using the RedBox MAC address so it's best if
> >> >> they all do?    
> >> >
> >> > The SAN (let's say it is a switch) can communicate with RedBox or
> >> > Node A. In that way the DA is different for both (so SA on reply
> >> > is also different). On my setup I've observed frames drop (caused
> >> > probably by switch filtering of incoming traffic not matching the
> >> > outgoing one).
> >> >
> >> > When I only use SA of RedBox on traffic going to SAN, the
> >> > problem is gone.
> >> >
> >> > IMHO, such separation (i.e. to use only RedBox's SA on traffic
> >> > going to SAN) is the "proxy" mentioned in the standard.
> >> >    
> >> >> 
> >> >> I see there is already some address replacement going on in the
> >> >> HSR interface, as you pointed out in [2]. And I get your idea
> >> >> of being a proxy. If no one else is opposed to this then I'm
> >> >> fine with it too.   
> >> >
> >> > Ok.
> >> >    
> >> >> >> The standard does not say to modify the
> >> >> >> SA. However, it also does not say to *not* modify it in
> >> >> >> HSR-SAN mode like it does in other places. In HSR-HSR and
> >> >> >> HSR-PRP mode modifying SA breaks the duplicate discard.      
> >> >> >
> >> >> > IMHO, the HSR-SAN shall be regarded as a "proxy" [*] between
> >> >> > two types (and not fully compatible) networks.
> >> >> >      
> >> >> >> So keeping the same behavior for all
> >> >> >> modes would be ideal.
> >> >> >> 
> >> >> >> I imagine any HW offloaded solutions will not modify the SA,
> >> >> >> so if possible the SW should also behave as such.      
> >> >> >
> >> >> > The HW offloading in most cases works with HSR-HSR setup
> >> >> > (i.e. it duplicates frames automatically or discards them
> >> >> > when recived - like ksz9477 [3]).
> >> >> >
> >> >> > I think that RedBox HW offloading would be difficult to
> >> >> > achieve to comply with standard. One "rough" idea would be to
> >> >> > configure aforementioned ksz9477 to pass all frames in its HW
> >> >> > between SAN and HSR network (but then it wouldn't filter
> >> >> > them).      
> >> >> 
> >> >> I don't know anything about ksz9477. The hardware I have access
> >> >> to is supposed to be compliant with 2016 version in an offloaded
> >> >> situation for all modes (HSR-SAN, PRP-SAN, HSR-PRP, HSR-HSR).
> >> >>  
> >> >
> >> > Hmm... Interesting.
> >> >
> >> > As fair as I know - the ksz9477 driver from Microchip for RedBox
> >> > sets internal (i.e. in chip) vlan for Node_A, Node_B and
> >> > Interlink, so _all_ packets are flowing back and forth between
> >> > HSR and SAN networks ....   
> >> >> Though, I haven't
> >> >> verified if the operation is fully according to standard.    
> >> >
> >> > You may use wireshark on device connected as SAN to redbox and
> >> > then see if there are any frames (especially supervisory ones)
> >> > passed from HSR network.    
> >> 
> >> I realized I should clarify, what I'm running is non-upstream
> >> software.  
> >
> > Ok.
> >  
> >> And by offloaded I mean the redbox forwarding is
> >> offloaded. Supervision frames are still handled in SW and only
> >> sent on HSR/PRP ports, and doesn't reach any SAN nodes. Basic
> >> operation works as it should.  
> >
> > Ok.
> >  
> >>   
> >> >> It does not
> >> >> modify any addresses in HW.    
> >> >
> >> > By address - you mean the MAC addresses of nodes?    
> >> 
> >> I mean that it forwards all frames without modification (except
> >> HSR/PRP and VLAN tags). It does not update SMAC with the proxy MAC
> >> like your implementation does.  
> >
> > Hmm... I'm wondering how "proxy" is implemented then.
> > Also, what is the purpose of ProxyNodeTable in that case?  
> 
> The ProxyNodeTable becomes the same as the MAC table for the interlink
> port. I.e. normal MAC learning, when a frame is sent by a SAN and
> received on interlink the HW learns that that SMAC is on the interlink
> port (until it ages out). This table can be read out and used for
> supervision frames.

Yes, this is how this patch handles it.

> 
> Though, the NodesTable I don't think is used in HW. As I understand
> it's an optional feature.

Yes.

> 
> >>   
> >> >> Does it call
> >> >> port_hsr_join and try to join as an HSR port?     
> >> >
> >> > No, not yet.
> >> >
> >> > The community (IIRC Vladimir Oltean) suggested to first implement
> >> > the RedBox Interlink (HSR-SAN) in SW. Then, we may think about
> >> > adding offloading support for it.
> >> >    
> >> >> Do we maybe need a
> >> >> separate path or setting for configuring the interlink in the
> >> >> different modes (SAN, HSR, PRP interlink)?    
> >> >
> >> > I think that it shall be handled as an extra parameter (like we
> >> > do have now with 'supervision' or 'version') in ip link add.
> >> >
> >> > However, first I would like to have the "interlink" parameter
> >> > added to iproute2 and then we can extend it to other modes if
> >> > requred.    
> >> 
> >> Alright, doing SW implementation first sounds good. From userspace
> >> it can probably be an extra parameter. But for the driver
> >> configuration maybe we want a port_interlink_join? (when it comes
> >> to implementing that).  
> >
> > IMHO, having port_interlink_join() may be useful in the future to
> > provide offloading support.
> >  
> >> 
> >> 
> >> I did some testing with veth interfaces (everything in SW) with
> >> your patches. I tried to do a setup like yours
> >>                 
> >>                   +-vethA---vethB-+
> >>                   |               |
> >> vethF---vethE---hsr0             hsr1
> >>                   |               |
> >>                   +-vethC---vethD-+
> >> 
> >> Sending traffic from vethF results in 3 copies being seen on the
> >> ring ports. One of which ends up being forwarded back to vethF
> >> (with SMAC updated to the proxy address). I assume this is not
> >> intended behavior.  
> >
> > I've reported this [2] (i.e. duplicated packets on HSR network with
> > veth) when I was checking hsr_ping.sh [1] script for regression.
> >
> > (However, I don't see the DUP pings on my KSZ9477 setup).
> >
> >   
> >> 
> >> Setup:
> >> ip link add dev vethA type veth peer name vethB
> >> ip link add dev vethC type veth peer name vethD
> >> ip link add dev vethE type veth peer name vethF
> >> ip link set up dev vethA
> >> ip link set up dev vethB
> >> ip link set up dev vethC
> >> ip link set up dev vethD
> >> ip link set up dev vethE
> >> ip link set up dev vethF
> >> 
> >> ip link add name hsr0 type hsr slave1 vethA slave2 vethC interlink
> >> vethE supervision 45 version 1 ip link add name hsr1 type hsr
> >> slave1 vethB slave2 vethD supervision 45 version 1 ip link set dev
> >> hsr0 up ip link set dev hsr1 up
> >> 
> >> I used Nemesis to send random UDP broadcast packets but you could
> >> use whatever: nemesis udp -d vethF -c 10000 -i 1   
> >
> > Ok, I will check nemesis load as well.  
> 
> Nemesis doesn't do anything specific, just generates packets. The
> command above sends a packet at 1 second intervals.
> 
> > Can you check the hsr_redbox.sh (from this patch set) and
> > hsr_ping.sh ?  
> 
> Running in SW I get the same results as you, hsr_redbox.sh passes and
> hsr_ping.sh fails.

Ok.

> 
> I haven't tried on HW. I'll see if I can find some time for it but it
> might take more time to prepare.

Ok. Thanks for help.

> 
> BR,
> Casper


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]