Add support for legacy Broadcom tags, which are similar to DSA_TAG_PROTO_BRCM.
These tags are used on BCM5325, BCM5365 and BCM63xx switches.
Signed-off-by: Álvaro Fernández Rojas <[email protected]>
---
include/net/dsa.h | 2 +
net/dsa/Kconfig | 7 ++++
net/dsa/tag_brcm.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 105 insertions(+)
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 83a933e563fe..dac303edd33d 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -49,10 +49,12 @@ struct phylink_link_state;
#define DSA_TAG_PROTO_XRS700X_VALUE 19
#define DSA_TAG_PROTO_OCELOT_8021Q_VALUE 20
#define DSA_TAG_PROTO_SEVILLE_VALUE 21
+#define DSA_TAG_PROTO_BRCM_LEGACY_VALUE 22
enum dsa_tag_protocol {
DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE,
DSA_TAG_PROTO_BRCM = DSA_TAG_PROTO_BRCM_VALUE,
+ DSA_TAG_PROTO_BRCM_LEGACY = DSA_TAG_PROTO_BRCM_LEGACY_VALUE,
DSA_TAG_PROTO_BRCM_PREPEND = DSA_TAG_PROTO_BRCM_PREPEND_VALUE,
DSA_TAG_PROTO_DSA = DSA_TAG_PROTO_DSA_VALUE,
DSA_TAG_PROTO_EDSA = DSA_TAG_PROTO_EDSA_VALUE,
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index 58b8fc82cd3c..aaf8a452fd5b 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -48,6 +48,13 @@ config NET_DSA_TAG_BRCM
Say Y if you want to enable support for tagging frames for the
Broadcom switches which place the tag after the MAC source address.
+config NET_DSA_TAG_BRCM_LEGACY
+ tristate "Tag driver for Broadcom legacy switches using in-frame headers"
+ select NET_DSA_TAG_BRCM_COMMON
+ help
+ Say Y if you want to enable support for tagging frames for the
+ Broadcom legacy switches which place the tag after the MAC source
+ address.
config NET_DSA_TAG_BRCM_PREPEND
tristate "Tag driver for Broadcom switches using prepended headers"
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index e2577a7dcbca..9dbff771c9b3 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -9,9 +9,23 @@
#include <linux/etherdevice.h>
#include <linux/list.h>
#include <linux/slab.h>
+#include <linux/types.h>
#include "dsa_priv.h"
+struct bcm_legacy_tag {
+ uint16_t type;
+#define BRCM_LEG_TYPE 0x8874
+
+ uint32_t tag;
+#define BRCM_LEG_TAG_PORT_ID (0xf)
+#define BRCM_LEG_TAG_MULTICAST (1 << 29)
+#define BRCM_LEG_TAG_EGRESS (2 << 29)
+#define BRCM_LEG_TAG_INGRESS (3 << 29)
+} __attribute__((packed));
+
+#define BRCM_LEG_TAG_LEN sizeof(struct bcm_legacy_tag)
+
/* This tag length is 4 bytes, older ones were 6 bytes, we do not
* handle them
*/
@@ -195,6 +209,85 @@ DSA_TAG_DRIVER(brcm_netdev_ops);
MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_BRCM);
#endif
+#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_LEGACY)
+static struct sk_buff *brcm_leg_tag_xmit(struct sk_buff *skb,
+ struct net_device *dev)
+{
+ struct dsa_port *dp = dsa_slave_to_port(dev);
+ struct bcm_legacy_tag *brcm_tag;
+
+ if (skb_cow_head(skb, BRCM_LEG_TAG_LEN) < 0)
+ return NULL;
+
+ /* The Ethernet switch we are interfaced with needs packets to be at
+ * least 64 bytes (including FCS) otherwise they will be discarded when
+ * they enter the switch port logic. When Broadcom tags are enabled, we
+ * need to make sure that packets are at least 70 bytes
+ * (including FCS and tag) because the length verification is done after
+ * the Broadcom tag is stripped off the ingress packet.
+ *
+ * Let dsa_slave_xmit() free the SKB
+ */
+ if (__skb_put_padto(skb, ETH_ZLEN + BRCM_LEG_TAG_LEN, false))
+ return NULL;
+
+ skb_push(skb, BRCM_LEG_TAG_LEN);
+
+ memmove(skb->data, skb->data + BRCM_LEG_TAG_LEN, 2 * ETH_ALEN);
+
+ brcm_tag = (struct bcm_legacy_tag *) (skb->data + 2 * ETH_ALEN);
+
+ brcm_tag->type = BRCM_LEG_TYPE;
+ brcm_tag->tag = BRCM_LEG_TAG_EGRESS;
+ brcm_tag->tag |= dp->index & BRCM_LEG_TAG_PORT_ID;
+
+ return skb;
+}
+
+
+static struct sk_buff *brcm_leg_tag_rcv(struct sk_buff *skb,
+ struct net_device *dev,
+ struct packet_type *pt)
+{
+ int source_port;
+ struct bcm_legacy_tag *brcm_tag;
+
+ if (unlikely(!pskb_may_pull(skb, BRCM_LEG_TAG_LEN)))
+ return NULL;
+
+ brcm_tag = (struct bcm_legacy_tag *) (skb->data - 2);
+
+ source_port = brcm_tag->tag & BRCM_LEG_TAG_PORT_ID;
+
+ skb->dev = dsa_master_find_slave(dev, 0, source_port);
+ if (!skb->dev)
+ return NULL;
+
+ /* Remove Broadcom tag and update checksum */
+ skb_pull_rcsum(skb, BRCM_LEG_TAG_LEN);
+
+ skb->offload_fwd_mark = 1;
+
+ /* Move the Ethernet DA and SA */
+ memmove(skb->data - ETH_HLEN,
+ skb->data - ETH_HLEN - BRCM_LEG_TAG_LEN,
+ 2 * ETH_ALEN);
+
+ return skb;
+}
+
+static const struct dsa_device_ops brcm_legacy_netdev_ops = {
+ .name = "brcm-legacy",
+ .proto = DSA_TAG_PROTO_BRCM_LEGACY,
+ .xmit = brcm_leg_tag_xmit,
+ .rcv = brcm_leg_tag_rcv,
+ .overhead = BRCM_LEG_TAG_LEN,
+};
+
+DSA_TAG_DRIVER(brcm_legacy_netdev_ops);
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_BRCM_LEGACY);
+#endif /* CONFIG_NET_DSA_TAG_BRCM_LEGACY */
+
#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_PREPEND)
static struct sk_buff *brcm_tag_xmit_prepend(struct sk_buff *skb,
struct net_device *dev)
@@ -227,6 +320,9 @@ static struct dsa_tag_driver *dsa_tag_driver_array[] = {
#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM)
&DSA_TAG_DRIVER_NAME(brcm_netdev_ops),
#endif
+#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_LEGACY)
+ &DSA_TAG_DRIVER_NAME(brcm_legacy_netdev_ops),
+#endif
#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_PREPEND)
&DSA_TAG_DRIVER_NAME(brcm_prepend_netdev_ops),
#endif
--
2.20.1
On 3/15/2021 7:27 AM, Álvaro Fernández Rojas wrote:
> Add support for legacy Broadcom tags, which are similar to DSA_TAG_PROTO_BRCM.
> These tags are used on BCM5325, BCM5365 and BCM63xx switches.
>
> Signed-off-by: Álvaro Fernández Rojas <[email protected]>
> ---
> include/net/dsa.h | 2 +
> net/dsa/Kconfig | 7 ++++
> net/dsa/tag_brcm.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 105 insertions(+)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 83a933e563fe..dac303edd33d 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -49,10 +49,12 @@ struct phylink_link_state;
> #define DSA_TAG_PROTO_XRS700X_VALUE 19
> #define DSA_TAG_PROTO_OCELOT_8021Q_VALUE 20
> #define DSA_TAG_PROTO_SEVILLE_VALUE 21
> +#define DSA_TAG_PROTO_BRCM_LEGACY_VALUE 22
>
> enum dsa_tag_protocol {
> DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE,
> DSA_TAG_PROTO_BRCM = DSA_TAG_PROTO_BRCM_VALUE,
> + DSA_TAG_PROTO_BRCM_LEGACY = DSA_TAG_PROTO_BRCM_LEGACY_VALUE,
> DSA_TAG_PROTO_BRCM_PREPEND = DSA_TAG_PROTO_BRCM_PREPEND_VALUE,
> DSA_TAG_PROTO_DSA = DSA_TAG_PROTO_DSA_VALUE,
> DSA_TAG_PROTO_EDSA = DSA_TAG_PROTO_EDSA_VALUE,
> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> index 58b8fc82cd3c..aaf8a452fd5b 100644
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -48,6 +48,13 @@ config NET_DSA_TAG_BRCM
> Say Y if you want to enable support for tagging frames for the
> Broadcom switches which place the tag after the MAC source address.
>
> +config NET_DSA_TAG_BRCM_LEGACY
> + tristate "Tag driver for Broadcom legacy switches using in-frame headers"
> + select NET_DSA_TAG_BRCM_COMMON
> + help
> + Say Y if you want to enable support for tagging frames for the
> + Broadcom legacy switches which place the tag after the MAC source
> + address.
>
> config NET_DSA_TAG_BRCM_PREPEND
> tristate "Tag driver for Broadcom switches using prepended headers"
> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
> index e2577a7dcbca..9dbff771c9b3 100644
> --- a/net/dsa/tag_brcm.c
> +++ b/net/dsa/tag_brcm.c
> @@ -9,9 +9,23 @@
> #include <linux/etherdevice.h>
> #include <linux/list.h>
> #include <linux/slab.h>
> +#include <linux/types.h>
>
> #include "dsa_priv.h"
>
> +struct bcm_legacy_tag {
> + uint16_t type;
> +#define BRCM_LEG_TYPE 0x8874
> +
> + uint32_t tag;
> +#define BRCM_LEG_TAG_PORT_ID (0xf)
> +#define BRCM_LEG_TAG_MULTICAST (1 << 29)
And you could define UNICAST with (0 << 29) just for documentation purposes.
> +#define BRCM_LEG_TAG_EGRESS (2 << 29)
> +#define BRCM_LEG_TAG_INGRESS (3 << 29)
> +} __attribute__((packed));
Please define these as relatives within a byte such that only byte
accesses are done, thus eliminating any endian issues, your code for
instance will work fine on a big-endian machine (like the 63xx you have
tested) but not on a little-endian machine.
Other than that, this looks good, thanks!
> +
> +#define BRCM_LEG_TAG_LEN sizeof(struct bcm_legacy_tag)
> +
> /* This tag length is 4 bytes, older ones were 6 bytes, we do not
> * handle them
> */
> @@ -195,6 +209,85 @@ DSA_TAG_DRIVER(brcm_netdev_ops);
> MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_BRCM);
> #endif
>
> +#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_LEGACY)
> +static struct sk_buff *brcm_leg_tag_xmit(struct sk_buff *skb,
> + struct net_device *dev)
> +{
> + struct dsa_port *dp = dsa_slave_to_port(dev);
> + struct bcm_legacy_tag *brcm_tag;
> +
> + if (skb_cow_head(skb, BRCM_LEG_TAG_LEN) < 0)
> + return NULL;
> +
> + /* The Ethernet switch we are interfaced with needs packets to be at
> + * least 64 bytes (including FCS) otherwise they will be discarded when
> + * they enter the switch port logic. When Broadcom tags are enabled, we
> + * need to make sure that packets are at least 70 bytes
> + * (including FCS and tag) because the length verification is done after
> + * the Broadcom tag is stripped off the ingress packet.
> + *
> + * Let dsa_slave_xmit() free the SKB
> + */
> + if (__skb_put_padto(skb, ETH_ZLEN + BRCM_LEG_TAG_LEN, false))
> + return NULL;
> +
> + skb_push(skb, BRCM_LEG_TAG_LEN);
> +
> + memmove(skb->data, skb->data + BRCM_LEG_TAG_LEN, 2 * ETH_ALEN);
> +
> + brcm_tag = (struct bcm_legacy_tag *) (skb->data + 2 * ETH_ALEN);
> +
> + brcm_tag->type = BRCM_LEG_TYPE;
> + brcm_tag->tag = BRCM_LEG_TAG_EGRESS;
> + brcm_tag->tag |= dp->index & BRCM_LEG_TAG_PORT_ID;
> +
> + return skb;
> +}
> +
> +
> +static struct sk_buff *brcm_leg_tag_rcv(struct sk_buff *skb,
> + struct net_device *dev,
> + struct packet_type *pt)
> +{
> + int source_port;
> + struct bcm_legacy_tag *brcm_tag;
> +
> + if (unlikely(!pskb_may_pull(skb, BRCM_LEG_TAG_LEN)))
> + return NULL;
> +
> + brcm_tag = (struct bcm_legacy_tag *) (skb->data - 2);
> +
> + source_port = brcm_tag->tag & BRCM_LEG_TAG_PORT_ID;
> +
> + skb->dev = dsa_master_find_slave(dev, 0, source_port);
> + if (!skb->dev)
> + return NULL;
> +
> + /* Remove Broadcom tag and update checksum */
> + skb_pull_rcsum(skb, BRCM_LEG_TAG_LEN);
> +
> + skb->offload_fwd_mark = 1;
> +
> + /* Move the Ethernet DA and SA */
> + memmove(skb->data - ETH_HLEN,
> + skb->data - ETH_HLEN - BRCM_LEG_TAG_LEN,
> + 2 * ETH_ALEN);
> +
> + return skb;
> +}
> +
> +static const struct dsa_device_ops brcm_legacy_netdev_ops = {
> + .name = "brcm-legacy",
> + .proto = DSA_TAG_PROTO_BRCM_LEGACY,
> + .xmit = brcm_leg_tag_xmit,
> + .rcv = brcm_leg_tag_rcv,
> + .overhead = BRCM_LEG_TAG_LEN,
> +};
> +
> +DSA_TAG_DRIVER(brcm_legacy_netdev_ops);
> +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_BRCM_LEGACY);
> +#endif /* CONFIG_NET_DSA_TAG_BRCM_LEGACY */
> +
> #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_PREPEND)
> static struct sk_buff *brcm_tag_xmit_prepend(struct sk_buff *skb,
> struct net_device *dev)
> @@ -227,6 +320,9 @@ static struct dsa_tag_driver *dsa_tag_driver_array[] = {
> #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM)
> &DSA_TAG_DRIVER_NAME(brcm_netdev_ops),
> #endif
> +#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_LEGACY)
> + &DSA_TAG_DRIVER_NAME(brcm_legacy_netdev_ops),
> +#endif
> #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_PREPEND)
> &DSA_TAG_DRIVER_NAME(brcm_prepend_netdev_ops),
> #endif
>
--
Florian
On Mon, Mar 15, 2021 at 03:27:35PM +0100, ?lvaro Fern?ndez Rojas wrote:
> Add support for legacy Broadcom tags, which are similar to DSA_TAG_PROTO_BRCM.
> These tags are used on BCM5325, BCM5365 and BCM63xx switches.
>
> Signed-off-by: ?lvaro Fern?ndez Rojas <[email protected]>
> ---
> include/net/dsa.h | 2 +
> net/dsa/Kconfig | 7 ++++
> net/dsa/tag_brcm.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 105 insertions(+)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 83a933e563fe..dac303edd33d 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -49,10 +49,12 @@ struct phylink_link_state;
> #define DSA_TAG_PROTO_XRS700X_VALUE 19
> #define DSA_TAG_PROTO_OCELOT_8021Q_VALUE 20
> #define DSA_TAG_PROTO_SEVILLE_VALUE 21
> +#define DSA_TAG_PROTO_BRCM_LEGACY_VALUE 22
>
> enum dsa_tag_protocol {
> DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE,
> DSA_TAG_PROTO_BRCM = DSA_TAG_PROTO_BRCM_VALUE,
> + DSA_TAG_PROTO_BRCM_LEGACY = DSA_TAG_PROTO_BRCM_LEGACY_VALUE,
Is there no better qualifier for this tagging protocol name than "legacy"?
> DSA_TAG_PROTO_BRCM_PREPEND = DSA_TAG_PROTO_BRCM_PREPEND_VALUE,
> DSA_TAG_PROTO_DSA = DSA_TAG_PROTO_DSA_VALUE,
> DSA_TAG_PROTO_EDSA = DSA_TAG_PROTO_EDSA_VALUE,
> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> index 58b8fc82cd3c..aaf8a452fd5b 100644
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -48,6 +48,13 @@ config NET_DSA_TAG_BRCM
> Say Y if you want to enable support for tagging frames for the
> Broadcom switches which place the tag after the MAC source address.
>
> +config NET_DSA_TAG_BRCM_LEGACY
> + tristate "Tag driver for Broadcom legacy switches using in-frame headers"
Aren't all headers in-frame?
> + select NET_DSA_TAG_BRCM_COMMON
> + help
> + Say Y if you want to enable support for tagging frames for the
> + Broadcom legacy switches which place the tag after the MAC source
> + address.
>
> config NET_DSA_TAG_BRCM_PREPEND
> tristate "Tag driver for Broadcom switches using prepended headers"
> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
> index e2577a7dcbca..9dbff771c9b3 100644
> --- a/net/dsa/tag_brcm.c
> +++ b/net/dsa/tag_brcm.c
> @@ -9,9 +9,23 @@
> #include <linux/etherdevice.h>
> #include <linux/list.h>
> #include <linux/slab.h>
> +#include <linux/types.h>
>
> #include "dsa_priv.h"
>
> +struct bcm_legacy_tag {
> + uint16_t type;
> +#define BRCM_LEG_TYPE 0x8874
> +
> + uint32_t tag;
> +#define BRCM_LEG_TAG_PORT_ID (0xf)
> +#define BRCM_LEG_TAG_MULTICAST (1 << 29)
> +#define BRCM_LEG_TAG_EGRESS (2 << 29)
> +#define BRCM_LEG_TAG_INGRESS (3 << 29)
> +} __attribute__((packed));
> +
> +#define BRCM_LEG_TAG_LEN sizeof(struct bcm_legacy_tag)
> +
As Florian pointed out, tagging protocol parsing should be
endian-independent, and mapping a struct over the frame header is pretty
much not that.
> /* This tag length is 4 bytes, older ones were 6 bytes, we do not
> * handle them
> */
> @@ -195,6 +209,85 @@ DSA_TAG_DRIVER(brcm_netdev_ops);
> MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_BRCM);
> #endif
>
> +#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_LEGACY)
> +static struct sk_buff *brcm_leg_tag_xmit(struct sk_buff *skb,
> + struct net_device *dev)
> +{
> + struct dsa_port *dp = dsa_slave_to_port(dev);
> + struct bcm_legacy_tag *brcm_tag;
> +
> + if (skb_cow_head(skb, BRCM_LEG_TAG_LEN) < 0)
> + return NULL;
This is not needed since commit 2f0d030c5ffe ("net: dsa: tag_brcm: let
DSA core deal with TX reallocation").
> + /* The Ethernet switch we are interfaced with needs packets to be at
> + * least 64 bytes (including FCS) otherwise they will be discarded when
> + * they enter the switch port logic. When Broadcom tags are enabled, we
> + * need to make sure that packets are at least 70 bytes
> + * (including FCS and tag) because the length verification is done after
> + * the Broadcom tag is stripped off the ingress packet.
> + *
> + * Let dsa_slave_xmit() free the SKB
> + */
> + if (__skb_put_padto(skb, ETH_ZLEN + BRCM_LEG_TAG_LEN, false))
> + return NULL;
Are you sure the switches you're working on need this, or is it just
another copy-pasta?
> + skb_push(skb, BRCM_LEG_TAG_LEN);
> +
> + memmove(skb->data, skb->data + BRCM_LEG_TAG_LEN, 2 * ETH_ALEN);
> +
> + brcm_tag = (struct bcm_legacy_tag *) (skb->data + 2 * ETH_ALEN);
> +
> + brcm_tag->type = BRCM_LEG_TYPE;
> + brcm_tag->tag = BRCM_LEG_TAG_EGRESS;
> + brcm_tag->tag |= dp->index & BRCM_LEG_TAG_PORT_ID;
> +
> + return skb;
> +}
> +
> +
Please remove the extra newline.
> +static struct sk_buff *brcm_leg_tag_rcv(struct sk_buff *skb,
> + struct net_device *dev,
> + struct packet_type *pt)
> +{
> + int source_port;
> + struct bcm_legacy_tag *brcm_tag;
Please declare the local variables in the order of decreasing line length.
> +
> + if (unlikely(!pskb_may_pull(skb, BRCM_LEG_TAG_LEN)))
> + return NULL;
> +
> + brcm_tag = (struct bcm_legacy_tag *) (skb->data - 2);
Nitpick: the space between the *) and the (skb-> is not needed.
> +
> + source_port = brcm_tag->tag & BRCM_LEG_TAG_PORT_ID;
> +
> + skb->dev = dsa_master_find_slave(dev, 0, source_port);
> + if (!skb->dev)
> + return NULL;
> +
> + /* Remove Broadcom tag and update checksum */
> + skb_pull_rcsum(skb, BRCM_LEG_TAG_LEN);
> +
> + skb->offload_fwd_mark = 1;
> +
> + /* Move the Ethernet DA and SA */
> + memmove(skb->data - ETH_HLEN,
> + skb->data - ETH_HLEN - BRCM_LEG_TAG_LEN,
> + 2 * ETH_ALEN);
> +
> + return skb;
> +}
> +
> +static const struct dsa_device_ops brcm_legacy_netdev_ops = {
> + .name = "brcm-legacy",
> + .proto = DSA_TAG_PROTO_BRCM_LEGACY,
> + .xmit = brcm_leg_tag_xmit,
> + .rcv = brcm_leg_tag_rcv,
> + .overhead = BRCM_LEG_TAG_LEN,
> +};
> +
> +DSA_TAG_DRIVER(brcm_legacy_netdev_ops);
> +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_BRCM_LEGACY);
> +#endif /* CONFIG_NET_DSA_TAG_BRCM_LEGACY */
> +
> #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_PREPEND)
> static struct sk_buff *brcm_tag_xmit_prepend(struct sk_buff *skb,
> struct net_device *dev)
> @@ -227,6 +320,9 @@ static struct dsa_tag_driver *dsa_tag_driver_array[] = {
> #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM)
> &DSA_TAG_DRIVER_NAME(brcm_netdev_ops),
> #endif
> +#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_LEGACY)
> + &DSA_TAG_DRIVER_NAME(brcm_legacy_netdev_ops),
> +#endif
> #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_PREPEND)
> &DSA_TAG_DRIVER_NAME(brcm_prepend_netdev_ops),
> #endif
> --
> 2.20.1
>
Hi Vladimir,
> El 15 mar 2021, a las 22:28, Vladimir Oltean <[email protected]> escribió:
>
> On Mon, Mar 15, 2021 at 03:27:35PM +0100, Álvaro Fernández Rojas wrote:
>> Add support for legacy Broadcom tags, which are similar to DSA_TAG_PROTO_BRCM.
>> These tags are used on BCM5325, BCM5365 and BCM63xx switches.
>>
>> Signed-off-by: Álvaro Fernández Rojas <[email protected]>
>> ---
>> include/net/dsa.h | 2 +
>> net/dsa/Kconfig | 7 ++++
>> net/dsa/tag_brcm.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 105 insertions(+)
>>
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index 83a933e563fe..dac303edd33d 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -49,10 +49,12 @@ struct phylink_link_state;
>> #define DSA_TAG_PROTO_XRS700X_VALUE 19
>> #define DSA_TAG_PROTO_OCELOT_8021Q_VALUE 20
>> #define DSA_TAG_PROTO_SEVILLE_VALUE 21
>> +#define DSA_TAG_PROTO_BRCM_LEGACY_VALUE 22
>>
>> enum dsa_tag_protocol {
>> DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE,
>> DSA_TAG_PROTO_BRCM = DSA_TAG_PROTO_BRCM_VALUE,
>> + DSA_TAG_PROTO_BRCM_LEGACY = DSA_TAG_PROTO_BRCM_LEGACY_VALUE,
>
> Is there no better qualifier for this tagging protocol name than "legacy"?
It’s always referred to as “legacy”, so that’s what I used.
Maybe @Florian can suggest a better name for this...
>
>> DSA_TAG_PROTO_BRCM_PREPEND = DSA_TAG_PROTO_BRCM_PREPEND_VALUE,
>> DSA_TAG_PROTO_DSA = DSA_TAG_PROTO_DSA_VALUE,
>> DSA_TAG_PROTO_EDSA = DSA_TAG_PROTO_EDSA_VALUE,
>> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
>> index 58b8fc82cd3c..aaf8a452fd5b 100644
>> --- a/net/dsa/Kconfig
>> +++ b/net/dsa/Kconfig
>> @@ -48,6 +48,13 @@ config NET_DSA_TAG_BRCM
>> Say Y if you want to enable support for tagging frames for the
>> Broadcom switches which place the tag after the MAC source address.
>>
>> +config NET_DSA_TAG_BRCM_LEGACY
>> + tristate "Tag driver for Broadcom legacy switches using in-frame headers"
>
> Aren't all headers in-frame?
I copied that from NET_DSA_TAG_BRCM:
https://github.com/torvalds/linux/blob/1df27313f50a57497c1faeb6a6ae4ca939c85a7d/net/dsa/Kconfig#L45
Do you want me to change it to "Tag driver for Broadcom legacy switches” or “Legacy tag driver for Broadcom switches"?
>
>> + select NET_DSA_TAG_BRCM_COMMON
>> + help
>> + Say Y if you want to enable support for tagging frames for the
>> + Broadcom legacy switches which place the tag after the MAC source
>> + address.
>>
>> config NET_DSA_TAG_BRCM_PREPEND
>> tristate "Tag driver for Broadcom switches using prepended headers"
>> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
>> index e2577a7dcbca..9dbff771c9b3 100644
>> --- a/net/dsa/tag_brcm.c
>> +++ b/net/dsa/tag_brcm.c
>> @@ -9,9 +9,23 @@
>> #include <linux/etherdevice.h>
>> #include <linux/list.h>
>> #include <linux/slab.h>
>> +#include <linux/types.h>
>>
>> #include "dsa_priv.h"
>>
>> +struct bcm_legacy_tag {
>> + uint16_t type;
>> +#define BRCM_LEG_TYPE 0x8874
>> +
>> + uint32_t tag;
>> +#define BRCM_LEG_TAG_PORT_ID (0xf)
>> +#define BRCM_LEG_TAG_MULTICAST (1 << 29)
>> +#define BRCM_LEG_TAG_EGRESS (2 << 29)
>> +#define BRCM_LEG_TAG_INGRESS (3 << 29)
>> +} __attribute__((packed));
>> +
>> +#define BRCM_LEG_TAG_LEN sizeof(struct bcm_legacy_tag)
>> +
>
> As Florian pointed out, tagging protocol parsing should be
> endian-independent, and mapping a struct over the frame header is pretty
> much not that.
Ok, I will change that in v2.
>
>> /* This tag length is 4 bytes, older ones were 6 bytes, we do not
>> * handle them
>> */
>> @@ -195,6 +209,85 @@ DSA_TAG_DRIVER(brcm_netdev_ops);
>> MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_BRCM);
>> #endif
>>
>> +#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_LEGACY)
>> +static struct sk_buff *brcm_leg_tag_xmit(struct sk_buff *skb,
>> + struct net_device *dev)
>> +{
>> + struct dsa_port *dp = dsa_slave_to_port(dev);
>> + struct bcm_legacy_tag *brcm_tag;
>> +
>> + if (skb_cow_head(skb, BRCM_LEG_TAG_LEN) < 0)
>> + return NULL;
>
> This is not needed since commit 2f0d030c5ffe ("net: dsa: tag_brcm: let
> DSA core deal with TX reallocation").
I’m testing this on v5.10 and I forgot to remove it, sorry :$.
>
>> + /* The Ethernet switch we are interfaced with needs packets to be at
>> + * least 64 bytes (including FCS) otherwise they will be discarded when
>> + * they enter the switch port logic. When Broadcom tags are enabled, we
>> + * need to make sure that packets are at least 70 bytes
>> + * (including FCS and tag) because the length verification is done after
>> + * the Broadcom tag is stripped off the ingress packet.
>> + *
>> + * Let dsa_slave_xmit() free the SKB
>> + */
>> + if (__skb_put_padto(skb, ETH_ZLEN + BRCM_LEG_TAG_LEN, false))
>> + return NULL;
>
> Are you sure the switches you're working on need this, or is it just
> another copy-pasta?
Yes, I need that.
>
>> + skb_push(skb, BRCM_LEG_TAG_LEN);
>> +
>> + memmove(skb->data, skb->data + BRCM_LEG_TAG_LEN, 2 * ETH_ALEN);
>> +
>> + brcm_tag = (struct bcm_legacy_tag *) (skb->data + 2 * ETH_ALEN);
>> +
>> + brcm_tag->type = BRCM_LEG_TYPE;
>> + brcm_tag->tag = BRCM_LEG_TAG_EGRESS;
>> + brcm_tag->tag |= dp->index & BRCM_LEG_TAG_PORT_ID;
>> +
>> + return skb;
>> +}
>> +
>> +
>
> Please remove the extra newline.
Ok.
>
>> +static struct sk_buff *brcm_leg_tag_rcv(struct sk_buff *skb,
>> + struct net_device *dev,
>> + struct packet_type *pt)
>> +{
>> + int source_port;
>> + struct bcm_legacy_tag *brcm_tag;
>
> Please declare the local variables in the order of decreasing line length.
Ok.
>
>> +
>> + if (unlikely(!pskb_may_pull(skb, BRCM_LEG_TAG_LEN)))
>> + return NULL;
>> +
>> + brcm_tag = (struct bcm_legacy_tag *) (skb->data - 2);
>
> Nitpick: the space between the *) and the (skb-> is not needed.
Ok, but this is going to disappear when I remove the struct usage.
>
>> +
>> + source_port = brcm_tag->tag & BRCM_LEG_TAG_PORT_ID;
>> +
>> + skb->dev = dsa_master_find_slave(dev, 0, source_port);
>> + if (!skb->dev)
>> + return NULL;
>> +
>> + /* Remove Broadcom tag and update checksum */
>> + skb_pull_rcsum(skb, BRCM_LEG_TAG_LEN);
>> +
>> + skb->offload_fwd_mark = 1;
>> +
>> + /* Move the Ethernet DA and SA */
>> + memmove(skb->data - ETH_HLEN,
>> + skb->data - ETH_HLEN - BRCM_LEG_TAG_LEN,
>> + 2 * ETH_ALEN);
>> +
>> + return skb;
>> +}
>> +
>> +static const struct dsa_device_ops brcm_legacy_netdev_ops = {
>> + .name = "brcm-legacy",
>> + .proto = DSA_TAG_PROTO_BRCM_LEGACY,
>> + .xmit = brcm_leg_tag_xmit,
>> + .rcv = brcm_leg_tag_rcv,
>> + .overhead = BRCM_LEG_TAG_LEN,
>> +};
>> +
>> +DSA_TAG_DRIVER(brcm_legacy_netdev_ops);
>> +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_BRCM_LEGACY);
>> +#endif /* CONFIG_NET_DSA_TAG_BRCM_LEGACY */
>> +
>> #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_PREPEND)
>> static struct sk_buff *brcm_tag_xmit_prepend(struct sk_buff *skb,
>> struct net_device *dev)
>> @@ -227,6 +320,9 @@ static struct dsa_tag_driver *dsa_tag_driver_array[] = {
>> #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM)
>> &DSA_TAG_DRIVER_NAME(brcm_netdev_ops),
>> #endif
>> +#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_LEGACY)
>> + &DSA_TAG_DRIVER_NAME(brcm_legacy_netdev_ops),
>> +#endif
>> #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_PREPEND)
>> &DSA_TAG_DRIVER_NAME(brcm_prepend_netdev_ops),
>> #endif
>> --
>> 2.20.1
Best regards,
Álvaro.
On Wed, 17 Mar 2021 at 10:16, Álvaro Fernández Rojas <[email protected]> wrote:
>
> Hi Vladimir,
>
> > El 15 mar 2021, a las 22:28, Vladimir Oltean <[email protected]> escribió:
> >
> > On Mon, Mar 15, 2021 at 03:27:35PM +0100, Álvaro Fernández Rojas wrote:
> >> Add support for legacy Broadcom tags, which are similar to DSA_TAG_PROTO_BRCM.
> >> These tags are used on BCM5325, BCM5365 and BCM63xx switches.
> >>
> >> Signed-off-by: Álvaro Fernández Rojas <[email protected]>
> >> ---
> >> include/net/dsa.h | 2 +
> >> net/dsa/Kconfig | 7 ++++
> >> net/dsa/tag_brcm.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 105 insertions(+)
> >>
> >> diff --git a/include/net/dsa.h b/include/net/dsa.h
> >> index 83a933e563fe..dac303edd33d 100644
> >> --- a/include/net/dsa.h
> >> +++ b/include/net/dsa.h
> >> @@ -49,10 +49,12 @@ struct phylink_link_state;
> >> #define DSA_TAG_PROTO_XRS700X_VALUE 19
> >> #define DSA_TAG_PROTO_OCELOT_8021Q_VALUE 20
> >> #define DSA_TAG_PROTO_SEVILLE_VALUE 21
> >> +#define DSA_TAG_PROTO_BRCM_LEGACY_VALUE 22
> >>
> >> enum dsa_tag_protocol {
> >> DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE,
> >> DSA_TAG_PROTO_BRCM = DSA_TAG_PROTO_BRCM_VALUE,
> >> + DSA_TAG_PROTO_BRCM_LEGACY = DSA_TAG_PROTO_BRCM_LEGACY_VALUE,
> >
> > Is there no better qualifier for this tagging protocol name than "legacy"?
>
> It’s always referred to as “legacy”, so that’s what I used.
> Maybe @Florian can suggest a better name for this...
Broadcom refers to both as "the BRCM tag" or "the Broadcom Management
Header/Tag" in documentation with no versioning at all.
Codewise, the brcm963xx code names the old one BRCM_TAG and the newer
one BRCM_TAG_TYPE2. Not really better IMHO.
Maybe BRCM_OLD? less characters than Legacy, and doesn't need to be abbreviated.
To make matters worse, there seem to exist different versions of the
tag variants where some opcodes mean different things, e.g. BCM5325
might set the opcode to 1 for Multicast frames.
I would probably suggest enabling it only for switch models we
verified to be working with it.
On a different side node, should the dsa_tag_protocol be ordered
numerically, i.e. should DSA_TAG_PROTO_BRCM_PREPEND be the last one
since it is the highest with 22?
> >
> >> DSA_TAG_PROTO_BRCM_PREPEND = DSA_TAG_PROTO_BRCM_PREPEND_VALUE,
> >> DSA_TAG_PROTO_DSA = DSA_TAG_PROTO_DSA_VALUE,
> >> DSA_TAG_PROTO_EDSA = DSA_TAG_PROTO_EDSA_VALUE,
> >> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> >> index 58b8fc82cd3c..aaf8a452fd5b 100644
> >> --- a/net/dsa/Kconfig
> >> +++ b/net/dsa/Kconfig
> >> @@ -48,6 +48,13 @@ config NET_DSA_TAG_BRCM
> >> Say Y if you want to enable support for tagging frames for the
> >> Broadcom switches which place the tag after the MAC source address.
> >>
> >> +config NET_DSA_TAG_BRCM_LEGACY
> >> + tristate "Tag driver for Broadcom legacy switches using in-frame headers"
> >
> > Aren't all headers in-frame?
>
> I copied that from NET_DSA_TAG_BRCM:
> https://github.com/torvalds/linux/blob/1df27313f50a57497c1faeb6a6ae4ca939c85a7d/net/dsa/Kconfig#L45
>
> Do you want me to change it to "Tag driver for Broadcom legacy switches” or “Legacy tag driver for Broadcom switches"?
This means that the tag is inserted after the SRC/DST mac addresses,
in contrast to BRCM_PREPEND that gets prepended to the full frame.
> >> + select NET_DSA_TAG_BRCM_COMMON
> >> + help
> >> + Say Y if you want to enable support for tagging frames for the
> >> + Broadcom legacy switches which place the tag after the MAC source
> >> + address.
> >>
> >> config NET_DSA_TAG_BRCM_PREPEND
> >> tristate "Tag driver for Broadcom switches using prepended headers"
> >> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
> >> index e2577a7dcbca..9dbff771c9b3 100644
> >> --- a/net/dsa/tag_brcm.c
> >> +++ b/net/dsa/tag_brcm.c
> >> @@ -9,9 +9,23 @@
> >> #include <linux/etherdevice.h>
> >> #include <linux/list.h>
> >> #include <linux/slab.h>
> >> +#include <linux/types.h>
> >>
> >> #include "dsa_priv.h"
> >>
> >> +struct bcm_legacy_tag {
> >> + uint16_t type;
> >> +#define BRCM_LEG_TYPE 0x8874
> >> +
> >> + uint32_t tag;
> >> +#define BRCM_LEG_TAG_PORT_ID (0xf)
> >> +#define BRCM_LEG_TAG_MULTICAST (1 << 29)
> >> +#define BRCM_LEG_TAG_EGRESS (2 << 29)
> >> +#define BRCM_LEG_TAG_INGRESS (3 << 29)
> >> +} __attribute__((packed));
> >> +
> >> +#define BRCM_LEG_TAG_LEN sizeof(struct bcm_legacy_tag)
> >> +
> >
> > As Florian pointed out, tagging protocol parsing should be
> > endian-independent, and mapping a struct over the frame header is pretty
> > much not that.
>
> Ok, I will change that in v2.
>
> >
> >> /* This tag length is 4 bytes, older ones were 6 bytes, we do not
> >> * handle them
You might want to update this comment, since you now handle them ;-)
Best Regards
Jonas