| Message ID | 20240619205220.965844-5-paweldembicki@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: vsc73xx: Implement VLAN operations | expand |
On Wed, Jun 19, 2024 at 10:52:10PM +0200, Pawel Dembicki wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > tag_sja1105 has a wrapper over dsa_8021q_rcv(): sja1105_vlan_rcv(), > which determines whether the packet came from a bridge with > vlan_filtering=1 (the case resolved via > dsa_find_designated_bridge_port_by_vid()), or if it contains a tag_8021q > header. > > Looking at a new tagger implementation for vsc73xx, based also on > tag_8021q, it is becoming clear that the logic is needed there as well. > So instead of forcing each tagger to wrap around dsa_8021q_rcv(), let's > merge the logic into the core. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com> > --- > v2,v1: > - resend only > --- > Before patch series split: > https://patchwork.kernel.org/project/netdevbpf/list/?series=841034&state=%2A&archive=both > v8, v7, v6: > - resend only > v5: > - add missing SoB > v4: > - introduced patch > --- > net/dsa/tag_8021q.c | 34 ++++++++++++++++++++++++++++------ > net/dsa/tag_8021q.h | 2 +- > net/dsa/tag_ocelot_8021q.c | 2 +- > net/dsa/tag_sja1105.c | 32 ++++---------------------------- > 4 files changed, 34 insertions(+), 36 deletions(-) > > diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c > index 3cb0293793a5..332b0ae02645 100644 > --- a/net/dsa/tag_8021q.c > +++ b/net/dsa/tag_8021q.c > @@ -507,27 +507,39 @@ EXPORT_SYMBOL_GPL(dsa_tag_8021q_find_port_by_vbid); > * @vbid: pointer to storage for imprecise bridge ID. Must be pre-initialized > * with -1. If a positive value is returned, the source_port and switch_id > * are invalid. > + * @vid: pointer to storage for original VID, in case tag_8021q decoding failed. > + * > + * If the packet has a tag_8021q header, decode it and set @source_port, > + * @switch_id and @vbid, and strip the header. Otherwise set @vid and keep the > + * header in the hwaccel area of the packet. > */ > void dsa_8021q_rcv(struct sk_buff *skb, int *source_port, int *switch_id, > - int *vbid) > + int *vbid, int *vid) > { > int tmp_source_port, tmp_switch_id, tmp_vbid; > - u16 vid, tci; > + __be16 vlan_proto; > + u16 tmp_vid, tci; > > if (skb_vlan_tag_present(skb)) { > + vlan_proto = skb->vlan_proto; > tci = skb_vlan_tag_get(skb); > __vlan_hwaccel_clear_tag(skb); > } else { > + struct vlan_ethhdr *hdr = vlan_eth_hdr(skb); > + > + vlan_proto = hdr->h_vlan_proto; > skb_push_rcsum(skb, ETH_HLEN); > __skb_vlan_pop(skb, &tci); > skb_pull_rcsum(skb, ETH_HLEN); > } > > - vid = tci & VLAN_VID_MASK; > + tmp_vid = tci & VLAN_VID_MASK; > + if (!vid_is_dsa_8021q(tmp_vid)) > + goto not_tag_8021q; I think this may be more clearly expressed linearly, without a goto. if (!vid_is_dsa_8021q(tmp_vid)) { /* Not a tag_8021q frame, so return the VID to the * caller for further processing, and put the tag back */ if (vid) *vid = tmp_vid; __vlan_hwaccel_put_tag(skb, vlan_proto, tci); return; } > > - tmp_source_port = dsa_8021q_rx_source_port(vid); > - tmp_switch_id = dsa_8021q_rx_switch_id(vid); > - tmp_vbid = dsa_tag_8021q_rx_vbid(vid); > + tmp_source_port = dsa_8021q_rx_source_port(tmp_vid); > + tmp_switch_id = dsa_8021q_rx_switch_id(tmp_vid); > + tmp_vbid = dsa_tag_8021q_rx_vbid(tmp_vid); > > /* Precise source port information is unknown when receiving from a > * VLAN-unaware bridging domain, and tmp_source_port and tmp_switch_id > @@ -546,5 +558,15 @@ void dsa_8021q_rcv(struct sk_buff *skb, int *source_port, int *switch_id, > *vbid = tmp_vbid; > > skb->priority = (tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT; > + return; > + > +not_tag_8021q: > + if (vid) > + *vid = tmp_vid; > + if (vbid) > + *vbid = -1; Thinking more about it, I don't think this is needed (hence it is missing in my rewritten snippet above). I mean, *vbid is already initialized with -1 (it's a requirement also specified in the kernel-doc) and we haven't changed it. > + > + /* Put the tag back */ > + __vlan_hwaccel_put_tag(skb, vlan_proto, tci); > } > EXPORT_SYMBOL_GPL(dsa_8021q_rcv); > diff --git a/net/dsa/tag_8021q.h b/net/dsa/tag_8021q.h > index 41f7167ac520..0c6671d7c1c2 100644 > --- a/net/dsa/tag_8021q.h > +++ b/net/dsa/tag_8021q.h > @@ -14,7 +14,7 @@ struct sk_buff *dsa_8021q_xmit(struct sk_buff *skb, struct net_device *netdev, > u16 tpid, u16 tci); > > void dsa_8021q_rcv(struct sk_buff *skb, int *source_port, int *switch_id, > - int *vbid); > + int *vbid, int *vid); > > struct net_device *dsa_tag_8021q_find_port_by_vbid(struct net_device *conduit, > int vbid);
On Wed, Jun 19, 2024 at 10:52:10PM +0200, Pawel Dembicki wrote: > +not_tag_8021q: > + if (vid) > + *vid = tmp_vid; > + if (vbid) > + *vbid = -1; I can confirm that the vbid assignment isn't needed here. Reviewed-by: Vladimir Oltean <olteanv@gmail.com> Tested-by: Vladimir Oltean <olteanv@gmail.com> > + > + /* Put the tag back */ > + __vlan_hwaccel_put_tag(skb, vlan_proto, tci); > }
diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c index 3cb0293793a5..332b0ae02645 100644 --- a/net/dsa/tag_8021q.c +++ b/net/dsa/tag_8021q.c @@ -507,27 +507,39 @@ EXPORT_SYMBOL_GPL(dsa_tag_8021q_find_port_by_vbid); * @vbid: pointer to storage for imprecise bridge ID. Must be pre-initialized * with -1. If a positive value is returned, the source_port and switch_id * are invalid. + * @vid: pointer to storage for original VID, in case tag_8021q decoding failed. + * + * If the packet has a tag_8021q header, decode it and set @source_port, + * @switch_id and @vbid, and strip the header. Otherwise set @vid and keep the + * header in the hwaccel area of the packet. */ void dsa_8021q_rcv(struct sk_buff *skb, int *source_port, int *switch_id, - int *vbid) + int *vbid, int *vid) { int tmp_source_port, tmp_switch_id, tmp_vbid; - u16 vid, tci; + __be16 vlan_proto; + u16 tmp_vid, tci; if (skb_vlan_tag_present(skb)) { + vlan_proto = skb->vlan_proto; tci = skb_vlan_tag_get(skb); __vlan_hwaccel_clear_tag(skb); } else { + struct vlan_ethhdr *hdr = vlan_eth_hdr(skb); + + vlan_proto = hdr->h_vlan_proto; skb_push_rcsum(skb, ETH_HLEN); __skb_vlan_pop(skb, &tci); skb_pull_rcsum(skb, ETH_HLEN); } - vid = tci & VLAN_VID_MASK; + tmp_vid = tci & VLAN_VID_MASK; + if (!vid_is_dsa_8021q(tmp_vid)) + goto not_tag_8021q; - tmp_source_port = dsa_8021q_rx_source_port(vid); - tmp_switch_id = dsa_8021q_rx_switch_id(vid); - tmp_vbid = dsa_tag_8021q_rx_vbid(vid); + tmp_source_port = dsa_8021q_rx_source_port(tmp_vid); + tmp_switch_id = dsa_8021q_rx_switch_id(tmp_vid); + tmp_vbid = dsa_tag_8021q_rx_vbid(tmp_vid); /* Precise source port information is unknown when receiving from a * VLAN-unaware bridging domain, and tmp_source_port and tmp_switch_id @@ -546,5 +558,15 @@ void dsa_8021q_rcv(struct sk_buff *skb, int *source_port, int *switch_id, *vbid = tmp_vbid; skb->priority = (tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT; + return; + +not_tag_8021q: + if (vid) + *vid = tmp_vid; + if (vbid) + *vbid = -1; + + /* Put the tag back */ + __vlan_hwaccel_put_tag(skb, vlan_proto, tci); } EXPORT_SYMBOL_GPL(dsa_8021q_rcv); diff --git a/net/dsa/tag_8021q.h b/net/dsa/tag_8021q.h index 41f7167ac520..0c6671d7c1c2 100644 --- a/net/dsa/tag_8021q.h +++ b/net/dsa/tag_8021q.h @@ -14,7 +14,7 @@ struct sk_buff *dsa_8021q_xmit(struct sk_buff *skb, struct net_device *netdev, u16 tpid, u16 tci); void dsa_8021q_rcv(struct sk_buff *skb, int *source_port, int *switch_id, - int *vbid); + int *vbid, int *vid); struct net_device *dsa_tag_8021q_find_port_by_vbid(struct net_device *conduit, int vbid); diff --git a/net/dsa/tag_ocelot_8021q.c b/net/dsa/tag_ocelot_8021q.c index b059381310fe..8e8b1bef6af6 100644 --- a/net/dsa/tag_ocelot_8021q.c +++ b/net/dsa/tag_ocelot_8021q.c @@ -81,7 +81,7 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb, { int src_port, switch_id; - dsa_8021q_rcv(skb, &src_port, &switch_id, NULL); + dsa_8021q_rcv(skb, &src_port, &switch_id, NULL, NULL); skb->dev = dsa_conduit_find_user(netdev, switch_id, src_port); if (!skb->dev) diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c index 48886d4b7e3e..7639ccb94d35 100644 --- a/net/dsa/tag_sja1105.c +++ b/net/dsa/tag_sja1105.c @@ -472,37 +472,14 @@ static bool sja1110_skb_has_inband_control_extension(const struct sk_buff *skb) return ntohs(eth_hdr(skb)->h_proto) == ETH_P_SJA1110; } -/* If the VLAN in the packet is a tag_8021q one, set @source_port and - * @switch_id and strip the header. Otherwise set @vid and keep it in the - * packet. - */ -static void sja1105_vlan_rcv(struct sk_buff *skb, int *source_port, - int *switch_id, int *vbid, u16 *vid) -{ - struct vlan_ethhdr *hdr = vlan_eth_hdr(skb); - u16 vlan_tci; - - if (skb_vlan_tag_present(skb)) - vlan_tci = skb_vlan_tag_get(skb); - else - vlan_tci = ntohs(hdr->h_vlan_TCI); - - if (vid_is_dsa_8021q(vlan_tci & VLAN_VID_MASK)) - return dsa_8021q_rcv(skb, source_port, switch_id, vbid); - - /* Try our best with imprecise RX */ - *vid = vlan_tci & VLAN_VID_MASK; -} - static struct sk_buff *sja1105_rcv(struct sk_buff *skb, struct net_device *netdev) { - int source_port = -1, switch_id = -1, vbid = -1; + int source_port = -1, switch_id = -1, vbid = -1, vid = -1; struct sja1105_meta meta = {0}; struct ethhdr *hdr; bool is_link_local; bool is_meta; - u16 vid; hdr = eth_hdr(skb); is_link_local = sja1105_is_link_local(skb); @@ -525,7 +502,7 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb, * a tag_8021q VLAN which we have to strip */ if (sja1105_skb_has_tag_8021q(skb)) - sja1105_vlan_rcv(skb, &source_port, &switch_id, &vbid, &vid); + dsa_8021q_rcv(skb, &source_port, &switch_id, &vbid, &vid); else if (source_port == -1 && switch_id == -1) /* Packets with no source information have no chance of * getting accepted, drop them straight away. @@ -660,9 +637,8 @@ static struct sk_buff *sja1110_rcv_inband_control_extension(struct sk_buff *skb, static struct sk_buff *sja1110_rcv(struct sk_buff *skb, struct net_device *netdev) { - int source_port = -1, switch_id = -1, vbid = -1; + int source_port = -1, switch_id = -1, vbid = -1, vid = -1; bool host_only = false; - u16 vid = 0; if (sja1110_skb_has_inband_control_extension(skb)) { skb = sja1110_rcv_inband_control_extension(skb, &source_port, @@ -674,7 +650,7 @@ static struct sk_buff *sja1110_rcv(struct sk_buff *skb, /* Packets with in-band control extensions might still have RX VLANs */ if (likely(sja1105_skb_has_tag_8021q(skb))) - sja1105_vlan_rcv(skb, &source_port, &switch_id, &vbid, &vid); + dsa_8021q_rcv(skb, &source_port, &switch_id, &vbid, &vid); if (vbid >= 1) skb->dev = dsa_tag_8021q_find_port_by_vbid(netdev, vbid);