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;
 
-	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);
-- 
2.34.1

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);
>  }

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);