Message-ID: <20230905130355.7x3vpgdlmdzg6skz@skbuf>
Date: Tue, 5 Sep 2023 16:03:55 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Lukasz Majewski <lukma@...x.de>
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

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)?

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.

> > > +	/* 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.

> 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.

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.

> > > +	/* 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?

Powered by blists - more mailing lists