Message-ID: <20230905172013.7234d653@wsk>
Date:   Tue, 5 Sep 2023 17:20:13 +0200
From:   Lukasz Majewski <lukma@...x.de>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Eric Dumazet <edumazet@...gle.com>, Andrew Lunn <andrew@...n.ch>,
        davem@...emloft.net, Paolo Abeni <pabeni@...hat.com>,
        Woojung Huh <woojung.huh@...rochip.com>,
        Tristram.Ha@...rochip.com, Florian Fainelli <f.fainelli@...il.com>,
        Jakub Kicinski <kuba@...nel.org>, UNGLinuxDriver@...rochip.com,
        George McCollister <george.mccollister@...il.com>,
        Oleksij Rempel <o.rempel@...gutronix.de>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 RFC 3/4] net: dsa: hsr: Enable in KSZ9477 switch HW
 HSR offloading

Hi Vladimir,

> On Tue, Sep 05, 2023 at 01:11:03PM +0200, Lukasz Majewski wrote:
> > > > +/* The KSZ9477 provides following HW features to accelerate
> > > > + * HSR frames handling:
> > > > + *
> > > > + * 1. TX PACKET DUPLICATION FROM HOST TO SWITCH
> > > > + * 2. RX PACKET DUPLICATION DISCARDING
> > > > + * 3. PREVENTING PACKET LOOP IN THE RING BY SELF-ADDRESS
> > > > FILTERING
> > > > + *
> > > > + * Only one from point 1. has the NETIF_F* flag available.
> > > > + *
> > > > + * Ones from point 2 and 3 are "best effort" - i.e. those will
> > > > + * work correctly most of the time, but it may happen that some
> > > > + * frames will not be caught. Hence, the SW needs to handle
> > > > those
> > > > + * special cases. However, the speed up gain is considerable
> > > > when
> > > > + * above features are used.
> > > > + *
> > > > + * Moreover, the NETIF_F_HW_HSR_FWD feature is also enabled,
> > > > as HSR frames
> > > > + * can be forwarded in the switch fabric between HSR ports.    
> > > 
> > > How do these 2 concepts (autonomous forwarding + software-based
> > > elimination of some frames) work together? If software is not the
> > > sole receiver of traffic which needs to be filtered further, and
> > > duplicates also get forwarded to the network, does this not break
> > > the HSR ring? 
> > 
> > Autonomous forwarding is based on KSZ9477, having the HSR ports
> > "bridged" to send frames between them.
> > 
> > Then, there is also based on HSR tag, and SA in-KSZ9477 feature RX
> > packet duplication discarding which will discard duplicated frames.
> > 
> > Last but not least the - packet loop prevention.
> > 
> > My understanding is as follows:
> > 
> > 1. RX packet duplication removes copy of a frame, which is
> > addressed to cpu port of switch.  
> 
> Does the duplicate elimination function only do that, as you say, or
> is it supposed to also eliminate duplicates for packets flooded to 2
> ports as well (the host port, and the other ring port)?
> 

In the "RX PACKET DUPLICATION DISCARDING" (from [1]) the hash of DA and
SA and seq number is used to assess if received frame is a duplicated.

----8<-------
If a matching (based on above??) frame has already been received on the
other port, the frame is dropped. If not, then standard forwarding rules
apply. 
---->8-------

> If the latter, then it will fail to eliminate duplicates for packets
> that didn't visit the CPU. So the ring will rely on the self-address
> filtering capability of the other devices, in order not to collapse.
> I see that the software implementation also offers self-address
> filtering in hsr_handle_frame() -> hsr_addr_is_self(), but I don't see
> anything making that mandatory in IEC 62439-3:2018. Can we assume that
> the other ring members will know how to deal with it?
> 
> > 2. The "bridge" of HSR passes frames in-KSZ9477, which are not
> > addressed to this cpu host (between other HSR nodes).  
> 
> How is the switch supposed to know which packets are addressed to this
> CPU port and which are not? I expect separate answers for unicast,
> multicast and broadcast.
> 
> > 3. Packet loop prevention - the HSR packet with SA of note which
> > sent it - is not further forwarded.
> >   
> > > What are the causes due to which self-address filtering and
> > > duplicate elimination only work "most of the time"?  
> > 
> > Please refer to section "KSZ9477 CHIP LIMITATIONS" in:
> > https://ww1.microchip.com/downloads/en/Appnotes/AN3474-KSZ9477-High-Availability-Seamless-Redundancy-Application-Note-00003474A.pdf
> >  
> 
> Ok, so the limitation is a race condition in hardware such that, when
> duplicate packets are received on member ports very close in time to
> each other, the hardware fails to detect that they're duplicates.

+1.

> 
> > > > +	/* Enable discarding of received HSR frames */
> > > > +	ksz_read8(dev, REG_HSR_ALU_CTRL_0__1, &data);
> > > > +	data |= HSR_DUPLICATE_DISCARD;
> > > > +	data &= ~HSR_NODE_UNICAST;
> > > > +	ksz_write8(dev, REG_HSR_ALU_CTRL_0__1, data);
> > > > +
> > > > +	/* Self MAC address filtering for HSR frames to avoid
> > > > +	 * traverse of the HSR ring more than once.
> > > > +	 *
> > > > +	 * The HSR port (i.e. hsr0) MAC address is used.
> > > > +	 */
> > > > +	for (i = 0; i < ETH_ALEN; i++) {
> > > > +		ret = ksz_write8(dev, REG_SW_MAC_ADDR_0 + i,
> > > > hsr->dev_addr[i]);
> > > > +		if (ret)
> > > > +			return ret;    
> > > 
> > > FWIW:
> > > https://lore.kernel.org/netdev/155ff37f-43d5-5fe0-6de4-c4639909553d@gmail.com/
> > > Some coordination will be required regarding the MAC address that
> > > the switch driver needs to program to these registers.   
> > 
> > Writing of this MAC address is _required_ for PREVENTING PACKET
> > LOOP IN THE RING BY SELF-ADDRESS FILTERING feature.  
> 
> In case it was not clear, I was talking about coordination between you
> and Oleksij. He needs to program the same register for Wake on LAN.

Ok. Thanks for the information.

> 
> > In the ifconfig output - the lan1, lan2 and hsr0 shall all have the
> > same MAC address assigned.
> > 
> > I simply take the hsr0 mac address.
> >   
> > > It seems that it is not single purpose.  
> > 
> > At least in the case of HSR it looks like single purpose (for the
> > loop prevention).  
> 
> And for WoL, REG_SW_MAC_ADDR_0 is also single purpose. And for the
> MAC SA in the generated PAUSE frames, also single purpose. Single +
> single + single = ?
> 
> Being a common register for multiple functions, I hope that it won't
> be the users who discover than when multiple functionalities are used
> in tandem (like WoL+HSR), they partially overwrite what the other has
> done.

This cannot be excluded, yes. However, I think that HSR ports will not
use WoL...

> 
> So, by coordination, I mean something like a cohesive way of thinking
> out the driver.
> 
> For WoL/pause frames, the linked discussion suggested that the switch
> MAC address could be kept in sync with the DSA master's MAC address.
> 
> Could that also work here, and could we add a restriction to say
> "Offload not supported for HSR device with a MAC address different
> from the DSA master" and return -EOPNOTSUPP in ksz_hsr_join()? Then
> ksz9477_hsr_join() would not modify this register.

In my case - the lanX ports (and hsr0) gets the same MAC address as it
is assigned to eth0 (the cpu port).

So, yes I can add such code to avoid problems.

> 
> > > > +	/* Enable per port self-address filtering */
> > > > +	ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL,
> > > > PORT_SRC_ADDR_FILTER, true);
> > > > +	ksz_port_cfg(dev, partner->index, REG_PORT_LUE_CTRL,
> > > > +		     PORT_SRC_ADDR_FILTER, true);
> > > > +
> > > > +	/* Setup HW supported features for lan HSR ports */
> > > > +	slave = dsa_to_port(ds, port)->slave;
> > > > +	slave->features |= KSZ9477_SUPPORTED_HSR_FEATURES;
> > > > +
> > > > +	slave = dsa_to_port(ds, partner->index)->slave;
> > > > +	slave->features |= KSZ9477_SUPPORTED_HSR_FEATURES;    
> > > 
> > > Can the code that is duplicated for the partner port be moved to
> > > the caller?  
> > 
> > I've followed the convention from xrs700x driver, where we only make
> > setup when we are sure that on both HSR ports the "join" has been
> > called.  
> 
> If code that is duplicated for both member ports can be moved to code
> paths that get executed for each individual port, then the resulting
> driver code may turn out to be less complex.
> 
> It's not a big deal if it can't, and maintaining a sane operating mode
> in that transient state (HSR device with a single member port) is
> obviously much more important. But the question was if it can, and I
> don't think you've answered that?

I think that it would be possible to have single execution patch per
joining port (for example join is called on lan1 and then on lan2).

I'm concerned with writing common registers - for example
REG_HSR_PORT_MAP. If there are no side effects (like hsr port support
must be set for both at once), we can try to make single execution path.

Links:

[1] -
https://ww1.microchip.com/downloads/en/Appnotes/AN3474-KSZ9477-High-Availability-Seamless-Redundancy-Application-Note-00003474A.pdf



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@...x.de

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists