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

Lukasz Majewski
2023-09-05T16:33:23Z

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

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

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