| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230905124409.40c7c2f1@wsk>
Date: Tue, 5 Sep 2023 12:44:09 +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 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@...x.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@...x.de
Content of type "application/pgp-signature" skipped
Powered by blists - more mailing lists