[PATCH net v7 0/2] net: stmmac: remove software VLAN tag stripping

[PATCH net v7 0/2] net: stmmac: remove software VLAN tag stripping

[peilinhe2020]

From: Peilin He <he.peilin@zte.com.cn>

This series removes the driver-level software VLAN stripping in stmmac.

Patch 1 is a prerequisite selftests fix from Maxime Chevallier to keep
the ethtool selftests working on platforms where NETIF_F_VLAN_FEATURES
may be empty after patch 2.

Patch 2 removes stmmac_rx_vlan() and the related VLAN RX feature
advertisement.

Changes in v7:
- Send as a new series, not as a reply to v6.
- Include Maxime's selftests fix as patch 1/2.
- Keep the software VLAN stripping removal as patch 2/2.

Maxime Chevallier (1):
  net: stmmac: selftests: Prevent RXC from being disabled in LPI

Peilin He (1):
  net: stmmac: remove software VLAN tag stripping

 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 28 ++-----------------
 .../stmicro/stmmac/stmmac_selftests.c         |  2 ++
 2 files changed, 4 insertions(+), 26 deletions(-)

-- 
2.43.0

[PATCH net v7 1/2] net: stmmac: selftests: Prevent RXC from being disabled in LPI

[peilinhe2020]

From: Maxime Chevallier <maxime.chevallier@bootlin.com>

RXC is critical to the RX path and some register banks in stmmac, so
it is blocked from being disabled in LPI when we know we're going
to need that clock.

During selftests, we're in MAC loopback so the PHY may enter LPI and
stop its RXC. This will prevent the RX path from properly receiving
the looped-back packets, hence failing the selftests.

As the NETIF_F_VLAN_FEATURES may be empty on some platforms once the
software vlan stripping is removed, the phylink config attribute
'eee_rx_clk_stop_enable' may no longer be inconditionnally set to
true, hence allowing RXC to be disabled during LPI on platforms
that don't have any VLAN features.

Fixes: 091810dbded9 ("net: stmmac: Introduce selftests support")
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Peilin He <he.peilin@zte.com.cn>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
index 664d7cfde2f1..4f4d61a9c098 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
@@ -1847,12 +1847,14 @@ void stmmac_selftest_run(struct net_device *dev,
 		return;
 	}
 
+	phylink_rx_clk_stop_block(priv->phylink);
 	for (i = 0; i < count; i++) {
 		ret = stmmac_selftests[i].fn(priv);
 		if (ret && (ret != -EOPNOTSUPP))
 			etest->flags |= ETH_TEST_FL_FAILED;
 		buf[i] = ret;
 	}
+	phylink_rx_clk_stop_unblock(priv->phylink);
 
 	stmmac_set_mac_loopback(priv, priv->ioaddr, false);
 }
-- 
2.43.0

Re: [PATCH net v7 1/2] net: stmmac: selftests: Prevent RXC from being disabled in LPI

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The last paragraph of the commit message describes the
  `eee_rx_clk_stop_enable` precondition backwards.

--

net: stmmac: selftests: Prevent RXC from being disabled in LPI

The patch wraps the selftest loop in stmmac_selftest_run() with
phylink_rx_clk_stop_block() / phylink_rx_clk_stop_unblock() so the PHY
does not stop the receive clock while the MAC is in loopback. The code
change itself looks balanced and is a no-op when the phylink config does
not allow RX clock stop.

The question below is only about the changelog, so the diff hunk is not
quoted.

>     As the NETIF_F_VLAN_FEATURES may be empty on some platforms once the
>     software vlan stripping is removed, the phylink config attribute
>     'eee_rx_clk_stop_enable' may no longer be inconditionnally set to
>     true, hence allowing RXC to be disabled during LPI on platforms
>     that don't have any VLAN features.

[Severity: Medium]
Is the true/false direction here inverted?

include/linux/phylink.h documents the field as:

 * @eee_rx_clk_stop_enable: if true, PHY can stop the receive clock during LPI

So true is what permits the PHY to stop RXC, which makes "may no longer
be inconditionnally set to true, hence allowing RXC to be disabled"
read as its own opposite.

The code in stmmac_phylink_setup() also goes the other way, setting the
flag when the VLAN feature mask is empty:

	/* Disable EEE RX clock stop to ensure VLAN register access works
	 * correctly.
	 */
	if (!(priv->plat->flags & STMMAC_FLAG_RX_CLK_RUNS_IN_LPI) &&
	    !(priv->dev->features & NETIF_F_VLAN_FEATURES))
		config->eee_rx_clk_stop_enable = true;

In the current tree __stmmac_dvr_probe() sets, under
STMMAC_VLAN_TAG_USED:

	ndev->features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_STAG_RX;

so with CONFIG_VLAN_8021Q=y the flag is never true today, and it only
becomes true once the software VLAN stripping removal drops that
assignment. Would it be clearer to say that the flag starts being set to
true, thereby letting the PHY stop RXC?

Related to that, should the changelog also mention that
CONFIG_VLAN_8021Q=n builds are already affected today? With
STMMAC_VLAN_TAG_USED undefined in common.h, no VLAN features are ever
set, so eee_rx_clk_stop_enable is already true on platforms without
STMMAC_FLAG_RX_CLK_RUNS_IN_LPI and the selftests can already fail there.

As written it is hard to tell whether this fixes a live failure or is a
prerequisite for the following patch in the series, which also makes the
Fixes tag range hard to check.

There is also a spelling slip in the same paragraph: "inconditionnally"
should be "unconditionally".

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914150612.9798-1-peilinhe2020%40163.com

Re: [PATCH net v7 1/2] net: stmmac: selftests: Prevent RXC from being disabled in LPI

[Maxime Chevallier]

Hi

On 9/17/26 17:08, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> - [Medium] The last paragraph of the commit message describes the
>   `eee_rx_clk_stop_enable` precondition backwards.
> 
> --
> 
> net: stmmac: selftests: Prevent RXC from being disabled in LPI
> 
> The patch wraps the selftest loop in stmmac_selftest_run() with
> phylink_rx_clk_stop_block() / phylink_rx_clk_stop_unblock() so the PHY
> does not stop the receive clock while the MAC is in loopback. The code
> change itself looks balanced and is a no-op when the phylink config does
> not allow RX clock stop.
> 
> The question below is only about the changelog, so the diff hunk is not
> quoted.
> 
>>     As the NETIF_F_VLAN_FEATURES may be empty on some platforms once the
>>     software vlan stripping is removed, the phylink config attribute
>>     'eee_rx_clk_stop_enable' may no longer be inconditionnally set to
>>     true, hence allowing RXC to be disabled during LPI on platforms
>>     that don't have any VLAN features.
> 
> [Severity: Medium]
> Is the true/false direction here inverted?

Yeah should say "eee_rx_clk_stop_enable' may no longer be unconditionnally
set to false", my bad...

Code is correct though.
> 
> Related to that, should the changelog also mention that
> CONFIG_VLAN_8021Q=n builds are already affected today? 
I'm fine with it.

Peilin He, are you ok with changing the commit log if you send another
iteration ?

Thanks for you work and for including this patch :)

Maxime

[PATCH net v7 2/2] net: stmmac: remove software VLAN tag stripping

[peilinhe2020]

From: Peilin He <he.peilin@zte.com.cn>

The software VLAN stripping logic in stmmac_rx_vlan() was originally
introduced in 2014 by commit b93819854d6e ("stmmac: Add vlan rx for
better GRO performance.") as a workaround to improve GRO performance,
since at that time GRO could not handle frames with VLAN tags. However,
this limitation was resolved in 2015 by commit 66e5133f19e9 ("vlan: Add
GRO support for non hardware accelerated vlan"), which added GRO support
for non-hardware-accelerated VLAN frames. Keeping a software fallback
path for VLAN stripping is no longer necessary and only adds complexity.

Fixes: b93819854d6e ("stmmac: Add vlan rx for better GRO performance.")
Signed-off-by: Peilin He <he.peilin@zte.com.cn>
Maxime Chevallier <maxime.chevallier@bootlin.com>
Reviewed-by: xu xin <xu.xin16@zte.com.cn>
Reviewed-by: Jiang Kun <jiang.kun2@zte.com.cn>
---
Changes in v7:
- Resend as part of a series after adding Maxime's selftests fix as patch 1/2.
- No code change.

 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 28 ++-----------------
 1 file changed, 2 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 9c9e95d66b09..854b488611b4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5032,24 +5032,6 @@ static netdev_features_t stmmac_features_check(struct sk_buff *skb,
 	return vlan_features_check(skb, features);
 }
 
-static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb)
-{
-	struct vlan_ethhdr *veth = skb_vlan_eth_hdr(skb);
-	__be16 vlan_proto = veth->h_vlan_proto;
-	u16 vlanid;
-
-	if ((vlan_proto == htons(ETH_P_8021Q) &&
-	     dev->features & NETIF_F_HW_VLAN_CTAG_RX) ||
-	    (vlan_proto == htons(ETH_P_8021AD) &&
-	     dev->features & NETIF_F_HW_VLAN_STAG_RX)) {
-		/* pop the vlan tag */
-		vlanid = ntohs(veth->h_vlan_TCI);
-		memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2);
-		skb_pull(skb, VLAN_HLEN);
-		__vlan_hwaccel_put_tag(skb, vlan_proto, vlanid);
-	}
-}
-
 /**
  * stmmac_rx_refill - refill used skb preallocated buffers
  * @priv: driver private structure
@@ -5418,9 +5400,7 @@ static void stmmac_dispatch_skb_zc(struct stmmac_priv *priv, u32 queue,
 	if (priv->hw->hw_vlan_en)
 		/* MAC level stripping. */
 		stmmac_rx_hw_vlan(priv, priv->hw, p, skb);
-	else
-		/* Driver level stripping. */
-		stmmac_rx_vlan(priv->dev, skb);
+
 	skb->protocol = eth_type_trans(skb, priv->dev);
 
 	if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb))
@@ -5912,9 +5892,6 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 		if (priv->hw->hw_vlan_en)
 			/* MAC level stripping. */
 			stmmac_rx_hw_vlan(priv, priv->hw, p, skb);
-		else
-			/* Driver level stripping. */
-			stmmac_rx_vlan(priv->dev, skb);
 
 		skb->protocol = eth_type_trans(skb, priv->dev);
 
@@ -7995,9 +7972,8 @@ static int __stmmac_dvr_probe(struct device *device,
 	ndev->features |= ndev->hw_features | NETIF_F_HIGHDMA;
 	ndev->watchdog_timeo = msecs_to_jiffies(watchdog);
 #ifdef STMMAC_VLAN_TAG_USED
-	/* Both mac100 and gmac support receive VLAN tag detection */
-	ndev->features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_STAG_RX;
 	if (dwmac_is_xmac(priv->plat->core_type)) {
+		ndev->features |= NETIF_F_HW_VLAN_CTAG_RX;
 		ndev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX;
 		priv->hw->hw_vlan_en = true;
 	}
-- 
2.43.0

Re: [PATCH net v7 2/2] net: stmmac: remove software VLAN tag stripping

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 2 (1) · Low: 0

- [High] Loss of RX hardware checksum indication for every VLAN-tagged
  frame on cores without MAC-level stripping.
- [Medium] Undocumented user-visible netdev-feature change bundled with
  the cleanup.
- [Medium] Commit-message/tagging defects: a cleanup that removes
  advertised netdev features carries `Fixes: b93819854d6e ("stmmac: Add…

Pre-existing issues:
- [Medium] RX VLAN offload is advertised for DWXLGMAC, whose VLAN ops
  cannot perform it: `__stmmac_dvr_probe()` sets NETIF_F_HW_VLAN_CTAG_RX…

--

net: stmmac: remove software VLAN tag stripping

This removes stmmac_rx_vlan() and its two call sites in stmmac_rx() and
stmmac_dispatch_skb_zc(), on the grounds that GRO has handled non
hardware accelerated VLAN frames since 2015.  It also stops advertising
NETIF_F_HW_VLAN_CTAG_RX / NETIF_F_HW_VLAN_STAG_RX unconditionally and
instead sets only NETIF_F_HW_VLAN_CTAG_RX inside the dwmac_is_xmac()
branch in __stmmac_dvr_probe().

A couple of questions about the commit message first:

>     Fixes: b93819854d6e ("stmmac: Add vlan rx for better GRO performance.")
>     Signed-off-by: Peilin He <he.peilin@zte.com.cn>
>     Maxime Chevallier <maxime.chevallier@bootlin.com>

[Severity: Medium]
The body describes a cleanup ("no longer necessary and only adds
complexity") and no failure mode, yet the change carries a Fixes: tag and
also removes advertised netdev features.  Is the Fixes: tag intended here?
With it, stable and AUTOSEL tooling will pick up a user visible feature
removal (rx-vlan-offload turning off on dwmac100/dwmac1000,
rx-vlan-stag-hw-parse disappearing everywhere) into trees where nothing is
broken.

The dependency on the companion patch ("net: stmmac: selftests: Prevent RXC
from being disabled in LPI") is only mentioned in the changelog below the
--- separator, so it is not part of the permanent commit log.  Should that
dependency be recorded in the commit message so a lone backport of this
patch does not regress the selftests?

Also, the line

    Maxime Chevallier <maxime.chevallier@bootlin.com>

has no trailer prefix (Reviewed-by/Suggested-by/Co-developed-by), so the
attribution is ambiguous and checkpatch will complain about it.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 62c3441911e71..0d097094fa453 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5049,24 +5049,6 @@ static netdev_features_t stmmac_features_check(struct sk_buff *skb,
>  	return vlan_features_check(skb, features);
>  }
>  
> -static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb)
> -{
> -	struct vlan_ethhdr *veth = skb_vlan_eth_hdr(skb);
> -	__be16 vlan_proto = veth->h_vlan_proto;
> -	u16 vlanid;
> -
> -	if ((vlan_proto == htons(ETH_P_8021Q) &&
> -	     dev->features & NETIF_F_HW_VLAN_CTAG_RX) ||
> -	    (vlan_proto == htons(ETH_P_8021AD) &&
> -	     dev->features & NETIF_F_HW_VLAN_STAG_RX)) {
> -		/* pop the vlan tag */
> -		vlanid = ntohs(veth->h_vlan_TCI);
> -		memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2);
> -		skb_pull(skb, VLAN_HLEN);
> -		__vlan_hwaccel_put_tag(skb, vlan_proto, vlanid);
> -	}
> -}
> -
>  /**
>   * stmmac_rx_refill - refill used skb preallocated buffers
>   * @priv: driver private structure

[ ... ]

> @@ -5435,9 +5417,7 @@ static void stmmac_dispatch_skb_zc(struct stmmac_priv *priv, u32 queue,
>  	if (priv->hw->hw_vlan_en)
>  		/* MAC level stripping. */
>  		stmmac_rx_hw_vlan(priv, priv->hw, p, skb);
> -	else
> -		/* Driver level stripping. */
> -		stmmac_rx_vlan(priv->dev, skb);
> +
>  	skb->protocol = eth_type_trans(skb, priv->dev);
>  
>  	if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb))

[Severity: High]
Does this drop RX checksum offload for every VLAN tagged frame when
priv->hw->hw_vlan_en is false?

With the else branch gone, the 802.1Q/802.1AD header stays in the payload,
and the checksum decision right below is gated on
stmmac_has_ip_ethertype():

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_has_ip_ethertype() {
	proto = __vlan_get_protocol(skb, eth_header_parse_protocol(skb),
				    &depth);

	return (depth <= ETH_HLEN) &&
		(proto == htons(ETH_P_IP) || proto == htons(ETH_P_IPV6));
}

For a freshly built RX skb, napi_build_skb() leaves skb->mac_len as 0 and
eth_type_trans() does not set it, so __vlan_get_protocol_offset() starts
parsing at ETH_HLEN:

net/core/skbuff.c:__vlan_get_protocol_offset() {
	unsigned int vlan_depth = skb->mac_len, parse_depth = VLAN_MAX_DEPTH;
	...
	} else {
		vlan_depth = ETH_HLEN;
	}

A single tagged frame then yields depth == ETH_HLEN + VLAN_HLEN == 18, so
the depth <= ETH_HLEN test fails and stmmac_has_ip_ethertype() returns
false for all still-tagged frames.  Before this change stmmac_rx_vlan()
had already popped the tag, so depth stayed at 0 and the hardware verdict
was honoured.

In this zero copy path there is no status & csum_none term at all, so the
result is skb_checksum_none_assert() for every tagged frame even though
the COE result is available.

Should stmmac_has_ip_ethertype() be taught to tolerate an in-payload VLAN
tag (or be evaluated with mac_len set) as part of this change?

> @@ -5929,9 +5909,6 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  		if (priv->hw->hw_vlan_en)
>  			/* MAC level stripping. */
>  			stmmac_rx_hw_vlan(priv, priv->hw, p, skb);
> -		else
> -			/* Driver level stripping. */
> -			stmmac_rx_vlan(priv->dev, skb);
>  
>  		skb->protocol = eth_type_trans(skb, priv->dev);
>  

[Severity: High]
Same question for the main receive path.  stmmac_rx() has:

	if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb) ||
	    (status & csum_none))
		skb_checksum_none_assert(skb);
	else
		skb->ip_summed = CHECKSUM_UNNECESSARY;

Since stmmac_has_ip_ethertype() now sees the VLAN ethertype and a depth of
18, does every tagged IPv4/IPv6 frame fall back to software checksum
validation on cores where hw_vlan_en is false, i.e. dwmac100/dwmac1000
after the probe hunk below, and any xmac core with ethtool -K rxvlan off?
dwmac1000 does report rx_coe_type1/type2, so the hardware result exists.

> @@ -7990,9 +7967,8 @@ static int __stmmac_dvr_probe(struct device *device,
>  	ndev->features |= ndev->hw_features | NETIF_F_HIGHDMA;
>  	ndev->watchdog_timeo = msecs_to_jiffies(watchdog);
>  #ifdef STMMAC_VLAN_TAG_USED
> -	/* Both mac100 and gmac support receive VLAN tag detection */
> -	ndev->features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_STAG_RX;
>  	if (dwmac_is_xmac(priv->plat->core_type)) {
> +		ndev->features |= NETIF_F_HW_VLAN_CTAG_RX;
>  		ndev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX;
>  		priv->hw->hw_vlan_en = true;
>  	}

[Severity: Medium]
This hunk changes what ethtool -k reports, but the commit message only
talks about deleting the software stripping helper.  Could the changelog
cover the feature changes too?

On dwmac100/dwmac1000, rx-vlan-offload goes from on [fixed] to off, and
there is no hardware replacement: the MAC100 and GMAC entries in
stmmac_hw[] in hwif.c have no .vlan member at all.

NETIF_F_HW_VLAN_STAG_RX is now gone on every core, including xmac cores
where the MAC still strips S-tags and reports them as C-tags:

drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c:vlan_set_hw_mode() {
		value |= VLAN_TAG_STRIP_ALL;

drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c:vlan_rx_hw() {
		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vid);

Is the advertisement now inconsistent with what the hardware does there?

There is also a side effect on platforms that end up with no
NETIF_F_VLAN_FEATURES bit in priv->dev->features:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_phylink_setup() {
	if (!(priv->plat->flags & STMMAC_FLAG_RX_CLK_RUNS_IN_LPI) &&
	    !(priv->dev->features & NETIF_F_VLAN_FEATURES))
		config->eee_rx_clk_stop_enable = true;

so EEE RX clock stop becomes permitted where it previously was not.  That
consequence is acknowledged only in the companion selftests patch; should
it be described here as well?

[Severity: Medium]
This isn't a bug introduced by this patch, but since the block is being
rewritten here it may be worth revisiting: the gate is the core family
rather than the presence of the VLAN callbacks.  dwmac_is_xmac() returns
true for DWMAC_CORE_XGMAC, and the DWXLGMAC_ID entry in stmmac_hw[] in
hwif.c binds dwxlgmac2_vlan_ops:

drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c {
	const struct stmmac_vlan_ops dwxlgmac2_vlan_ops = {
		.update_vlan_hash = dwxgmac2_update_vlan_hash,
		.enable_vlan = vlan_enable,
	};

With neither .set_hw_vlan_mode nor .rx_hw_vlan, both
stmmac_set_hw_vlan_mode() and stmmac_rx_hw_vlan() are
stmmac_do_void_callback() no-ops, so EVLS/EVLRXS is never programmed and
no tag is ever put in skb->vlan_tci, while ethtool still shows
rx-vlan-offload as on.  Would gating on the presence of .rx_hw_vlan and
.set_hw_vlan_mode be more accurate than dwmac_is_xmac()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914150612.9798-1-peilinhe2020%40163.com