Messages in this thread Patch in this message | | | Date | Tue, 5 Sep 2023 12:44:09 +0200 | From | Lukasz Majewski <> | Subject | Re: [PATCH v3 RFC 2/4] net: dsa: Extend ksz9477 TAG setup to support HSR frames duplication |
| |
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 [unhandled content-type:application/pgp-signature] | |