| Message ID | 20230912122201.3752918-7-paweldembicki@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: vsc73xx: Make vsc73xx usable | expand |
Hi Pawel, On Tue, Sep 12, 2023 at 02:22:00PM +0200, Pawel Dembicki wrote: > This commit introduces a new tagger based on 802.1q tagging. > It's designed for the vsc73xx driver. The VSC73xx family doesn't have > any tag support for the RGMII port, but it could be based on VLANs. > > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com> > --- > diff --git a/net/dsa/tag_vsc73xx_8021q.c b/net/dsa/tag_vsc73xx_8021q.c > new file mode 100644 > index 000000000000..9093a71e6eb0 > --- /dev/null > +++ b/net/dsa/tag_vsc73xx_8021q.c > @@ -0,0 +1,91 @@ > +// SPDX-License-Identifier: GPL-2.0 OR MIT > +/* Copyright (C) 2022 Pawel Dembicki <paweldembicki@gmail.com> 2022-2023 by now, maybe? > + * Based on tag_sja1105.c: > + * Copyright (c) 2019, Vladimir Oltean <olteanv@gmail.com> > + */ > +#include <linux/dsa/8021q.h> > + > +#include "tag.h" > +#include "tag_8021q.h" > + > +#define VSC73XX_8021Q_NAME "vsc73xx-8021q" > + > +static struct sk_buff *vsc73xx_xmit(struct sk_buff *skb, struct net_device *netdev) > +{ > + struct dsa_port *dp = dsa_slave_to_port(netdev); > + u16 queue_mapping = skb_get_queue_mapping(skb); > + u16 tx_vid = dsa_tag_8021q_standalone_vid(dp); > + u8 pcp; > + > + if (skb->offload_fwd_mark) { > + unsigned int bridge_num = dsa_port_bridge_num_get(dp); > + struct net_device *br = dsa_port_bridge_dev_get(dp); > + > + if (br_vlan_enabled(br)) > + return skb; > + > + tx_vid = dsa_tag_8021q_bridge_vid(bridge_num); > + } > + > + pcp = netdev_txq_to_tc(netdev, queue_mapping); > + > + return dsa_8021q_xmit(skb, netdev, ETH_P_8021Q, > + ((pcp << VLAN_PRIO_SHIFT) | tx_vid)); > +} > + > +static void vsc73xx_vlan_rcv(struct sk_buff *skb, int *source_port, > + int *switch_id, int *vbid, u16 *vid) > +{ > + if (vid_is_dsa_8021q(skb_vlan_tag_get(skb) & VLAN_VID_MASK)) > + return dsa_8021q_rcv(skb, source_port, switch_id, vbid); > + > + /* Try our best with imprecise RX */ > + *vid = skb_vlan_tag_get(skb) & VLAN_VID_MASK; > +} > + > +static struct sk_buff *vsc73xx_rcv(struct sk_buff *skb, struct net_device *netdev) > +{ > + int src_port = -1, switch_id = -1, vbid = -1; > + u16 vid; > + > + if (skb_vlan_tag_present(skb)) { > + /* Normal traffic path. */ > + vsc73xx_vlan_rcv(skb, &src_port, &switch_id, &vbid, &vid); > + } else { > + netdev_warn(netdev, "Couldn't decode source port\n"); > + return NULL; > + } > + > + if (vbid >= 1) > + skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid); > + else if (src_port == -1 || switch_id == -1) > + skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid); > + else > + skb->dev = dsa_master_find_slave(netdev, switch_id, src_port); Hmm, this isn't looking too good. I think the fact that you had to add my copyright on what should be such a simple thing as a VLAN-based tagger is a bad sign :) It's time to consolidate some more stuff that currently lives in tag_sja1105 and move it into tag_8021q so that you can reuse it more easily. I've prepared some (only compile-tested) patches on this branch here: https://github.com/vladimiroltean/linux/commits/pawel-dembicki-vsc73xx-v3 I need to double-check that they don't introduce regressions, and we should discuss the merging strategy. Probably you're going to submit them together with your patch set. With that, you can drop my part of the copyright :) The remainder should look like straightforward use of API which can be written in only a limited number of ways. > + if (!skb->dev) { > + netdev_warn(netdev, "Couldn't decode source port\n"); > + return NULL; > + } > + > + dsa_default_offload_fwd_mark(skb); > + > + if (dsa_port_is_vlan_filtering(dsa_slave_to_port(skb->dev)) && > + eth_hdr(skb)->h_proto == htons(ETH_P_8021Q)) > + __vlan_hwaccel_clear_tag(skb); Why do you need to do this? We send VLAN-tagged packets to the VLAN-aware bridge intentionally, so that it knows what VLAN they come from (in the dsa_find_designated_bridge_port_by_vid() case). So don't strip it if it's not causing a problem. > + > + return skb; > +}
wt., 12 wrz 2023 o 23:39 Vladimir Oltean <olteanv@gmail.com> napisał(a): > > Hi Pawel, Hi Vladimir, > > On Tue, Sep 12, 2023 at 02:22:00PM +0200, Pawel Dembicki wrote: > > This commit introduces a new tagger based on 802.1q tagging. > > It's designed for the vsc73xx driver. The VSC73xx family doesn't have > > any tag support for the RGMII port, but it could be based on VLANs. > > > > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com> > > --- > > diff --git a/net/dsa/tag_vsc73xx_8021q.c b/net/dsa/tag_vsc73xx_8021q.c > > new file mode 100644 > > index 000000000000..9093a71e6eb0 > > --- /dev/null > > +++ b/net/dsa/tag_vsc73xx_8021q.c > > @@ -0,0 +1,91 @@ > > +// SPDX-License-Identifier: GPL-2.0 OR MIT > > +/* Copyright (C) 2022 Pawel Dembicki <paweldembicki@gmail.com> > > 2022-2023 by now, maybe? > > > + * Based on tag_sja1105.c: > > + * Copyright (c) 2019, Vladimir Oltean <olteanv@gmail.com> > > + */ > > +#include <linux/dsa/8021q.h> > > + > > +#include "tag.h" > > +#include "tag_8021q.h" > > + > > +#define VSC73XX_8021Q_NAME "vsc73xx-8021q" > > + > > +static struct sk_buff *vsc73xx_xmit(struct sk_buff *skb, struct net_device *netdev) > > +{ > > + struct dsa_port *dp = dsa_slave_to_port(netdev); > > + u16 queue_mapping = skb_get_queue_mapping(skb); > > + u16 tx_vid = dsa_tag_8021q_standalone_vid(dp); > > + u8 pcp; > > + > > + if (skb->offload_fwd_mark) { > > + unsigned int bridge_num = dsa_port_bridge_num_get(dp); > > + struct net_device *br = dsa_port_bridge_dev_get(dp); > > + > > + if (br_vlan_enabled(br)) > > + return skb; > > + > > + tx_vid = dsa_tag_8021q_bridge_vid(bridge_num); > > + } > > + > > + pcp = netdev_txq_to_tc(netdev, queue_mapping); > > + > > + return dsa_8021q_xmit(skb, netdev, ETH_P_8021Q, > > + ((pcp << VLAN_PRIO_SHIFT) | tx_vid)); > > +} > > + > > +static void vsc73xx_vlan_rcv(struct sk_buff *skb, int *source_port, > > + int *switch_id, int *vbid, u16 *vid) > > +{ > > + if (vid_is_dsa_8021q(skb_vlan_tag_get(skb) & VLAN_VID_MASK)) > > + return dsa_8021q_rcv(skb, source_port, switch_id, vbid); > > + > > + /* Try our best with imprecise RX */ > > + *vid = skb_vlan_tag_get(skb) & VLAN_VID_MASK; > > +} > > + > > +static struct sk_buff *vsc73xx_rcv(struct sk_buff *skb, struct net_device *netdev) > > +{ > > + int src_port = -1, switch_id = -1, vbid = -1; > > + u16 vid; > > + > > + if (skb_vlan_tag_present(skb)) { > > + /* Normal traffic path. */ > > + vsc73xx_vlan_rcv(skb, &src_port, &switch_id, &vbid, &vid); > > + } else { > > + netdev_warn(netdev, "Couldn't decode source port\n"); > > + return NULL; > > + } > > + > > + if (vbid >= 1) > > + skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid); > > + else if (src_port == -1 || switch_id == -1) > > + skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid); > > + else > > + skb->dev = dsa_master_find_slave(netdev, switch_id, src_port); > > Hmm, this isn't looking too good. > > I think the fact that you had to add my copyright on what should be such > a simple thing as a VLAN-based tagger is a bad sign :) > > It's time to consolidate some more stuff that currently lives in > tag_sja1105 and move it into tag_8021q so that you can reuse it more > easily. > > I've prepared some (only compile-tested) patches on this branch here: > https://github.com/vladimiroltean/linux/commits/pawel-dembicki-vsc73xx-v3 > > I need to double-check that they don't introduce regressions, I tested it on my device and I couldn't find any regressions. vlan filtering and tagging work as expected. > and we > should discuss the merging strategy. Probably you're going to submit > them together with your patch set. I prepared the v4 patch series. Please look if that format is ok with you. https://github.com/CHKDSK88/linux/commits/vsc73xx-vlan-net-next > > With that, you can drop my part of the copyright :) The remainder should > look like straightforward use of API which can be written in only a > limited number of ways. Now it is much simpler. > > > + if (!skb->dev) { > > + netdev_warn(netdev, "Couldn't decode source port\n"); > > + return NULL; > > + } > > + > > + dsa_default_offload_fwd_mark(skb); > > + > > + if (dsa_port_is_vlan_filtering(dsa_slave_to_port(skb->dev)) && > > + eth_hdr(skb)->h_proto == htons(ETH_P_8021Q)) > > + __vlan_hwaccel_clear_tag(skb); > > Why do you need to do this? We send VLAN-tagged packets to the > VLAN-aware bridge intentionally, so that it knows what VLAN they come > from (in the dsa_find_designated_bridge_port_by_vid() case). So don't > strip it if it's not causing a problem. > I dropped it in v4. I needed it when I started with this patch series, because bridge in vlan filtering causes double tagged frames (one from hardware and one from bridge). But after recent changes it is useless and it could be dropped because vlan works as expected without it. > > + > > + return skb; > > +}
On Mon, Sep 25, 2023 at 10:55:29PM +0200, Paweł Dembicki wrote: > > I've prepared some (only compile-tested) patches on this branch here: > > https://github.com/vladimiroltean/linux/commits/pawel-dembicki-vsc73xx-v3 > > > > I need to double-check that they don't introduce regressions, > > I tested it on my device and I couldn't find any regressions. vlan > filtering and tagging work as expected. Thanks for testing. Obviously I still haven't :-/ Please feel free to post the next version and I'll try to find some time to test. > > and we > > should discuss the merging strategy. Probably you're going to submit > > them together with your patch set. > > I prepared the v4 patch series. Please look if that format is ok with you. > https://github.com/CHKDSK88/linux/commits/vsc73xx-vlan-net-next I still have some style nitpicks similar to the ones expressed already, but it isn't trivial for me to leave them through Github, so you can post your best attempt at v4. You should also try to add a comment explaining what's the reason for the special case for "if (port != CPU_PORT)" in vsc73xx_port_vlan_add().
diff --git a/include/net/dsa.h b/include/net/dsa.h index 0b9c6aa27047..f83454d4ad02 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -56,6 +56,7 @@ struct phylink_link_state; #define DSA_TAG_PROTO_RTL8_4T_VALUE 25 #define DSA_TAG_PROTO_RZN1_A5PSW_VALUE 26 #define DSA_TAG_PROTO_LAN937X_VALUE 27 +#define DSA_TAG_PROTO_VSC73XX_8021Q_VALUE 28 enum dsa_tag_protocol { DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE, @@ -86,6 +87,7 @@ enum dsa_tag_protocol { DSA_TAG_PROTO_RTL8_4T = DSA_TAG_PROTO_RTL8_4T_VALUE, DSA_TAG_PROTO_RZN1_A5PSW = DSA_TAG_PROTO_RZN1_A5PSW_VALUE, DSA_TAG_PROTO_LAN937X = DSA_TAG_PROTO_LAN937X_VALUE, + DSA_TAG_PROTO_VSC73XX_8021Q = DSA_TAG_PROTO_VSC73XX_8021Q_VALUE, }; struct dsa_switch; diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig index 8e698bea99a3..e59360071c67 100644 --- a/net/dsa/Kconfig +++ b/net/dsa/Kconfig @@ -166,6 +166,12 @@ config NET_DSA_TAG_TRAILER Say Y or M if you want to enable support for tagging frames at with a trailed. e.g. Marvell 88E6060. +config NET_DSA_TAG_VSC73XX_8021Q + tristate "Tag driver for Microchip/Vitesse VSC73xx family of switches, using VLAN" + help + Say Y or M if you want to enable support for tagging frames with a + custom VLAN-based header. + config NET_DSA_TAG_XRS700X tristate "Tag driver for XRS700x switches" help diff --git a/net/dsa/Makefile b/net/dsa/Makefile index 12e305824a96..bab8a933c514 100644 --- a/net/dsa/Makefile +++ b/net/dsa/Makefile @@ -37,6 +37,7 @@ obj-$(CONFIG_NET_DSA_TAG_RTL8_4) += tag_rtl8_4.o obj-$(CONFIG_NET_DSA_TAG_RZN1_A5PSW) += tag_rzn1_a5psw.o obj-$(CONFIG_NET_DSA_TAG_SJA1105) += tag_sja1105.o obj-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o +obj-$(CONFIG_NET_DSA_TAG_VSC73XX_8021Q) += tag_vsc73xx_8021q.o obj-$(CONFIG_NET_DSA_TAG_XRS700X) += tag_xrs700x.o # for tracing framework to find trace.h diff --git a/net/dsa/tag_vsc73xx_8021q.c b/net/dsa/tag_vsc73xx_8021q.c new file mode 100644 index 000000000000..9093a71e6eb0 --- /dev/null +++ b/net/dsa/tag_vsc73xx_8021q.c @@ -0,0 +1,91 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT +/* Copyright (C) 2022 Pawel Dembicki <paweldembicki@gmail.com> + * Based on tag_sja1105.c: + * Copyright (c) 2019, Vladimir Oltean <olteanv@gmail.com> + */ +#include <linux/dsa/8021q.h> + +#include "tag.h" +#include "tag_8021q.h" + +#define VSC73XX_8021Q_NAME "vsc73xx-8021q" + +static struct sk_buff *vsc73xx_xmit(struct sk_buff *skb, struct net_device *netdev) +{ + struct dsa_port *dp = dsa_slave_to_port(netdev); + u16 queue_mapping = skb_get_queue_mapping(skb); + u16 tx_vid = dsa_tag_8021q_standalone_vid(dp); + u8 pcp; + + if (skb->offload_fwd_mark) { + unsigned int bridge_num = dsa_port_bridge_num_get(dp); + struct net_device *br = dsa_port_bridge_dev_get(dp); + + if (br_vlan_enabled(br)) + return skb; + + tx_vid = dsa_tag_8021q_bridge_vid(bridge_num); + } + + pcp = netdev_txq_to_tc(netdev, queue_mapping); + + return dsa_8021q_xmit(skb, netdev, ETH_P_8021Q, + ((pcp << VLAN_PRIO_SHIFT) | tx_vid)); +} + +static void vsc73xx_vlan_rcv(struct sk_buff *skb, int *source_port, + int *switch_id, int *vbid, u16 *vid) +{ + if (vid_is_dsa_8021q(skb_vlan_tag_get(skb) & VLAN_VID_MASK)) + return dsa_8021q_rcv(skb, source_port, switch_id, vbid); + + /* Try our best with imprecise RX */ + *vid = skb_vlan_tag_get(skb) & VLAN_VID_MASK; +} + +static struct sk_buff *vsc73xx_rcv(struct sk_buff *skb, struct net_device *netdev) +{ + int src_port = -1, switch_id = -1, vbid = -1; + u16 vid; + + if (skb_vlan_tag_present(skb)) { + /* Normal traffic path. */ + vsc73xx_vlan_rcv(skb, &src_port, &switch_id, &vbid, &vid); + } else { + netdev_warn(netdev, "Couldn't decode source port\n"); + return NULL; + } + + if (vbid >= 1) + skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid); + else if (src_port == -1 || switch_id == -1) + skb->dev = dsa_find_designated_bridge_port_by_vid(netdev, vid); + else + skb->dev = dsa_master_find_slave(netdev, switch_id, src_port); + if (!skb->dev) { + netdev_warn(netdev, "Couldn't decode source port\n"); + return NULL; + } + + dsa_default_offload_fwd_mark(skb); + + if (dsa_port_is_vlan_filtering(dsa_slave_to_port(skb->dev)) && + eth_hdr(skb)->h_proto == htons(ETH_P_8021Q)) + __vlan_hwaccel_clear_tag(skb); + + return skb; +} + +static const struct dsa_device_ops vsc73xx_8021q_netdev_ops = { + .name = VSC73XX_8021Q_NAME, + .proto = DSA_TAG_PROTO_VSC73XX_8021Q, + .xmit = vsc73xx_xmit, + .rcv = vsc73xx_rcv, + .needed_headroom = VLAN_HLEN, + .promisc_on_master = true, +}; + +MODULE_LICENSE("GPL"); +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_VSC73XX_8021Q, VSC73XX_8021Q_NAME); + +module_dsa_tag_driver(vsc73xx_8021q_netdev_ops);
This commit introduces a new tagger based on 802.1q tagging. It's designed for the vsc73xx driver. The VSC73xx family doesn't have any tag support for the RGMII port, but it could be based on VLANs. Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com> --- v3: - Introduce a patch after the tagging patch split include/net/dsa.h | 2 + net/dsa/Kconfig | 6 +++ net/dsa/Makefile | 1 + net/dsa/tag_vsc73xx_8021q.c | 91 +++++++++++++++++++++++++++++++++++++ 4 files changed, 100 insertions(+) create mode 100644 net/dsa/tag_vsc73xx_8021q.c