[PATCH RFC net 0/2] net: dsa: felix: fix VLAN-unaware reception

[PATCH RFC net 0/2] net: dsa: felix: fix VLAN-unaware reception

[Robert Hodaszi]

Hello,

This patchset supposed to fix the currently non-working ocelot-8021q
mode of the Felix driver if bridge is VLAN-unaware.

As can see in the commit messages, the driver enables
'untag_vlan_aware_bridge_pvid' to software VLAN untag all packets, but
tagging is only enabled if VLAN-filtering is enabled
(push_inner_tag=1).

Untagging packets from VLAN-unaware bridge ports is wrong, and corrupts
the packets.

It was tempting to simply restore dsa_software_vlan_untag()'s checking
as it was before:

  /* Move VLAN tag from data to hwaccel **
  if (!skb_vlan_tag_present(skb) && skb->protocol == htons(proto)) {
    skb = skb_vlan_untag(skb);
    if (!skb)
      return NULL;
  }

And so untagging only VLAN packets, but that's not really a solution,
VLAN-tagged packets may arrive on VLAN-unaware ports, and those would
get untagged incorrectly.

So I added a way to mark ports as untagged when untagging is enabled,
and return without altering the packet.

Robert Hodaszi (2):
  net: dsa: provide a way to exclude switch ports from VLAN untagging
  net: dsa: felix: fix reception from VLAN-unaware ports

 drivers/net/dsa/ocelot/felix.c | 9 ++++++++-
 include/net/dsa.h              | 7 +++++++
 net/dsa/tag.h                  | 4 ++++
 3 files changed, 19 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH RFC net 1/2] net: dsa: provide a way to exclude switch ports from VLAN untagging

[Robert Hodaszi]

Even if a switch driver sets 'untag_vlan_aware_bridge_pvid' or
'untag_bridge_pvid', it may not enable tagging on one or more ports
(VLAN-unaware port). Without a way to exclude these ports,
dsa_software_vlan_untag() untags packets it shouldn't, and so it
corrupts the packet.

That happens currently with the felix driver. The driver always sets
'untag_vlan_aware_bridge_pvid', but enables inner tags
(push_inner_tag=1) only when VLAN-filtering is enabled.

To exclude a port from untagging, set dp->not_tagged=1. The inverse
logic is to keep backward compatibility, and avoid the need to
potentially change currently working users.

Fixes: f1288fd7293b ("net: dsa: felix: fix VLAN tag loss on CPU reception with ocelot-8021q")
Signed-off-by: Robert Hodaszi <robert.hodaszi@digi.com>
---
 include/net/dsa.h | 7 +++++++
 net/dsa/tag.h     | 4 ++++
 2 files changed, 11 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 72ae65e7246a..99b1267f0c46 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -289,6 +289,13 @@ struct dsa_port {
 
 	u8			setup:1;
 
+	/* Use this to mark a switch port as "untagged", to avoid
+	 * dsa_software_vlan_untag() do a VLAN untagging on it if
+	 * ds->untag_bridge_pvid or ds->untag_vlan_aware_bridge_pvid is set to
+	 * true
+	 */
+	u8			not_tagged:1;
+
 	struct device_node	*dn;
 	unsigned int		ageing_time;
 
diff --git a/net/dsa/tag.h b/net/dsa/tag.h
index d5707870906b..c3d76a49b1c6 100644
--- a/net/dsa/tag.h
+++ b/net/dsa/tag.h
@@ -155,6 +155,10 @@ static inline struct sk_buff *dsa_software_vlan_untag(struct sk_buff *skb)
 	if (!br)
 		return skb;
 
+	/* This switch port is not tagged, no need for untagging */
+	if (dp->not_tagged)
+		return skb;
+
 	/* Move VLAN tag from data to hwaccel */
 	if (!skb_vlan_tag_present(skb)) {
 		skb = skb_vlan_untag(skb);
-- 
2.43.0

[PATCH RFC net 2/2] net: dsa: felix: fix reception from VLAN-unaware ports

[Robert Hodaszi]

In ocelot-8021q mode, the driver always enables
'untag_vlan_aware_bridge_pvid' to do software VLAN untagging, no matter
if tagging is enabled on the port (VLAN-aware) or not (VLAN-unaware).
That corrupts packets on VLAN-unaware ports.

Exclude the port from VLAN untagging by setting the 'not_tagged' port
flag on VLAN-unaware ports.

Fixes: f1288fd7293b ("net: dsa: felix: fix VLAN tag loss on CPU reception with ocelot-8021q")
Signed-off-by: Robert Hodaszi <robert.hodaszi@digi.com>
---
 drivers/net/dsa/ocelot/felix.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 3aa9c997018a..8a2650a428ec 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -273,6 +273,7 @@ static int felix_tag_8021q_vlan_add(struct dsa_switch *ds, int port, u16 vid,
 	struct dsa_port *dp = dsa_to_port(ds, port);
 	struct dsa_port *cpu_dp;
 	int err;
+	bool tagging_enabled;
 
 	/* tag_8021q.c assumes we are implementing this via port VLAN
 	 * membership, which we aren't. So we don't need to add any VCAP filter
@@ -281,9 +282,11 @@ static int felix_tag_8021q_vlan_add(struct dsa_switch *ds, int port, u16 vid,
 	if (!dsa_port_is_user(dp))
 		return 0;
 
+	tagging_enabled = dsa_port_is_vlan_filtering(dp);
+
 	dsa_switch_for_each_cpu_port(cpu_dp, ds) {
 		err = felix_tag_8021q_vlan_add_rx(ds, port, cpu_dp->index, vid,
-						  dsa_port_is_vlan_filtering(dp));
+						  tagging_enabled);
 		if (err)
 			return err;
 	}
@@ -292,6 +295,8 @@ static int felix_tag_8021q_vlan_add(struct dsa_switch *ds, int port, u16 vid,
 	if (err)
 		goto add_tx_failed;
 
+	dp->not_tagged = !tagging_enabled;
+
 	return 0;
 
 add_tx_failed:
@@ -320,6 +325,8 @@ static int felix_tag_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
 	if (err)
 		goto del_tx_failed;
 
+	dp->not_tagged = 0;
+
 	return 0;
 
 del_tx_failed:
-- 
2.43.0

Re: [PATCH RFC net 0/2] net: dsa: felix: fix VLAN-unaware reception

[Vladimir Oltean]

On Sun, Dec 15, 2024 at 05:33:32PM +0100, Robert Hodaszi wrote:
> Hello,
> 
> This patchset supposed to fix the currently non-working ocelot-8021q
> mode of the Felix driver if bridge is VLAN-unaware.
> 
> As can see in the commit messages, the driver enables
> 'untag_vlan_aware_bridge_pvid' to software VLAN untag all packets, but
> tagging is only enabled if VLAN-filtering is enabled
> (push_inner_tag=1).
> 
> Untagging packets from VLAN-unaware bridge ports is wrong, and corrupts
> the packets.
> 
> It was tempting to simply restore dsa_software_vlan_untag()'s checking
> as it was before:
> 
>   /* Move VLAN tag from data to hwaccel **
>   if (!skb_vlan_tag_present(skb) && skb->protocol == htons(proto)) {
>     skb = skb_vlan_untag(skb);
>     if (!skb)
>       return NULL;
>   }
> 
> And so untagging only VLAN packets, but that's not really a solution,
> VLAN-tagged packets may arrive on VLAN-unaware ports, and those would
> get untagged incorrectly.
> 
> So I added a way to mark ports as untagged when untagging is enabled,
> and return without altering the packet.

Give me an example traffic pattern, Linux configuration and corruption,
please. I spent a lot of time trying to make sure I am not introducing
regressions, and I have no idea what you are seeing that is wrong.
Please don't try to make assumptions, just let me see what you see.

Re: [PATCH RFC net 0/2] net: dsa: felix: fix VLAN-unaware reception

[Robert Hodaszi]

On Sunday, 15.12.2024 at 18:09 +0100, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> 
> Give me an example traffic pattern, Linux configuration and corruption,
> please. I spent a lot of time trying to make sure I am not introducing
> regressions, and I have no idea what you are seeing that is wrong.
> Please don't try to make assumptions, just let me see what you see.

The config I'm using:
 - Using the 2.5Gbps as CPU port in 'ocelot-8021q' mode, Linux interface name is 'eth0'
 - Using 2 downstream ports as external Ethernet ports: 'eth1' and 'eth2'
 - 'eth1' port of the device is directly connected with my PC (Ethernet interface #1, 192.168.1.1)
 - 'eth2' port of the device is directly connected with my PC (Ethernet interface #2, 192.168.2.1)

DTS:

  &mscc_felix_port0 {
    label = "eth1";
    managed = "in-band-status";
    phy-handle = <&qsgmii_phy0>;
    phy-mode = "qsgmii";
    status = "okay";
  };

  &mscc_felix_port1 {
    label = "eth2";
    managed = "in-band-status";
    phy-handle = <&qsgmii_phy1>;
    phy-mode = "qsgmii";
    status = "okay";
  };

  &mscc_felix_port4 {
    ethernet = <&enetc_port2>;
    status = "okay";
    dsa-tag-protocol = "ocelot-8021q";
  };

LS1028 unit's Linux config:

  # Static IP to 'eth1'
  $ ifconfig eth1 192.168.1.2 up

  # Create a VLAN-unaware bridge, and add 'eth2' to that
  $ brctl addbr br0
  $ brctl addif br0 eth2

  # Set static IP to the bridge
  $ ifconfig br0 192.168.2.2 up
  $ ifconfig eth2 up

Now at this point:

  1. I can ping perfectly fine the eth1 interface from my PC ("ping 192.168.1.2"), and vice-versa
  2. Pinging 'br0' from my PC is not working ("ping 192.168.2.2"). I can see the ARP requests, but there are not ARP replies at all.

If I enable VLAN-filtering on 'br0', it starts working:

  $ echo 1 > /sys/class/net/br0/bridge/vlan_filtering


So basically:

  1. Raw interface -> working
  2. VLAN-aware bridge -> working
  3. VLAN-unaware bridge -> NOT working

I traced what is happening. When VLAN-filtering is not enabled on the bridge, LS1028's switch is configured with 'push_inner_tag = OCELOT_NO_ES0_TAG'. But ds->untag_vlan_aware_bridge_pvid is always set to true at switch setup, in felix_tag_8021q_setup(). That makes dsa_switch_rcv() call dsa_software_vlan_untag() for each packets.


Now in dsa_software_vlan_untag(), if the port is not part of the bridge (case #1), it returns with the skb early. That's OK.


  static inline struct sk_buff *dsa_software_vlan_untag(struct sk_buff *skb)
  {
    struct dsa_port *dp = dsa_user_to_port(skb->dev);
    struct net_device *br = dsa_port_bridge_dev_get(dp);
    u16 vid;

    /* software untagging for standalone ports not yet necessary */
    if (!br)
      return skb;


But if port is part of a bridge, no matter "push_inner_tag" is set as OCELOT_ES0_TAG or OCELOT_NO_ES0_TAG, it always untags it:

    /* Move VLAN tag from data to hwaccel */
    if (!skb_vlan_tag_present(skb)) {
      skb = skb_vlan_untag(skb);
      if (!skb)
        return NULL;
    }

As the "untag_vlan_aware_bridge_pvid" is a switch-specific thing, not port-specific, I cannot change it to false/true depending on the port is added to a VLAN-unaware/aware bridge, as the other port may be added to another bridge (eth1 -> VLAN-aware (tags enabled), eth2 -> VLAN-unaware (tags disabled)).

Also, in the past this code part looked like this:

    /* Move VLAN tag from data to hwaccel */
    if (!skb_vlan_tag_present(skb) && skb->protocol == htons(proto)) {
      skb = skb_vlan_untag(skb);
      if (!skb)
        return NULL;
    }

So we had a protocol check. This wouldn't work 100% neither, because what if a VLAN packet arrives from the outer world into a VLAN-unaware bridge? I assume, that shouldn't be untagged, still, it would do that.


I'm not that happy with my patch though, as I had to add another flag for each ports. But that seems to be the "cleanest" solution. That's why as marked it as RFC.

Thanks,
Robert

Re: [PATCH RFC net 0/2] net: dsa: felix: fix VLAN-unaware reception

[Vladimir Oltean]

On Mon, Dec 16, 2024 at 11:10:05AM +0100, Robert Hodaszi wrote:
> On Sunday, 15.12.2024 at 18:09 +0100, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> > 
> > Give me an example traffic pattern, Linux configuration and corruption,
> > please. I spent a lot of time trying to make sure I am not introducing
> > regressions, and I have no idea what you are seeing that is wrong.
> > Please don't try to make assumptions, just let me see what you see.
> 
> The config I'm using:
>  - Using the 2.5Gbps as CPU port in 'ocelot-8021q' mode, Linux interface name is 'eth0'
>  - Using 2 downstream ports as external Ethernet ports: 'eth1' and 'eth2'
>  - 'eth1' port of the device is directly connected with my PC (Ethernet interface #1, 192.168.1.1)
>  - 'eth2' port of the device is directly connected with my PC (Ethernet interface #2, 192.168.2.1)
> 
> DTS:
> 
>   &mscc_felix_port0 {
>     label = "eth1";
>     managed = "in-band-status";
>     phy-handle = <&qsgmii_phy0>;
>     phy-mode = "qsgmii";
>     status = "okay";
>   };
> 
>   &mscc_felix_port1 {
>     label = "eth2";
>     managed = "in-band-status";
>     phy-handle = <&qsgmii_phy1>;
>     phy-mode = "qsgmii";
>     status = "okay";
>   };
> 
>   &mscc_felix_port4 {
>     ethernet = <&enetc_port2>;
>     status = "okay";
>     dsa-tag-protocol = "ocelot-8021q";
>   };
> 
> LS1028 unit's Linux config:
> 
>   # Static IP to 'eth1'
>   $ ifconfig eth1 192.168.1.2 up
> 
>   # Create a VLAN-unaware bridge, and add 'eth2' to that
>   $ brctl addbr br0
>   $ brctl addif br0 eth2
> 
>   # Set static IP to the bridge
>   $ ifconfig br0 192.168.2.2 up
>   $ ifconfig eth2 up
> 
> Now at this point:
> 
>   1. I can ping perfectly fine the eth1 interface from my PC ("ping 192.168.1.2"), and vice-versa
>   2. Pinging 'br0' from my PC is not working ("ping 192.168.2.2"). I can see the ARP requests, but there are not ARP replies at all.
> 
> If I enable VLAN-filtering on 'br0', it starts working:
> 
>   $ echo 1 > /sys/class/net/br0/bridge/vlan_filtering
> 
> 
> So basically:
> 
>   1. Raw interface -> working
>   2. VLAN-aware bridge -> working
>   3. VLAN-unaware bridge -> NOT working
> 
> I traced what is happening. When VLAN-filtering is not enabled on the bridge, LS1028's switch is configured with 'push_inner_tag = OCELOT_NO_ES0_TAG'. But ds->untag_vlan_aware_bridge_pvid is always set to true at switch setup, in felix_tag_8021q_setup(). That makes dsa_switch_rcv() call dsa_software_vlan_untag() for each packets.
> 
> 
> Now in dsa_software_vlan_untag(), if the port is not part of the bridge (case #1), it returns with the skb early. That's OK.
> 
> 
>   static inline struct sk_buff *dsa_software_vlan_untag(struct sk_buff *skb)
>   {
>     struct dsa_port *dp = dsa_user_to_port(skb->dev);
>     struct net_device *br = dsa_port_bridge_dev_get(dp);
>     u16 vid;
> 
>     /* software untagging for standalone ports not yet necessary */
>     if (!br)
>       return skb;
> 
> 
> But if port is part of a bridge, no matter "push_inner_tag" is set as OCELOT_ES0_TAG or OCELOT_NO_ES0_TAG, it always untags it:
> 
>     /* Move VLAN tag from data to hwaccel */
>     if (!skb_vlan_tag_present(skb)) {
>       skb = skb_vlan_untag(skb);
>       if (!skb)
>         return NULL;
>     }
> 
> As the "untag_vlan_aware_bridge_pvid" is a switch-specific thing, not port-specific, I cannot change it to false/true depending on the port is added to a VLAN-unaware/aware bridge, as the other port may be added to another bridge (eth1 -> VLAN-aware (tags enabled), eth2 -> VLAN-unaware (tags disabled)).
> 
> Also, in the past this code part looked like this:
> 
>     /* Move VLAN tag from data to hwaccel */
>     if (!skb_vlan_tag_present(skb) && skb->protocol == htons(proto)) {
>       skb = skb_vlan_untag(skb);
>       if (!skb)
>         return NULL;
>     }
> 
> So we had a protocol check. This wouldn't work 100% neither, because what if a VLAN packet arrives from the outer world into a VLAN-unaware bridge? I assume, that shouldn't be untagged, still, it would do that.
> 
> 
> I'm not that happy with my patch though, as I had to add another flag for each ports. But that seems to be the "cleanest" solution. That's why as marked it as RFC.
> 
> Thanks,
> Robert

The memory is starting to come back :-|

Ok, so the good news is that you aren't seeing things, I can reproduce
with tools/testing/selftests/drivers/net/dsa/local_termination.sh.

Another good thing is that the fix is easier than your posted attempt.
You've correctly identified the previous VLAN stripping logic, and that
is what we should go forward with. I don't agree with your analysis that
it wouldn't work, because if you look at the implementation of
skb_vlan_untag(), it strips the VLAN header from the skb head, but still
keeps it in the hwaccel area, so packets are still VLAN-tagged.

This does not have a functional impact upon reception, it is just done
to have unified handling later on in the function:
skb_vlan_tag_present() and skb_vlan_tag_get_id(). This side effect is
also mentioned as a comment on dsa_software_vlan_untag().

The stripping itself will only take place in dsa_software_untag_vlan_unaware_bridge()
if the switch driver sets dp->ds->untag_bridge_pvid. The felix driver
does not set this.

What is not so good is that I'm seriously starting to doubt my sanity.
You'd think that I ran the selftests that I had posted together with the
patch introducing the bug, but somehow they fail :-| And not only that,
but thoughts about this problem itself have since passed through my head,
and I failed to correctly identify where the problem applies and where
it does not. I'm sorry for that.

I've just posted a fix to this bug, which I would like you to double-check
and respond with review and test tags, or let me know if it doesn't work.
https://lore.kernel.org/netdev/20241216135059.1258266-1-vladimir.oltean@nxp.com/
I posted it myself because I don't expect you to have the full context
(it's a bug that I introduced), and with yours there are still a lot of
unanswered "why"s, as well as not the simplest solution.

Re: [PATCH RFC net 0/2] net: dsa: felix: fix VLAN-unaware reception

[Robert Hodaszi]

On Monday, 16.12.2024 at 14:51 +0100, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> 
> The memory is starting to come back :-|
> 
> Ok, so the good news is that you aren't seeing things, I can reproduce
> with tools/testing/selftests/drivers/net/dsa/https://linkprotect.cudasvc.com/url?a=https%3a%2f%2flocal_termination.sh&c=E,1,Ez0FDT_USqvD0083KxZU7x7ffGuJDoeCC6xMtetczJrwErBfCEyO1pnImOnY_ifhHDMKhhPtJGv8MpKk1zKoqa6Gm1JP-zTkotP2AOShxr3N&typo=1
> 
> Another good thing is that the fix is easier than your posted attempt.
> You've correctly identified the previous VLAN stripping logic, and that
> is what we should go forward with. I don't agree with your analysis that
> it wouldn't work, because if you look at the implementation of
> skb_vlan_untag(), it strips the VLAN header from the skb head, but still
> keeps it in the hwaccel area, so packets are still VLAN-tagged.
> 
> This does not have a functional impact upon reception, it is just done
> to have unified handling later on in the function:
> skb_vlan_tag_present() and skb_vlan_tag_get_id(). This side effect is
> also mentioned as a comment on dsa_software_vlan_untag().
> 
> The stripping itself will only take place in dsa_software_untag_vlan_unaware_bridge()
> if the switch driver sets dp->ds->untag_bridge_pvid. The felix driver
> does not set this.
> 
> What is not so good is that I'm seriously starting to doubt my sanity.
> You'd think that I ran the selftests that I had posted together with the
> patch introducing the bug, but somehow they fail :-| And not only that,
> but thoughts about this problem itself have since passed through my head,
> and I failed to correctly identify where the problem applies and where
> it does not. I'm sorry for that.
> 
> I've just posted a fix to this bug, which I would like you to double-check
> and respond with review and test tags, or let me know if it doesn't work.
> https://linkprotect.cudasvc.com/url?a=https%3a%2f%2flore.kernel.org%2fnetdev%2f20241216135059.1258266-1-vladimir.oltean%40nxp.com%2f&c=E,1,iMsl_DfLMdZXF3FfFIT1CISQcjOL417WIsr7z01GodEy-1vyX9d_6X-8hFJih2CA2zAax4kx2mFdtftzn-ELRkGDBCa9lxIWU_wEN8dtO2aVO7NS7ck,&typo=1
> I posted it myself because I don't expect you to have the full context
> (it's a bug that I introduced), and with yours there are still a lot of
> unanswered "why"s, as well as not the simplest solution.

Actually, what you did is exactly what I did first to fix the issue, but it broke my setup when I sent VLAN-tagged messages to the device. Now I tested again, and it is working fine. That made me think it's happening because it is stripping incorrectly the VLAN tag. Probably it was just an incorrect setup, maybe something remained set either on my PC or on the unit from the previous test.

One thing is different to my change though: you're calling the br_vlan_get_proto() twice. You can tweak performance a bit probably, if you rather pass 'proto' to both dsa_software_untag_vlan_aware_bridge and dsa_software_untag_vlan_unaware_bridge instead. So something like this:

diff --git a/net/dsa/tag.h b/net/dsa/tag.h
index d5707870906b..3d790d8e16cd 100644
--- a/net/dsa/tag.h
+++ b/net/dsa/tag.h
@@ -57,15 +57,11 @@ static inline struct net_device *dsa_conduit_find_user(struct net_device *dev,
  */
 static inline void dsa_software_untag_vlan_aware_bridge(struct sk_buff *skb,
 							struct net_device *br,
-							u16 vid)
+							u16 vid, u16 proto)
 {
-	u16 pvid, proto;
+	u16 pvid;
 	int err;
 
-	err = br_vlan_get_proto(br, &proto);
-	if (err)
-		return;
-
 	err = br_vlan_get_pvid_rcu(skb->dev, &pvid);
 	if (err)
 		return;
@@ -103,16 +99,12 @@ static inline void dsa_software_untag_vlan_aware_bridge(struct sk_buff *skb,
  */
 static inline void dsa_software_untag_vlan_unaware_bridge(struct sk_buff *skb,
 							  struct net_device *br,
-							  u16 vid)
+							  u16 vid, u16 proto)
 {
 	struct net_device *upper_dev;
-	u16 pvid, proto;
+	u16 pvid;
 	int err;
 
-	err = br_vlan_get_proto(br, &proto);
-	if (err)
-		return;
-
 	err = br_vlan_get_pvid_rcu(skb->dev, &pvid);
 	if (err)
 		return;
@@ -149,14 +141,19 @@ static inline struct sk_buff *dsa_software_vlan_untag(struct sk_buff *skb)
 {
 	struct dsa_port *dp = dsa_user_to_port(skb->dev);
 	struct net_device *br = dsa_port_bridge_dev_get(dp);
-	u16 vid;
+	u16 vid, proto;
+	int err;
 
 	/* software untagging for standalone ports not yet necessary */
 	if (!br)
 		return skb;
 
+	err = br_vlan_get_proto(br, &proto);
+	if (err)
+		return skb;
+
 	/* Move VLAN tag from data to hwaccel */
-	if (!skb_vlan_tag_present(skb)) {
+	if (!skb_vlan_tag_present(skb) && skb->protocol == htons(proto)) {
 		skb = skb_vlan_untag(skb);
 		if (!skb)
 			return NULL;
@@ -169,10 +166,12 @@ static inline struct sk_buff *dsa_software_vlan_untag(struct sk_buff *skb)
 
 	if (br_vlan_enabled(br)) {
 		if (dp->ds->untag_vlan_aware_bridge_pvid)
-			dsa_software_untag_vlan_aware_bridge(skb, br, vid);
+			dsa_software_untag_vlan_aware_bridge(skb, br, vid,
+							     proto);
 	} else {
 		if (dp->ds->untag_bridge_pvid)
-			dsa_software_untag_vlan_unaware_bridge(skb, br, vid);
+			dsa_software_untag_vlan_unaware_bridge(skb, br, vid,
+							       proto);
 	}
 
 	return skb;


Robert

Re: [PATCH RFC net 0/2] net: dsa: felix: fix VLAN-unaware reception

[Vladimir Oltean]

On Mon, Dec 16, 2024 at 03:39:17PM +0100, Robert Hodaszi wrote:
> Actually, what you did is exactly what I did first to fix the issue, but it broke my setup when I sent VLAN-tagged messages to the device. Now I tested again, and it is working fine. That made me think it's happening because it is stripping incorrectly the VLAN tag. Probably it was just an incorrect setup, maybe something remained set either on my PC or on the unit from the previous test.
> 
> One thing is different to my change though: you're calling the br_vlan_get_proto() twice. You can tweak performance a bit probably, if you rather pass 'proto' to both dsa_software_untag_vlan_aware_bridge and dsa_software_untag_vlan_unaware_bridge instead. So something like this:

The patch is going to become stuffy, but ok. We also have to update the
kernel-doc of the 2 untagging functions to document the new argument.
I will send a v2 tomorrow after the expiry of the 24 hour timeout for
other review comments.

Re: [PATCH RFC net 0/2] net: dsa: felix: fix VLAN-unaware reception

[Simon Horman]

On Mon, Dec 16, 2024 at 04:48:31PM +0200, Vladimir Oltean wrote:
> On Mon, Dec 16, 2024 at 03:39:17PM +0100, Robert Hodaszi wrote:
> > Actually, what you did is exactly what I did first to fix the issue, but it broke my setup when I sent VLAN-tagged messages to the device. Now I tested again, and it is working fine. That made me think it's happening because it is stripping incorrectly the VLAN tag. Probably it was just an incorrect setup, maybe something remained set either on my PC or on the unit from the previous test.
> > 
> > One thing is different to my change though: you're calling the br_vlan_get_proto() twice. You can tweak performance a bit probably, if you rather pass 'proto' to both dsa_software_untag_vlan_aware_bridge and dsa_software_untag_vlan_unaware_bridge instead. So something like this:
> 
> The patch is going to become stuffy, but ok. We also have to update the
> kernel-doc of the 2 untagging functions to document the new argument.
> I will send a v2 tomorrow after the expiry of the 24 hour timeout for
> other review comments.

Hi Vladimir,

As you are touching both Kernel doc and dsa_software_vlan_untag,
could you consider also adding a "Returns: " section to the Kernel doc
of that function?