Re: [PATCH v3 RFC 2/4] net: dsa: Extend ksz9477 TAG setup to support HSR frames duplication

[Lukasz Majewski]

Hi Vladimir,

> On Mon, Sep 04, 2023 at 02:02:07PM +0200, Lukasz Majewski wrote:
> > The KSZ9477 has support for HSR (High-Availability Seamless
> > Redundancy). One of its offloading (i.e. performed in the switch IC
> > hardware) features is to duplicate received frame to both HSR aware
> > switch ports.
> > 
> > To achieve this goal - the tail TAG needs to be modified. To be more
> > specific, both ports must be marked as destination (egress) ones.
> > 
> > Moreover, according to AN3474 application note, the lookup bit (10)
> > should not be set in the tail tag.
> > 
> > Last but not least - the NETIF_F_HW_HSR_DUP flag indicates that the
> > device supports HSR and assures (in HSR core code) that frame is
> > sent only once from HOST to switch with tail tag indicating both
> > ports.
> > 
> > Information about bits to be set in tag is provided via KSZ generic
> > ksz_hsr_get_ports() function.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@xxxxxxx>
> > ---
> > Changes for v2:
> > - Use ksz_hsr_get_ports() to obtain the bits values corresponding to
> >   HSR aware ports
> > 
> > Changes for v3:
> > - None
> > ---
> >  drivers/net/dsa/microchip/ksz_common.c | 12 ++++++++++++
> >  include/linux/dsa/ksz_common.h         |  1 +
> >  net/dsa/tag_ksz.c                      |  5 +++++
> >  3 files changed, 18 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz_common.c
> > b/drivers/net/dsa/microchip/ksz_common.c index
> > d9d843efd111..579fde54d1e1 100644 ---
> > a/drivers/net/dsa/microchip/ksz_common.c +++
> > b/drivers/net/dsa/microchip/ksz_common.c @@ -3421,6 +3421,18 @@
> > static int ksz_setup_tc(struct dsa_switch *ds, int port, }
> >  }
> >  
> > +u16 ksz_hsr_get_ports(struct dsa_switch *ds)
> > +{
> > +	struct ksz_device *dev = ds->priv;
> > +
> > +	switch (dev->chip_id) {
> > +	case KSZ9477_CHIP_ID:
> > +		return dev->hsr_ports;
> > +	}
> > +
> > +	return 0;
> > +}  
> 
> When CONFIG_NET_DSA_MICROCHIP_KSZ_COMMON=m:
> 
> ld.lld: error: undefined symbol: ksz_hsr_get_ports
> referenced by tag_ksz.c:298
> (/opt/net-next/output-arm64-clang/../net/dsa/tag_ksz.c:298)
> net/dsa/tag_ksz.o:(ksz9477_xmit) in archive vmlinux.a
> 
> But before you rush to add EXPORT_SYMBOL_GPL(ksz_hsr_get_ports), be
> aware that due to DSA's design, tag_ksz.ko and ksz_common.ko cannot
> have any symbol dependency on each other, and if you do that, you
> will break module auto-loading. More information here, there were
> also patches that removed those dependencies for other tagger/switch
> driver pairs:
> https://lore.kernel.org/netdev/20210908220834.d7gmtnwrorhharna@skbuf/
> 

Ok. I will look on that

> Not to mention that there are other problems with the "dev->hsr_ports"
> concept. For example, having a hsr0 over lan0 and lan1, and a hsr1
> over lan2 and lan3, would set dev->hsr_ports to GENMASK(3, 0).

I doubt that having two hsr{01} interfaces is possible with current
kernel.

The KSZ9477 allows only to have 2 ports of 5 available as HSR
ones.

The same is with earlier chip xrs700x (but this have even bigger
constrain - there only ports 1 and 2 can support HSR). 

> But
> you want an xmit coming from hsr0 to get sent only to GENMASK(1, 0),
> and an xmit from hsr1 only to GENMASK(3, 2).
> 
> In this particular case, the best option seems to be to delete
> ksz_hsr_get_ports().

Please see my below comment.

> 
> > +
> >  static const struct dsa_switch_ops ksz_switch_ops = {
> >  	.get_tag_protocol	= ksz_get_tag_protocol,
> >  	.connect_tag_protocol   = ksz_connect_tag_protocol,
> > diff --git a/include/linux/dsa/ksz_common.h
> > b/include/linux/dsa/ksz_common.h index 576a99ca698d..fa3d9b0f3a72
> > 100644 --- a/include/linux/dsa/ksz_common.h
> > +++ b/include/linux/dsa/ksz_common.h
> > @@ -50,4 +50,5 @@ ksz_tagger_data(struct dsa_switch *ds)
> >  	return ds->tagger_data;
> >  }
> >  
> > +u16 ksz_hsr_get_ports(struct dsa_switch *ds);
> >  #endif /* _NET_DSA_KSZ_COMMON_H_ */
> > diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> > index ea100bd25939..903db95c37ee 100644
> > --- a/net/dsa/tag_ksz.c
> > +++ b/net/dsa/tag_ksz.c
> > @@ -293,6 +293,11 @@ static struct sk_buff *ksz9477_xmit(struct
> > sk_buff *skb, if (is_link_local_ether_addr(hdr->h_dest))
> >  		val |= KSZ9477_TAIL_TAG_OVERRIDE;
> >  
> > +	if (dev->features & NETIF_F_HW_HSR_DUP) {
> > +		val &= ~KSZ9477_TAIL_TAG_LOOKUP;  
> 
> No need to unset a bit which was never set.

I've explicitly followed the vendor's guidelines - the TAG_LOOKUP needs
to be cleared.

But if we can assure that it is not set here I can remove it.

> 
> > +		val |= ksz_hsr_get_ports(dp->ds);
> > +	}  
> 
> Would this work instead?
> 
> 	struct net_device *hsr_dev = dp->hsr_dev;
> 	struct dsa_port *other_dp;
> 
> 	dsa_hsr_foreach_port(other_dp, dp->ds, hsr_dev)
> 		val |= BIT(other_dp->index);
> 

I thought about this solution as well, but I've been afraid, that going
through the loop of all 5 ports each time we want to send single packet
will reduce the performance.

Hence, the idea with having the "hsr_ports" set once during join
function and then use this cached value afterwards.

> > +
> >  	*tag = cpu_to_be16(val);
> >  
> >  	return ksz_defer_xmit(dp, skb);
> > -- 
> > 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@xxxxxxx

Attachment: pgpBnYXaRAmUt.pgp
Description: OpenPGP digital signature