[PATCH net v2] cxgb4: flower: fix 802.1ad VLAN TPID matching in tc flower filters

[PATCH net v2] cxgb4: flower: fix 802.1ad VLAN TPID matching in tc flower filters

[Harsha M]

The cxgb4 driver does not correctly handle tc flower filters that
match on an 802.1ad (Q-in-Q) outer VLAN tag. While the VLAN VID is
processed, the specified 802.1ad TPID is not programmed into the
adapter's outer VLAN matching configuration, resulting in incorrect
filter matches.

Fix this by configuring the port-specific OVLAN register with the
requested TPID value, enabling OVLAN matching through the RX control
register, and populating the filter specification with the outer VLAN
VID and valid-bit match fields when an 802.1ad TPID is requested.This
restores correct matching for tc flower filters that specify an 802.1ad
outer VLAN tag

Signed-off-by: Harsha M <h.mahadeva@chelsio.com>
Signed-off-by: Potnuri Bharat Teja <bharat@chelsio.com>
---
v2:
- Compare vlan_tpid with 'cpu_to_be16(ETH_P_8021AD)'
- Program the OVLAN register using the ETH_P_8021AD constant
  and TPID mask as 0xffff instead of deriving the value from the
  filter key and mask
v1:
https://lore.kernel.org/all/20260629184417.31b21223@kernel.org/
---
 .../ethernet/chelsio/cxgb4/cxgb4_tc_flower.c  | 61 +++++++++++++------
 drivers/net/ethernet/chelsio/cxgb4/t4_regs.h  |  3 +
 2 files changed, 46 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
index 3307e5042681..8c5cfa6982e7 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
@@ -40,6 +40,7 @@
 #include "cxgb4.h"
 #include "cxgb4_filter.h"
 #include "cxgb4_tc_flower.h"
+#include "t4_regs.h"
 
 #define STATS_CHECK_PERIOD (HZ / 2)
 
@@ -266,24 +267,48 @@ static void cxgb4_process_flow_match(struct net_device *dev,
 					       VLAN_PRIO_SHIFT);
 		vlan_tci_mask = match.mask->vlan_id | (match.mask->vlan_priority <<
 						     VLAN_PRIO_SHIFT);
-		fs->val.ivlan = vlan_tci;
-		fs->mask.ivlan = vlan_tci_mask;
-
-		fs->val.ivlan_vld = 1;
-		fs->mask.ivlan_vld = 1;
-
-		/* Chelsio adapters use ivlan_vld bit to match vlan packets
-		 * as 802.1Q. Also, when vlan tag is present in packets,
-		 * ethtype match is used then to match on ethtype of inner
-		 * header ie. the header following the vlan header.
-		 * So, set the ivlan_vld based on ethtype info supplied by
-		 * TC for vlan packets if its 802.1Q. And then reset the
-		 * ethtype value else, hw will try to match the supplied
-		 * ethtype value with ethtype of inner header.
-		 */
-		if (fs->val.ethtype == ETH_P_8021Q) {
-			fs->val.ethtype = 0;
-			fs->mask.ethtype = 0;
+
+		if (match.key->vlan_tpid == cpu_to_be16(ETH_P_8021AD)) {
+			struct adapter *adap = netdev2adap(dev);
+			u32 ovlan_reg, ctl_reg, val, port_id;
+
+			if (!adap) {
+				netdev_err(dev, "%s: adap not found\n", __func__);
+				return;
+			}
+
+			val = (0xffff << 16) | ETH_P_8021AD;
+			port_id = netdev2pinfo(dev)->port_id;
+			fs->val.ovlan = vlan_tci;
+			fs->mask.ovlan = vlan_tci_mask;
+			fs->val.ovlan_vld = 1;
+			fs->mask.ovlan_vld = 1;
+			ovlan_reg = PORT_REG(port_id, MPS_PORT_RX_OVLAN0_A);
+			ctl_reg = PORT_REG(port_id, MPS_PORT_RX_CTL_A);
+			t4_write_reg(adap, ovlan_reg, val);
+			val = t4_read_reg(adap, ctl_reg);
+			t4_write_reg(adap, ctl_reg, val | 1);
+			t4_tp_wr_bits_indirect(adap, TP_INGRESS_CONFIG_A, 1U << 9, 0);
+		} else {
+			fs->val.ivlan = vlan_tci;
+			fs->mask.ivlan = vlan_tci_mask;
+			fs->val.ivlan_vld = 1;
+			fs->mask.ivlan_vld = 1;
+
+			/* Chelsio adapters use ivlan_vld bit to match vlan packets
+			 * as 802.1Q. Also, when vlan tag is present in packets,
+			 * ethtype match is used then to match on ethtype of inner
+			 * header ie. the header following the vlan header.
+			 * So, set the ivlan_vld based on ethtype info supplied by
+			 * TC for vlan packets if its 802.1Q. And then reset the
+			 * ethtype value else, hw will try to match the supplied
+			 * ethtype value with ethtype of inner header.
+			 */
+
+			if (fs->val.ethtype == ETH_P_8021Q) {
+				fs->val.ethtype = 0;
+				fs->mask.ethtype = 0;
+			}
 		}
 	}
 
diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h b/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h
index 695916ba0405..38c585f3b1ad 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h
@@ -1921,6 +1921,9 @@
 #define MAC_PORT_PTP_SUM_LO_A 0x990
 #define MAC_PORT_PTP_SUM_HI_A 0x994
 
+#define MPS_PORT_RX_OVLAN0_A 0x120
+#define MPS_PORT_RX_CTL_A    0X100
+
 #define MPS_CMN_CTL_A	0x9000
 
 #define COUNTPAUSEMCRX_S    5
-- 
2.43.5

RE: [PATCH net v2] cxgb4: flower: fix 802.1ad VLAN TPID matching in tc flower filters

[Jagielski, Jedrzej]

From: Harsha M <h.mahadeva@chelsio.com> 
Sent: Thursday, July 2, 2026 6:49 AM

>The cxgb4 driver does not correctly handle tc flower filters that
>match on an 802.1ad (Q-in-Q) outer VLAN tag. While the VLAN VID is
>processed, the specified 802.1ad TPID is not programmed into the
>adapter's outer VLAN matching configuration, resulting in incorrect
>filter matches.
>
>Fix this by configuring the port-specific OVLAN register with the
>requested TPID value, enabling OVLAN matching through the RX control
>register, and populating the filter specification with the outer VLAN
>VID and valid-bit match fields when an 802.1ad TPID is requested.This
>restores correct matching for tc flower filters that specify an 802.1ad
>outer VLAN tag
>

Hi

would be nice to have fixes tag

>Signed-off-by: Harsha M <h.mahadeva@chelsio.com>
>Signed-off-by: Potnuri Bharat Teja <bharat@chelsio.com>
>---
>v2:
>- Compare vlan_tpid with 'cpu_to_be16(ETH_P_8021AD)'
>- Program the OVLAN register using the ETH_P_8021AD constant
>  and TPID mask as 0xffff instead of deriving the value from the
>  filter key and mask
>v1:
>https://lore.kernel.org/all/20260629184417.31b21223@kernel.org/
>---
> .../ethernet/chelsio/cxgb4/cxgb4_tc_flower.c  | 61 +++++++++++++------
> drivers/net/ethernet/chelsio/cxgb4/t4_regs.h  |  3 +
> 2 files changed, 46 insertions(+), 18 deletions(-)
>
>diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
>index 3307e5042681..8c5cfa6982e7 100644
>--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
>+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
>@@ -40,6 +40,7 @@
> #include "cxgb4.h"
> #include "cxgb4_filter.h"
> #include "cxgb4_tc_flower.h"
>+#include "t4_regs.h"
> 
> #define STATS_CHECK_PERIOD (HZ / 2)
> 
>@@ -266,24 +267,48 @@ static void cxgb4_process_flow_match(struct net_device *dev,
> 					       VLAN_PRIO_SHIFT);
> 		vlan_tci_mask = match.mask->vlan_id | (match.mask->vlan_priority <<
> 						     VLAN_PRIO_SHIFT);
>-		fs->val.ivlan = vlan_tci;
>-		fs->mask.ivlan = vlan_tci_mask;
>-
>-		fs->val.ivlan_vld = 1;
>-		fs->mask.ivlan_vld = 1;
>-
>-		/* Chelsio adapters use ivlan_vld bit to match vlan packets
>-		 * as 802.1Q. Also, when vlan tag is present in packets,
>-		 * ethtype match is used then to match on ethtype of inner
>-		 * header ie. the header following the vlan header.
>-		 * So, set the ivlan_vld based on ethtype info supplied by
>-		 * TC for vlan packets if its 802.1Q. And then reset the
>-		 * ethtype value else, hw will try to match the supplied
>-		 * ethtype value with ethtype of inner header.
>-		 */
>-		if (fs->val.ethtype == ETH_P_8021Q) {
>-			fs->val.ethtype = 0;
>-			fs->mask.ethtype = 0;
>+
>+		if (match.key->vlan_tpid == cpu_to_be16(ETH_P_8021AD)) {
>+			struct adapter *adap = netdev2adap(dev);
>+			u32 ovlan_reg, ctl_reg, val, port_id;
>+
>+			if (!adap) {

is such scenario really even possible?

>+				netdev_err(dev, "%s: adap not found\n", __func__);
>+				return;
>+			}
>+
>+			val = (0xffff << 16) | ETH_P_8021AD;
>+			port_id = netdev2pinfo(dev)->port_id;
>+			fs->val.ovlan = vlan_tci;
>+			fs->mask.ovlan = vlan_tci_mask;
>+			fs->val.ovlan_vld = 1;
>+			fs->mask.ovlan_vld = 1;
>+			ovlan_reg = PORT_REG(port_id, MPS_PORT_RX_OVLAN0_A);
>+			ctl_reg = PORT_REG(port_id, MPS_PORT_RX_CTL_A);
>+			t4_write_reg(adap, ovlan_reg, val);
>+			val = t4_read_reg(adap, ctl_reg);
>+			t4_write_reg(adap, ctl_reg, val | 1);
>+			t4_tp_wr_bits_indirect(adap, TP_INGRESS_CONFIG_A, 1U << 9, 0);
>+		} else {
>+			fs->val.ivlan = vlan_tci;
>+			fs->mask.ivlan = vlan_tci_mask;
>+			fs->val.ivlan_vld = 1;
>+			fs->mask.ivlan_vld = 1;
>+
>+			/* Chelsio adapters use ivlan_vld bit to match vlan packets
>+			 * as 802.1Q. Also, when vlan tag is present in packets,
>+			 * ethtype match is used then to match on ethtype of inner
>+			 * header ie. the header following the vlan header.
>+			 * So, set the ivlan_vld based on ethtype info supplied by
>+			 * TC for vlan packets if its 802.1Q. And then reset the
>+			 * ethtype value else, hw will try to match the supplied
>+			 * ethtype value with ethtype of inner header.
>+			 */
>+

isn't this already fs->val.ethtype != ETH_P_8021Q branch and fs has not
been modified since entering?

>+			if (fs->val.ethtype == ETH_P_8021Q) {
>+				fs->val.ethtype = 0;
>+				fs->mask.ethtype = 0;
>+			}
> 		}
> 	}
> 
>diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h b/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h
>index 695916ba0405..38c585f3b1ad 100644
>--- a/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h
>+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h
>@@ -1921,6 +1921,9 @@
> #define MAC_PORT_PTP_SUM_LO_A 0x990
> #define MAC_PORT_PTP_SUM_HI_A 0x994
> 
>+#define MPS_PORT_RX_OVLAN0_A 0x120
>+#define MPS_PORT_RX_CTL_A    0X100
>+
> #define MPS_CMN_CTL_A	0x9000
> 
> #define COUNTPAUSEMCRX_S    5
>-- 
>2.43.5

Re: [PATCH net v2] cxgb4: flower: fix 802.1ad VLAN TPID matching in tc flower filters

[Harsha Mahadeva]

On Thursday, July 07/02/26, 2026 at 13:05:42 +0530, Jagielski, Jedrzej wrote:
> From: Harsha M <h.mahadeva@chelsio.com> 
> Sent: Thursday, July 2, 2026 6:49 AM
> 
> >The cxgb4 driver does not correctly handle tc flower filters that
> >match on an 802.1ad (Q-in-Q) outer VLAN tag. While the VLAN VID is
> >processed, the specified 802.1ad TPID is not programmed into the
> >adapter's outer VLAN matching configuration, resulting in incorrect
> >filter matches.
> >
> >Fix this by configuring the port-specific OVLAN register with the
> >requested TPID value, enabling OVLAN matching through the RX control
> >register, and populating the filter specification with the outer VLAN
> >VID and valid-bit match fields when an 802.1ad TPID is requested.This
> >restores correct matching for tc flower filters that specify an 802.1ad
> >outer VLAN tag
> >
> 
> Hi

Hi Jedrzej,
Thanks for reviewing the patch
> 
> would be nice to have fixes tag

Sure, I'll add the appropriate Fixes: tag in the next revision
> 
> >Signed-off-by: Harsha M <h.mahadeva@chelsio.com>
> >Signed-off-by: Potnuri Bharat Teja <bharat@chelsio.com>
> >---
> >v2:
> >- Compare vlan_tpid with 'cpu_to_be16(ETH_P_8021AD)'
> >- Program the OVLAN register using the ETH_P_8021AD constant
> >  and TPID mask as 0xffff instead of deriving the value from the
> >  filter key and mask
> >v1:
> >https://lore.kernel.org/all/20260629184417.31b21223@kernel.org/
> >---
> > .../ethernet/chelsio/cxgb4/cxgb4_tc_flower.c  | 61 +++++++++++++------
> > drivers/net/ethernet/chelsio/cxgb4/t4_regs.h  |  3 +
> > 2 files changed, 46 insertions(+), 18 deletions(-)
> >
> >diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> >index 3307e5042681..8c5cfa6982e7 100644
> >--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> >+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> >@@ -40,6 +40,7 @@
> > #include "cxgb4.h"
> > #include "cxgb4_filter.h"
> > #include "cxgb4_tc_flower.h"
> >+#include "t4_regs.h"
> > 
> > #define STATS_CHECK_PERIOD (HZ / 2)
> > 
> >@@ -266,24 +267,48 @@ static void cxgb4_process_flow_match(struct net_device *dev,
> > 					       VLAN_PRIO_SHIFT);
> > 		vlan_tci_mask = match.mask->vlan_id | (match.mask->vlan_priority <<
> > 						     VLAN_PRIO_SHIFT);
> >-		fs->val.ivlan = vlan_tci;
> >-		fs->mask.ivlan = vlan_tci_mask;
> >-
> >-		fs->val.ivlan_vld = 1;
> >-		fs->mask.ivlan_vld = 1;
> >-
> >-		/* Chelsio adapters use ivlan_vld bit to match vlan packets
> >-		 * as 802.1Q. Also, when vlan tag is present in packets,
> >-		 * ethtype match is used then to match on ethtype of inner
> >-		 * header ie. the header following the vlan header.
> >-		 * So, set the ivlan_vld based on ethtype info supplied by
> >-		 * TC for vlan packets if its 802.1Q. And then reset the
> >-		 * ethtype value else, hw will try to match the supplied
> >-		 * ethtype value with ethtype of inner header.
> >-		 */
> >-		if (fs->val.ethtype == ETH_P_8021Q) {
> >-			fs->val.ethtype = 0;
> >-			fs->mask.ethtype = 0;
> >+
> >+		if (match.key->vlan_tpid == cpu_to_be16(ETH_P_8021AD)) {
> >+			struct adapter *adap = netdev2adap(dev);
> >+			u32 ovlan_reg, ctl_reg, val, port_id;
> >+
> >+			if (!adap) {
> 
> is such scenario really even possible?

You're right. netdev2adap(dev) should always return a valid adapter in this path, so I'll remove the unnecessary NULL check
> 
> >+				netdev_err(dev, "%s: adap not found\n", __func__);
> >+				return;
> >+			}
> >+
> >+			val = (0xffff << 16) | ETH_P_8021AD;
> >+			port_id = netdev2pinfo(dev)->port_id;
> >+			fs->val.ovlan = vlan_tci;
> >+			fs->mask.ovlan = vlan_tci_mask;
> >+			fs->val.ovlan_vld = 1;
> >+			fs->mask.ovlan_vld = 1;
> >+			ovlan_reg = PORT_REG(port_id, MPS_PORT_RX_OVLAN0_A);
> >+			ctl_reg = PORT_REG(port_id, MPS_PORT_RX_CTL_A);
> >+			t4_write_reg(adap, ovlan_reg, val);
> >+			val = t4_read_reg(adap, ctl_reg);
> >+			t4_write_reg(adap, ctl_reg, val | 1);
> >+			t4_tp_wr_bits_indirect(adap, TP_INGRESS_CONFIG_A, 1U << 9, 0);
> >+		} else {
> >+			fs->val.ivlan = vlan_tci;
> >+			fs->mask.ivlan = vlan_tci_mask;
> >+			fs->val.ivlan_vld = 1;
> >+			fs->mask.ivlan_vld = 1;
> >+
> >+			/* Chelsio adapters use ivlan_vld bit to match vlan packets
> >+			 * as 802.1Q. Also, when vlan tag is present in packets,
> >+			 * ethtype match is used then to match on ethtype of inner
> >+			 * header ie. the header following the vlan header.
> >+			 * So, set the ivlan_vld based on ethtype info supplied by
> >+			 * TC for vlan packets if its 802.1Q. And then reset the
> >+			 * ethtype value else, hw will try to match the supplied
> >+			 * ethtype value with ethtype of inner header.
> >+			 */
> >+
> 
> isn't this already fs->val.ethtype != ETH_P_8021Q branch and fs has not
> been modified since entering?

This branch is based on match.key->vlan_tpid != ETH_P_8021AD, not on fs->val.ethtype
> 
> >+			if (fs->val.ethtype == ETH_P_8021Q) {
> >+				fs->val.ethtype = 0;
> >+				fs->mask.ethtype = 0;
> >+			}
> > 		}
> > 	}
> > 
> >diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h b/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h
> >index 695916ba0405..38c585f3b1ad 100644
> >--- a/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h
> >+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h
> >@@ -1921,6 +1921,9 @@
> > #define MAC_PORT_PTP_SUM_LO_A 0x990
> > #define MAC_PORT_PTP_SUM_HI_A 0x994
> > 
> >+#define MPS_PORT_RX_OVLAN0_A 0x120
> >+#define MPS_PORT_RX_CTL_A    0X100
> >+
> > #define MPS_CMN_CTL_A	0x9000
> > 
> > #define COUNTPAUSEMCRX_S    5
> >-- 
> >2.43.5
> 
> 

Re: [PATCH net v2] cxgb4: flower: fix 802.1ad VLAN TPID matching in tc flower filters

[Paolo Abeni]

From: AI Reviewer <ai@example.com>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
cxgb4: flower: fix 802.1ad VLAN TPID matching in tc flower filters

This patch adds an 802.1ad branch to cxgb4_process_flow_match() that
programs a port-specific OVLAN register with ETH_P_8021AD, sets bit 0
of the RX control register, clears bit 9 of TP_INGRESS_CONFIG, and
populates the filter's outer-VLAN match fields.

A few questions about the new code follow inline.

> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> index 3307e5042681..8c5cfa6982e7 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> @@ -266,24 +267,48 @@ static void cxgb4_process_flow_match(struct net_device *dev,
>  					       VLAN_PRIO_SHIFT);
>  		vlan_tci_mask = match.mask->vlan_id | (match.mask->vlan_priority <<
>  						     VLAN_PRIO_SHIFT);
> -		fs->val.ivlan = vlan_tci;
> -		fs->mask.ivlan = vlan_tci_mask;
> -
> -		fs->val.ivlan_vld = 1;
> -		fs->mask.ivlan_vld = 1;
> -
> -		/* Chelsio adapters use ivlan_vld bit to match vlan packets
> [ ... ]
> -		if (fs->val.ethtype == ETH_P_8021Q) {
> -			fs->val.ethtype = 0;
> -			fs->mask.ethtype = 0;
> +
> +		if (match.key->vlan_tpid == cpu_to_be16(ETH_P_8021AD)) {

[Medium]
The gating condition consults only match.key->vlan_tpid and does not
look at match.mask->vlan_tpid.  In tc-flower semantics a key value is
only significant to the extent the mask covers it.

If a rule sets key=0x88a8 with mask=0 (don't care about TPID), this
branch is still entered and the adapter registers are still
reprogrammed.  Should this test be gated on the mask as well, similar
to how mlx5 and nfp consult the mask before honoring the key?

> +			struct adapter *adap = netdev2adap(dev);
> +			u32 ovlan_reg, ctl_reg, val, port_id;
> +
> +			if (!adap) {
> +				netdev_err(dev, "%s: adap not found\n", __func__);
> +				return;
> +			}

[Low]
netdev2adap() expands to netdev2pinfo(dev)->adapter and is not expected
to return NULL for a netdev that tc-flower can bind to.  If the check
ever did fire, cxgb4_process_flow_match() is void, so this early return
also skips the trailing:

    fs->val.iport = netdev2pinfo(dev)->port_id;
    fs->mask.iport = ~0;

and cxgb4_flow_rule_replace() would proceed to install a filter with
iport=0 / iport_mask=0.  Should the check either be removed, or the
function refactored to propagate an error?

> +
> +			val = (0xffff << 16) | ETH_P_8021AD;

[Low]
The literal 0xffff has type int (signed), so 0xffff << 16 evaluates to
a value that is not representable in a signed int.  Per C11 6.5.7 this
is undefined behavior and UBSAN's shift sanitizer flags it.  Would
0xffffU << 16 (or a symbolic _V()/_M() macro) be preferable?

The layout of MPS_PORT_RX_OVLAN0_A ([31:16]=mask, [15:0]=value) is also
not expressed through the driver's usual _S/_V/_F/_M helpers used by
neighboring registers.  Could symbolic field macros be added?

> +			port_id = netdev2pinfo(dev)->port_id;
> +			fs->val.ovlan = vlan_tci;
> +			fs->mask.ovlan = vlan_tci_mask;
> +			fs->val.ovlan_vld = 1;
> +			fs->mask.ovlan_vld = 1;

[High]
Does this ovlan programming survive on adapters that have VNIC_F set in
the cached ingress_config?  In cxgb4_filter.c, both __cxgb4_set_filter()
and cxgb4_set_hash_filter() unconditionally do:

    if (adap->params.tp.ingress_config & VNIC_F) {
        fs->val.ovlan = (fs->val.pf << 13) | fs->val.vf;
        fs->val.ovlan_vld = fs->val.pfvf_vld;
        ...
    }

which overwrites the fs->val.ovlan / fs->val.ovlan_vld set here.
validate_filter() also rejects with -EOPNOTSUPP when ovlan_vld is set
together with VNIC_F, or when the filter mode does not include
VNIC_ID_F.

On adapters where VNIC_F is enabled, does this branch actually restore
matching, or does it silently lose the VID (and, per the next comment,
still mutate hardware before failing validation)?

> +			ovlan_reg = PORT_REG(port_id, MPS_PORT_RX_OVLAN0_A);
> +			ctl_reg = PORT_REG(port_id, MPS_PORT_RX_CTL_A);
> +			t4_write_reg(adap, ovlan_reg, val);

[High]
cxgb4_process_flow_match() is called from cxgb4_flow_rule_replace()
before __cxgb4_set_filter() runs validate_filter() and the mailbox/L2T
install.  Do these three writes need to be deferred until after
validation and successful install, so that a filter rejected by
validate_filter(), or one that fails allocation/mailbox/L2T setup,
does not leave the port and adapter in a reconfigured state with no
installed filter to justify it?

Additionally, clear_filter() (cxgb4_filter.c) only tears down L2T/SMT/
CLIP and memsets the filter entry.  It does not undo MPS_PORT_RX_OVLAN0,
MPS_PORT_RX_CTL bit 0, or TP_INGRESS_CONFIG bit 9.  Is a corresponding
teardown expected on filter delete/replace so that 'tc filter add' +
'tc filter del' does not leave OVLAN matching permanently enabled and
TP_INGRESS_CONFIG bit 9 permanently cleared?

MPS_PORT_RX_OVLAN0_A is also hardcoded to slot 0 with ETH_P_8021AD as
the TPID.  How is a second concurrent flow rule requesting a different
outer TPID intended to be represented, given that installing it would
clobber slot 0?

> +			val = t4_read_reg(adap, ctl_reg);
> +			t4_write_reg(adap, ctl_reg, val | 1);

[High]
This RMW on MPS_PORT_RX_CTL_A is unserialized.  If another context
modifies other bits of the same register between the read and the
write, that update is lost.  Is there a lock that should be held here
(win0_lock or similar)?

> +			t4_tp_wr_bits_indirect(adap, TP_INGRESS_CONFIG_A, 1U << 9, 0);

[High, Medium, Medium]
t4_tp_wr_bits_indirect() drives the TP_PIO_ADDR_A / TP_PIO_DATA_A
indirect-register pair:

    void t4_tp_wr_bits_indirect(struct adapter *adap, unsigned int addr,
                                unsigned int mask, unsigned int val)
    {
        t4_write_reg(adap, TP_PIO_ADDR_A, addr);
        val |= t4_read_reg(adap, TP_PIO_DATA_A) & ~mask;
        t4_write_reg(adap, TP_PIO_DATA_A, val);
    }

Before this patch the only in-tree caller was probe-time
adap_init0_tweaks().  This patch adds a runtime, user-triggerable
caller.

Can a concurrent TP_PIO user (t4_tp_pio_read/t4_tp_pio_write when LDST
is unavailable, register-dump code, or another tc-flower install on a
different netdev of the same adapter) interleave between the ADDR
write and the DATA read/write and cause the wrong TP register to be
read or written?  What serializes the TP_PIO_ADDR_A/TP_PIO_DATA_A pair
here?

Separately, the hardware bit is toggled but adap->params.tp.ingress_config
is not updated.  That shadow is consulted throughout cxgb4_filter.c
(validate_filter, __cxgb4_set_filter, cxgb4_set_hash_filter) and l2t.c
to make VNIC_F / USE_ENC_IDX_F / ovlan-vs-pfvf decisions.  Should the
shadow be kept in sync with the hardware write?

Also, bit 9 of TP_INGRESS_CONFIG_A is written as the bare literal
1U << 9, while neighboring bits of the same register are exposed as
_S/_V/_F macros in t4_regs.h (CSUM_HAS_PSEUDO_HDR_S=10, VNIC_S=11,
USE_ENC_IDX_S=13).  What feature does bit 9 control, and could a
symbolic name be introduced?  The commit message enumerates three
actions (OVLAN register, RX control enable, filter fields) but does
not mention this fourth adapter-wide write.

> +		} else {
> +			fs->val.ivlan = vlan_tci;
> +			fs->mask.ivlan = vlan_tci_mask;
> +			fs->val.ivlan_vld = 1;
> +			fs->mask.ivlan_vld = 1;
> +
> +			/* Chelsio adapters use ivlan_vld bit to match vlan packets
> +			 * as 802.1Q. Also, when vlan tag is present in packets,
> +			 * ethtype match is used then to match on ethtype of inner
> +			 * header ie. the header following the vlan header.
> +			 * So, set the ivlan_vld based on ethtype info supplied by
> +			 * TC for vlan packets if its 802.1Q. And then reset the
> +			 * ethtype value else, hw will try to match the supplied
> +			 * ethtype value with ethtype of inner header.
> +			 */
> +
> +			if (fs->val.ethtype == ETH_P_8021Q) {
> +				fs->val.ethtype = 0;
> +				fs->mask.ethtype = 0;
> +			}

[High]
The preserved comment states that the hardware ethtype field matches
the ethtype after the outer VLAN tag (the inner header), and the
802.1Q branch resets fs->val.ethtype/fs->mask.ethtype to avoid a
mismatch.

The new 802.1ad branch above does not perform the equivalent reset.
For a common rule such as:

    tc filter add ... flower \
        vlan_ethtype 0x88a8 vlan_id X ...

FLOW_DISSECTOR_KEY_BASIC processing earlier in the same function has
already set fs->val.ethtype = 0x88a8, but the ethtype field is matched
against the inner header (which for real 802.1ad traffic is 0x8100 or
an L3 protocol, never 0x88a8).  Should the 802.1ad branch clear
fs->val.ethtype / fs->mask.ethtype the same way the 802.1Q branch
does?

>  		}
>  	}
>  
> diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h b/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h
> index 695916ba0405..38c585f3b1ad 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h
> +++ b/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h
> @@ -1921,6 +1921,9 @@
>  #define MAC_PORT_PTP_SUM_LO_A 0x990
>  #define MAC_PORT_PTP_SUM_HI_A 0x994
>  
> +#define MPS_PORT_RX_OVLAN0_A 0x120
> +#define MPS_PORT_RX_CTL_A    0X100

[Low]
This isn't a bug, but the hex prefix on 0X100 is uppercase while the
line above (and every neighboring macro) uses lowercase 0x.  Was that
intentional?

> +
>  #define MPS_CMN_CTL_A	0x9000
-- 
This is an AI-generated review.

Re: [PATCH net v2] cxgb4: flower: fix 802.1ad VLAN TPID matching in tc flower filters

[Harsha Mahadeva]

On Thursday, July 07/09/26, 2026 at 15:18:16 +0530, Paolo Abeni wrote:
> From: AI Reviewer <ai@example.com>
> 
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/
> ---
> cxgb4: flower: fix 802.1ad VLAN TPID matching in tc flower filters
> 
> This patch adds an 802.1ad branch to cxgb4_process_flow_match() that
> programs a port-specific OVLAN register with ETH_P_8021AD, sets bit 0
> of the RX control register, clears bit 9 of TP_INGRESS_CONFIG, and
> populates the filter's outer-VLAN match fields.
> 
> A few questions about the new code follow inline.

Hi Paolo,

Thank you for taking the time to review the patch. Please find my responses inline below.

> 
> > diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> > index 3307e5042681..8c5cfa6982e7 100644
> > --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> > +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> > @@ -266,24 +267,48 @@ static void cxgb4_process_flow_match(struct net_device *dev,
> >  					       VLAN_PRIO_SHIFT);
> >  		vlan_tci_mask = match.mask->vlan_id | (match.mask->vlan_priority <<
> >  						     VLAN_PRIO_SHIFT);
> > -		fs->val.ivlan = vlan_tci;
> > -		fs->mask.ivlan = vlan_tci_mask;
> > -
> > -		fs->val.ivlan_vld = 1;
> > -		fs->mask.ivlan_vld = 1;
> > -
> > -		/* Chelsio adapters use ivlan_vld bit to match vlan packets
> > [ ... ]
> > -		if (fs->val.ethtype == ETH_P_8021Q) {
> > -			fs->val.ethtype = 0;
> > -			fs->mask.ethtype = 0;
> > +
> > +		if (match.key->vlan_tpid == cpu_to_be16(ETH_P_8021AD)) {
> 
> [Medium]
> The gating condition consults only match.key->vlan_tpid and does not
> look at match.mask->vlan_tpid.  In tc-flower semantics a key value is
> only significant to the extent the mask covers it.
> 
> If a rule sets key=0x88a8 with mask=0 (don't care about TPID), this
> branch is still entered and the adapter registers are still
> reprogrammed.  Should this test be gated on the mask as well, similar
> to how mlx5 and nfp consult the mask before honoring the key?
> 
This programming is per port configuration rather than per-filter state. 
The TPID value is used to configure the port's OVLAN parsing behavior, 
and the corresponding hardware register uses a fixed mask of `0xffff` 
regardless of the mask supplied in the tc-flower rule. As a result, the intent 
here is to select the port's outer VLAN TPID based on the requested TPID value,
rather than to honor per-rule TPID masking semantics.
> > +			struct adapter *adap = netdev2adap(dev);
> > +			u32 ovlan_reg, ctl_reg, val, port_id;
> > +
> > +			if (!adap) {
> > +				netdev_err(dev, "%s: adap not found\n", __func__);
> > +				return;
> > +			}
> 
> [Low]
> netdev2adap() expands to netdev2pinfo(dev)->adapter and is not expected
> to return NULL for a netdev that tc-flower can bind to.  If the check
> ever did fire, cxgb4_process_flow_match() is void, so this early return
> also skips the trailing:
> 
>     fs->val.iport = netdev2pinfo(dev)->port_id;
>     fs->mask.iport = ~0;
> 
> and cxgb4_flow_rule_replace() would proceed to install a filter with
> iport=0 / iport_mask=0.  Should the check either be removed, or the
> function refactored to propagate an error?

Yes , `netdev2adap()` is not expected to return `NULL` for a valid cxgb4 netdev. 
I'll remove this check in the next revision
> 
> > +
> > +			val = (0xffff << 16) | ETH_P_8021AD;
> 
> [Low]
> The literal 0xffff has type int (signed), so 0xffff << 16 evaluates to
> a value that is not representable in a signed int.  Per C11 6.5.7 this
> is undefined behavior and UBSAN's shift sanitizer flags it.  Would
> 0xffffU << 16 (or a symbolic _V()/_M() macro) be preferable?
> 
> The layout of MPS_PORT_RX_OVLAN0_A ([31:16]=mask, [15:0]=value) is also
> not expressed through the driver's usual _S/_V/_F/_M helpers used by
> neighboring registers.  Could symbolic field macros be added?

I'll switch to using an unsigned value and add symbolic field macros in the next 
revision to match the existing register definition style.
> 
> > +			port_id = netdev2pinfo(dev)->port_id;
> > +			fs->val.ovlan = vlan_tci;
> > +			fs->mask.ovlan = vlan_tci_mask;
> > +			fs->val.ovlan_vld = 1;
> > +			fs->mask.ovlan_vld = 1;
> 
> [High]
> Does this ovlan programming survive on adapters that have VNIC_F set in
> the cached ingress_config?  In cxgb4_filter.c, both __cxgb4_set_filter()
> and cxgb4_set_hash_filter() unconditionally do:
> 
>     if (adap->params.tp.ingress_config & VNIC_F) {
>         fs->val.ovlan = (fs->val.pf << 13) | fs->val.vf;
>         fs->val.ovlan_vld = fs->val.pfvf_vld;
>         ...
>     }
> 
> which overwrites the fs->val.ovlan / fs->val.ovlan_vld set here.
> validate_filter() also rejects with -EOPNOTSUPP when ovlan_vld is set
> together with VNIC_F, or when the filter mode does not include
> VNIC_ID_F.
> 
> On adapters where VNIC_F is enabled, does this branch actually restore
> matching, or does it silently lose the VID (and, per the next comment,
> still mutate hardware before failing validation)?

I'll address this issue by moving the register configuration to happen 
after validate_filter() and install succeeds, rather than at cxgb4_process_flow_match() time 
as it does now. That way, if validate_filter() rejects the filter including the 
-EOPNOTSUPP case you mentioned,  the hardware registers are never touched 
in the first place, since we bail out before we get anywhere near programming them
> 
> > +			ovlan_reg = PORT_REG(port_id, MPS_PORT_RX_OVLAN0_A);
> > +			ctl_reg = PORT_REG(port_id, MPS_PORT_RX_CTL_A);
> > +			t4_write_reg(adap, ovlan_reg, val);
> 
> [High]
> cxgb4_process_flow_match() is called from cxgb4_flow_rule_replace()
> before __cxgb4_set_filter() runs validate_filter() and the mailbox/L2T
> install.  Do these three writes need to be deferred until after
> validation and successful install, so that a filter rejected by
> validate_filter(), or one that fails allocation/mailbox/L2T setup,
> does not leave the port and adapter in a reconfigured state with no
> installed filter to justify it?

will move the register writes to run after validate_filter() and successful install, so 
a filter that's rejected or fails allocation doesn't leave the port/adapter reconfigured 
with nothing installed to justify it
> 
> Additionally, clear_filter() (cxgb4_filter.c) only tears down L2T/SMT/
> CLIP and memsets the filter entry.  It does not undo MPS_PORT_RX_OVLAN0,
> MPS_PORT_RX_CTL bit 0, or TP_INGRESS_CONFIG bit 9.  Is a corresponding
> teardown expected on filter delete/replace so that 'tc filter add' +
> 'tc filter del' does not leave OVLAN matching permanently enabled and
> TP_INGRESS_CONFIG bit 9 permanently cleared?

Yes, this needs teardown handling. Since these are per-port registers rather than 
per-filter state, we can't simply undo them on every individual filter delete another 802.1ad 
filter on the same port may still depend on that configuration. I'll add a per-port reference count 
of installed 802.1ad/ovlan filters, incremented when one is installed and decremented in clear_filter() 
when one is removed, and only restore MPS_PORT_RX_OVLAN0, MPS_PORT_RX_CTL bit 0 and 
TP_INGRESS_CONFIG bit 9 to their original state once that count reaches zero
> 
> MPS_PORT_RX_OVLAN0_A is also hardcoded to slot 0 with ETH_P_8021AD as
> the TPID.  How is a second concurrent flow rule requesting a different
> outer TPID intended to be represented, given that installing it would
> clobber slot 0?

Currently we only support ETH_P_8021AD as the outer TPID there's no intent
right now to support multiple outer TPIDs concurrently on the same port, since 
only a single slot (MPS_PORT_RX_OVLAN0_A) is used. I'll add an explicit check 
so that if a second flow rule requests a different outer TPID while one is already 
active on the port, it's rejected  instead of silently clobbering slot 0
> 
> > +			val = t4_read_reg(adap, ctl_reg);
> > +			t4_write_reg(adap, ctl_reg, val | 1);
> 
> [High]
> This RMW on MPS_PORT_RX_CTL_A is unserialized.  If another context
> modifies other bits of the same register between the read and the
> write, that update is lost.  Is there a lock that should be held here
> (win0_lock or similar)?
> 
> > +			t4_tp_wr_bits_indirect(adap, TP_INGRESS_CONFIG_A, 1U << 9, 0);
> 
> [High, Medium, Medium]
> t4_tp_wr_bits_indirect() drives the TP_PIO_ADDR_A / TP_PIO_DATA_A
> indirect-register pair:
> 
>     void t4_tp_wr_bits_indirect(struct adapter *adap, unsigned int addr,
>                                 unsigned int mask, unsigned int val)
>     {
>         t4_write_reg(adap, TP_PIO_ADDR_A, addr);
>         val |= t4_read_reg(adap, TP_PIO_DATA_A) & ~mask;
>         t4_write_reg(adap, TP_PIO_DATA_A, val);
>     }
> 
> Before this patch the only in-tree caller was probe-time
> adap_init0_tweaks().  This patch adds a runtime, user-triggerable
> caller.
> 
> Can a concurrent TP_PIO user (t4_tp_pio_read/t4_tp_pio_write when LDST
> is unavailable, register-dump code, or another tc-flower install on a
> different netdev of the same adapter) interleave between the ADDR
> write and the DATA read/write and cause the wrong TP register to be
> read or written?  What serializes the TP_PIO_ADDR_A/TP_PIO_DATA_A pair
> here?

I will fix above in next revision .Will take locking around the MPS_PORT_RX_CTL_A RMW and 
TP_PIO_ADDR_A/TP_PIO_DATA_A sequence in t4_tp_wr_bits_indirect(), so both are 
serialized against concurrent access
> 
> Separately, the hardware bit is toggled but adap->params.tp.ingress_config
> is not updated.  That shadow is consulted throughout cxgb4_filter.c
> (validate_filter, __cxgb4_set_filter, cxgb4_set_hash_filter) and l2t.c
> to make VNIC_F / USE_ENC_IDX_F / ovlan-vs-pfvf decisions.  Should the
> shadow be kept in sync with the hardware write?
> 
> Also, bit 9 of TP_INGRESS_CONFIG_A is written as the bare literal
> 1U << 9, while neighboring bits of the same register are exposed as
> _S/_V/_F macros in t4_regs.h (CSUM_HAS_PSEUDO_HDR_S=10, VNIC_S=11,
> USE_ENC_IDX_S=13).  What feature does bit 9 control, and could a
> symbolic name be introduced?  The commit message enumerates three
> actions (OVLAN register, RX control enable, filter fields) but does
> not mention this fourth adapter-wide write.

Bit 9 is RM_OVLAN and when its unset, it stops hardware from stripping the outer 
VLAN tag. I will add a symbolic _S/_F macro for it (replacing the bare 1U << 9) and 
call out this fourth write in the commit message, and will also update 
adap->params.tp.ingress_config alongside the hardware write 
> 
> > +		} else {
> > +			fs->val.ivlan = vlan_tci;
> > +			fs->mask.ivlan = vlan_tci_mask;
> > +			fs->val.ivlan_vld = 1;
> > +			fs->mask.ivlan_vld = 1;
> > +
> > +			/* Chelsio adapters use ivlan_vld bit to match vlan packets
> > +			 * as 802.1Q. Also, when vlan tag is present in packets,
> > +			 * ethtype match is used then to match on ethtype of inner
> > +			 * header ie. the header following the vlan header.
> > +			 * So, set the ivlan_vld based on ethtype info supplied by
> > +			 * TC for vlan packets if its 802.1Q. And then reset the
> > +			 * ethtype value else, hw will try to match the supplied
> > +			 * ethtype value with ethtype of inner header.
> > +			 */
> > +
> > +			if (fs->val.ethtype == ETH_P_8021Q) {
> > +				fs->val.ethtype = 0;
> > +				fs->mask.ethtype = 0;
> > +			}
> 
> [High]
> The preserved comment states that the hardware ethtype field matches
> the ethtype after the outer VLAN tag (the inner header), and the
> 802.1Q branch resets fs->val.ethtype/fs->mask.ethtype to avoid a
> mismatch.
> 
> The new 802.1ad branch above does not perform the equivalent reset.
> For a common rule such as:
> 
>     tc filter add ... flower \
>         vlan_ethtype 0x88a8 vlan_id X ...
> 
> FLOW_DISSECTOR_KEY_BASIC processing earlier in the same function has
> already set fs->val.ethtype = 0x88a8, but the ethtype field is matched
> against the inner header (which for real 802.1ad traffic is 0x8100 or
> an L3 protocol, never 0x88a8).  Should the 802.1ad branch clear
> fs->val.ethtype / fs->mask.ethtype the same way the 802.1Q branch
> does?

The preserved comment applies to a single-tagged VLAN packet. I'll rework the logic 
to handle ethtype reset correctly for both the single-tagged and QinQ (802.1ad) cases
> 
> >  		}
> >  	}
> >  
> > diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h b/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h
> > index 695916ba0405..38c585f3b1ad 100644
> > --- a/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h
> > +++ b/drivers/net/ethernet/chelsio/cxgb4/t4_regs.h
> > @@ -1921,6 +1921,9 @@
> >  #define MAC_PORT_PTP_SUM_LO_A 0x990
> >  #define MAC_PORT_PTP_SUM_HI_A 0x994
> >  
> > +#define MPS_PORT_RX_OVLAN0_A 0x120
> > +#define MPS_PORT_RX_CTL_A    0X100
> 
> [Low]
> This isn't a bug, but the hex prefix on 0X100 is uppercase while the
> line above (and every neighboring macro) uses lowercase 0x.  Was that
> intentional?

Its not intentional ,I will fix this in next revision.Thanks for flagging it
> 
> > +
> >  #define MPS_CMN_CTL_A	0x9000
> -- 
> This is an AI-generated review.
>