Date: Tue, 5 Sep 2023 13:11:03 +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 Mon, Sep 04, 2023 at 02:02:08PM +0200, Lukasz Majewski wrote:
> > This patch adds functions for providing in KSZ9477 switch HSR
> > (High-availability Seamless Redundancy) hardware offloading.
> > 
> > According to AN3474 application note following features are
> > provided:
> > - TX packet duplication from host to switch (NETIF_F_HW_HSR_DUP)
> > - RX packet duplication discarding
> > - Prevention of packet loop
> > 
> > For last two ones - there is a probability that some packets will
> > not be filtered in HW (in some special cases). Hence, the HSR core
> > code shall be used to discard those not caught frames.
> > 
> > Moreover, some switch registers adjustments are required - like
> > setting MAC address of HSR network interface.
> > 
> > Additionally, the KSZ9477 switch has been configured to forward
> > frames between HSR ports (1,2) members.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@...x.de>
> > ---
> > Changes for v2:
> > - Use struct ksz_device to store hsr ports information (not struct
> > dsa)
> > 
> > Changes for v3:
> > - Enable in-switch forwarding of frames between HSR ports (i.e.
> > enable bridging of those two ports)
> > 
> > - The NETIF_F_HW_HSR_FWD flag has been marked as supported by the
> > HSR network device
> > 
> > - Remove ETH MAC address validity check as it is done earlier in
> > the net driver
> > 
> > - Add comment regarding adding support for NETIF_F_HW_HSR_FWD flag
> > ---
> >  drivers/net/dsa/microchip/ksz9477.c | 103
> > ++++++++++++++++++++++++++++ drivers/net/dsa/microchip/ksz9477.h |
> >  4 ++ 2 files changed, 107 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz9477.c
> > b/drivers/net/dsa/microchip/ksz9477.c index
> > 83b7f2d5c1ea..c4ed89c1de48 100644 ---
> > a/drivers/net/dsa/microchip/ksz9477.c +++
> > b/drivers/net/dsa/microchip/ksz9477.c @@ -1141,6 +1141,109 @@ int
> > ksz9477_tc_cbs_set_cinc(struct ksz_device *dev, int port, u32 val)
> > return ksz_pwrite16(dev, port, REG_PORT_MTI_CREDIT_INCREMENT, val);
> > } 
> > +/* 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.

2. The "bridge" of HSR passes frames in-KSZ9477, which are not
addressed to this cpu host (between other HSR nodes).

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

> 
> > + */
> > +#define KSZ9477_SUPPORTED_HSR_FEATURES (NETIF_F_HW_HSR_DUP |
> > NETIF_F_HW_HSR_FWD) +
> > +int ksz9477_hsr_join(struct dsa_switch *ds, int port, struct
> > net_device *hsr,
> > +		     struct dsa_port *partner)
> > +{
> > +	struct ksz_device *dev = ds->priv;
> > +	struct net_device *slave;
> > +	u8 i, data;
> > +	int ret;
> > +
> > +	/* Program which ports shall support HSR */
> > +	dev->hsr_ports = BIT(port) | BIT(partner->index);
> > +	ksz_write32(dev, REG_HSR_PORT_MAP__4, dev->hsr_ports);
> > +
> > +	/* Forward frames between HSR ports (i.e. bridge together
> > HSR ports) */
> > +	ksz_prmw32(dev, port, REG_PORT_VLAN_MEMBERSHIP__4,
> > dev->hsr_ports,
> > +		   dev->hsr_ports);
> > +	ksz_prmw32(dev, partner->index,
> > REG_PORT_VLAN_MEMBERSHIP__4,
> > +		   dev->hsr_ports, dev->hsr_ports);  
> 
> Call ksz9477_cfg_port_member() instead?

+1

Thanks for the information.

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

> 
> > +	}
> > +
> > +	/* Enable global self-address filtering if not yet done
> > during switch
> > +	 * start
> > +	 */
> > +	ksz_read8(dev, REG_SW_LUE_CTRL_1, &data);
> > +	if (!(data & SW_SRC_ADDR_FILTER)) {
> > +		data |= SW_SRC_ADDR_FILTER;
> > +		ksz_write8(dev, REG_SW_LUE_CTRL_1, data);
> > +	}  
> 
> If there is no way that SW_SRC_ADDR_FILTER can be unset after
> ksz9477_reset_switch() is called, then this is dead code which should
> be removed.

Yes. Correct. I will remove it.

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

> 
> > +
> > +	pr_debug("%s: HSR join port: %d partner: %d port_map:
> > 0x%x\n", __func__,
> > +		 port, partner->index, dev->hsr_ports);
> > +
> > +	return 0;
> > +}
> > +
> > +int ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct
> > net_device *hsr,
> > +		      struct dsa_port *partner)
> > +{
> > +	struct ksz_device *dev = ds->priv;
> > +
> > +	/* Clear ports HSR support */
> > +	ksz_write32(dev, REG_HSR_PORT_MAP__4, 0);
> > +
> > +	/* Disable forwarding frames between HSR ports */
> > +	ksz_prmw32(dev, port, REG_PORT_VLAN_MEMBERSHIP__4,
> > dev->hsr_ports, 0);
> > +	ksz_prmw32(dev, partner->index,
> > REG_PORT_VLAN_MEMBERSHIP__4,
> > +		   dev->hsr_ports, 0);
> > +
> > +	/* Disable per port self-address filtering */
> > +	ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL,
> > PORT_SRC_ADDR_FILTER, false);
> > +	ksz_port_cfg(dev, partner->index, REG_PORT_LUE_CTRL,
> > +		     PORT_SRC_ADDR_FILTER, false);
> > +
> > +	return 0;
> > +}
> > +
> >  int ksz9477_switch_init(struct ksz_device *dev)
> >  {
> >  	u8 data8;
> > diff --git a/drivers/net/dsa/microchip/ksz9477.h
> > b/drivers/net/dsa/microchip/ksz9477.h index
> > b6f7e3c46e3f..634262efb73c 100644 ---
> > a/drivers/net/dsa/microchip/ksz9477.h +++
> > b/drivers/net/dsa/microchip/ksz9477.h @@ -58,5 +58,9 @@ int
> > ksz9477_dsa_init(struct ksz_device *dev); int
> > ksz9477_switch_init(struct ksz_device *dev); void
> > ksz9477_switch_exit(struct ksz_device *dev); void
> > ksz9477_port_queue_split(struct ksz_device *dev, int port); +int
> > ksz9477_hsr_join(struct dsa_switch *ds, int port, struct net_device
> > *hsr,
> > +		     struct dsa_port *partner);
> > +int ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct
> > net_device *hsr,
> > +		      struct dsa_port *partner);
> >  
> >  #endif
> > -- 
> > 2.20.1
> >   
> 




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