| Message ID | 20230904120209.741207-3-lukma@denx.de (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: hsr: Enable HSR HW offloading for KSZ9477 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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/ 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). 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(). > + > 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. > + 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); > + > *tag = cpu_to_be16(val); > > return ksz_defer_xmit(dp, skb); > -- > 2.20.1 >
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
On Tue, Sep 05, 2023 at 12:44:09PM +0200, Lukasz Majewski wrote: > > 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. You mean 2 hsr{01} interfaces not being able to coexist in general, or just "offloaded" ones? > 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). > > > + 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. Let's look at ksz9477_xmit(), filtering only for changes to "u16 val". static struct sk_buff *ksz9477_xmit(struct sk_buff *skb, struct net_device *dev) { u16 val; val = BIT(dp->index); val |= FIELD_PREP(KSZ9477_TAIL_TAG_PRIO, prio); 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; val |= ksz_hsr_get_ports(dp->ds); } } Is KSZ9477_TAIL_TAG_LOOKUP ever set in "val", or am I missing something? > > > + 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. There was a quote about "premature optimization" which I can't quite remember... If you can see a measurable performance difference, then the list traversal can be converted to something more efficient. In this case, struct dsa_port :: hsr_dev can be converted to a larger struct dsa_hsr structure, similar to struct dsa_port :: bridge. That structure could look like this: struct dsa_hsr { struct net_device *dev; unsigned long port_mask; refcount_t refcount; }; and you could replace the list traversal with "val |= dp->hsr->port_mask". But a more complex solution requires a justification, which in this case is performance-related. So performance data must be gathered. FWIW, dsa_master_find_slave() also performs a list traversal. But similar discussions about performance improvements didn't lead anywhere.
Hi Vladimir, > On Tue, Sep 05, 2023 at 12:44:09PM +0200, Lukasz Majewski wrote: > > > 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. > > You mean 2 hsr{01} interfaces not being able to coexist in general, > or just "offloaded" ones? The KSZ9477 IC only allows to have two its ports from 5 available to be configured as HSR ones (so the HW offloading would work). And having single hsr0 with lan[12] is the used case on which I'm focused (with offloading or pure SW). > > > 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). > > > > > + 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. > > Let's look at ksz9477_xmit(), filtering only for changes to "u16 val". > > static struct sk_buff *ksz9477_xmit(struct sk_buff *skb, > struct net_device *dev) > { > u16 val; > > val = BIT(dp->index); > > val |= FIELD_PREP(KSZ9477_TAIL_TAG_PRIO, prio); > > 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; > val |= ksz_hsr_get_ports(dp->ds); > } > } > > Is KSZ9477_TAIL_TAG_LOOKUP ever set in "val", or am I missing > something? No, it looks like you are not. The clearance of KSZ9477_TAIL_TAG_LOOKUP seems to be an overkill. > > > > > + 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. > > There was a quote about "premature optimization" which I can't quite > remember... Yes, using caching by default instead of list iterating is the "premature optimization" .... :-) > > If you can see a measurable performance difference, then the list > traversal can be converted to something more efficient. > > In this case, struct dsa_port :: hsr_dev can be converted to a larger > struct dsa_hsr structure, similar to struct dsa_port :: bridge. > That structure could look like this: > > struct dsa_hsr { > struct net_device *dev; > unsigned long port_mask; > refcount_t refcount; > }; > > and you could replace the list traversal with "val |= > dp->hsr->port_mask". But a more complex solution requires a > justification, which in this case is performance-related. So > performance data must be gathered. > > FWIW, dsa_master_find_slave() also performs a list traversal. > But similar discussions about performance improvements didn't lead > anywhere. The iteration over hsr ports would simplify the code. I will use it and provide feedback if I find performance drop. Thanks for the feedback. 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
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; +} + 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; + val |= ksz_hsr_get_ports(dp->ds); + } + *tag = cpu_to_be16(val); return ksz_defer_xmit(dp, skb);
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(+)