[PATCH net v4 0/7] net: stmmac: More selftest-related fixes

[PATCH net v4 0/7] net: stmmac: More selftest-related fixes

[Maxime Chevallier]

Hi everyone,

This is V4 of stmmac selftest fixes, addressing Sashiko's issues over
the MTU patch. This lead to the introduction of a new one. The
dev_add_pack races have been addressed, however the double-vlan issue
stayed there. Ovidiu is actively working on it, let's wait for his work
to land before fixing that.

Nicolai, I took the liberty of keeping your review tags on affected
patches, let me know if this isn't ok.

This is another round of stmmac selftest fixes, mostly about the selftests
themselves but a few things were discovered w.r.t MTU and buffer size
handling, see patch 5 anf 6.

After this is merged, I consider the selftests to be now reliable enough
to run them nightly on every stmmac series that's sent, and I'll be requiring
clean selftests for new glue drivers.

Since V3, the testing farm grew ! I've been running this on :

 - Altera CycloneV (dwmac-socfpga, dwmac1000 IP, v3.70a)
 - NXP imx8mp (dwmac-imx, dwmac4, v5.10a)
 - Allwinner H2S (dwmac-sun8i, dwmac1000)
 - Amlogic S905X3 (dwmac-meson8b, dwmac1000, v3.70a)
 - STM32mp157a (dwmac-stm32, dwmac4, v4.20a)
 - SiFive JH7110 (dwmac-starfive, dwmac4, v5.20)
 - Motorcomm YT8061 (PCIe, dwmac-motorcomm, dwmac4)
 - Qualcomm IPQ8064 (dwmac-ipq806x, dwmac1000)
 - Altera AgileX5 (dwmac-socfpga, dwxgmac2 !) (NEW)
 - Generic dwmac1000 (Loongson 2K0300, dwmac 3.70a) (NEW)
 - Rockchip RK3566 (dwmac-rk, dwmac4) (NEW)

It's becoming cumbersome to list the test results here, they can be
found, updated daily, here :

https://minimaxwell.github.io/stmmac-ci/

Thanks,

Maxime

V4:
 - Move the dev_add_pack later, to avoid potential races (sashiko)
 - Add one more MTU fix for 8K buffer selection
 - Ditch an extra SKB padding in patch 5.

V3:
https://lore.kernel.org/netdev/20260911212028.1497613-1-maxime.chevallier@bootlin.com/
 - Bail out of the EEE test if LPI is over 5 seconds

V2:
https://lore.kernel.org/r/20260910151404.976753-1-maxime.chevallier@bootlin.com
 - Snapshot before sending the frame for EEE, in case we immediately
   enter LPI
 - Also return -ETIMEDOUT instead of EINVAL

V1: https://lore.kernel.org/lkml/20260909074740.1247762-1-maxime.chevallier@bootlin.com/

Maxime Chevallier (7):
  net: stmmac: selftests: Support running selftests on DSA conduits
  net: stmmac: selftests: Validate EEE based on the actual LPI timer
    value
  net: stmmac: selftests: Check the dev->features for S-TAG offload
    testing
  net: stmmac: selftests: Capture all packets for vlan checks
  net: stmmac: dwmac4: Use the correct bufzise when the len is exactly
    8K
  net: stmmac: size the RX buffers from the frame length, not the MTU
  net: stmmac: selftests: Account for alignment shift on dwmac1000 for
    Jumbo test

 .../ethernet/stmicro/stmmac/dwmac4_descs.c    |   4 +-
 drivers/net/ethernet/stmicro/stmmac/hwif.h    |   2 +-
 .../net/ethernet/stmicro/stmmac/ring_mode.c   |   4 +-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  20 +--
 .../stmicro/stmmac/stmmac_selftests.c         | 153 ++++++++++++++----
 5 files changed, 139 insertions(+), 44 deletions(-)

-- 
2.55.0

[PATCH net v4 1/7] net: stmmac: selftests: Support running selftests on DSA conduits

[Maxime Chevallier]

Most stmmac selftests rely on dev_add_pack() to add custom handlers,
that validate the packets sent to ourselves through MAC loopback.

However, when the stmmac-driven interface is a DSA CPU conduit, all
frames that are received have ETH_P_XDSA as a protocol, even though they
don't actually contain any tag as they come from the loopback and not
the switch.

This will prevent any incoming packet to match our packet handlers.

Let's register a ETH_P_ALL packet handler when we detect that we're a
DSA conduit, and use a proxy packet handler to filter the h_proto.

As this allows external frames to be received through our .func(), the
packet handler is added after the dev->addr field is populated in our
selftest attributes.

Note that we may still receive incoming packets from the switch, but
these frames shouldn't interfere with the very specific frames used for
selftests, and stmmac selftests in general aren't safe against external
traffic interferences.

This was validated on a WPQ864 devkit for IPQ8064, that has the SoC
connected to a QCA8k switch.

The ARP offload's packet handler is left alone, this feature is just not
implemented in stmmac and due for removal.

Fixes: 091810dbded9 ("net: stmmac: Introduce selftests support")
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 .../stmicro/stmmac/stmmac_selftests.c         | 89 +++++++++++++++----
 1 file changed, 71 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
index 6372ec7c3f31..614b5995dec5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
@@ -12,6 +12,7 @@
 #include <linux/ethtool.h>
 #include <linux/ip.h>
 #include <linux/udp.h>
+#include <net/dsa.h>
 #include <net/pkt_cls.h>
 #include <net/pkt_sched.h>
 #include <net/tcp.h>
@@ -237,6 +238,9 @@ struct stmmac_test_priv {
 	struct stmmac_packet_attrs *packet;
 	struct packet_type pt;
 	struct completion comp;
+	__be16 packet_type;
+	int (*func)(struct sk_buff *skb, struct net_device *ndev,
+		    struct packet_type *pt, struct net_device *orig_ndev);
 	int double_vlan;
 	int vlan_id;
 	int ok;
@@ -316,6 +320,50 @@ static int stmmac_test_loopback_validate(struct sk_buff *skb,
 	return 0;
 }
 
+static int stmmac_sft_filter(struct sk_buff *skb, struct net_device *ndev,
+			     struct packet_type *pt,
+			     struct net_device *orig_ndev)
+{
+	struct stmmac_test_priv *tpriv = pt->af_packet_priv;
+	struct ethhdr *hdr = eth_hdr(skb);
+	int ret = 0;
+
+	if (hdr->h_proto == tpriv->packet_type) {
+		struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
+
+		if (nskb)
+			ret = tpriv->func(nskb, ndev, pt, orig_ndev);
+	}
+
+	kfree_skb(skb);
+	return ret;
+}
+
+static void stmmac_sft_add_pack(struct packet_type *pt)
+{
+	struct stmmac_test_priv *tpriv = pt->af_packet_priv;
+
+	if (netdev_uses_dsa(tpriv->pt.dev)) {
+		tpriv->packet_type = tpriv->pt.type;
+		tpriv->func = tpriv->pt.func;
+
+		/* DSA conduit will report ETH_P_XDSA, so our packet handler
+		 * won't match. Let's register a ETH_P_ALL match and filter
+		 * manually in stmmac_sft_filter.
+		 */
+		tpriv->pt.type = htons(ETH_P_ALL);
+		tpriv->pt.func = stmmac_sft_filter;
+		tpriv->pt.ignore_outgoing = true;
+	}
+
+	dev_add_pack(pt);
+}
+
+static void stmmac_sft_remove_pack(struct packet_type *pt)
+{
+	dev_remove_pack(pt);
+}
+
 static int __stmmac_test_loopback(struct stmmac_priv *priv,
 				  struct stmmac_packet_attrs *attr)
 {
@@ -337,7 +385,7 @@ static int __stmmac_test_loopback(struct stmmac_priv *priv,
 	tpriv->packet = attr;
 
 	if (!attr->dont_wait)
-		dev_add_pack(&tpriv->pt);
+		stmmac_sft_add_pack(&tpriv->pt);
 
 	skb = stmmac_test_get_udp_skb(priv, attr);
 	if (!skb) {
@@ -360,7 +408,7 @@ static int __stmmac_test_loopback(struct stmmac_priv *priv,
 
 cleanup:
 	if (!attr->dont_wait)
-		dev_remove_pack(&tpriv->pt);
+		stmmac_sft_remove_pack(&tpriv->pt);
 	kfree(tpriv);
 	return ret;
 }
@@ -767,7 +815,7 @@ static int stmmac_test_flowctrl(struct stmmac_priv *priv)
 	tpriv->pt.func = stmmac_test_flowctrl_validate;
 	tpriv->pt.dev = priv->dev;
 	tpriv->pt.af_packet_priv = tpriv;
-	dev_add_pack(&tpriv->pt);
+	stmmac_sft_add_pack(&tpriv->pt);
 
 	/* Compute minimum number of packets to make FIFO full */
 	pkt_count = rx_fifo_size;
@@ -823,7 +871,7 @@ static int stmmac_test_flowctrl(struct stmmac_priv *priv)
 cleanup:
 	dev_mc_del(priv->dev, paddr);
 	dev_set_promiscuity(priv->dev, -1);
-	dev_remove_pack(&tpriv->pt);
+	stmmac_sft_remove_pack(&tpriv->pt);
 	kfree(tpriv);
 	return ret;
 }
@@ -928,18 +976,20 @@ static int __stmmac_test_vlanfilt(struct stmmac_priv *priv)
 	 * HASH values.
 	 */
 	tpriv->vlan_id = 0x123;
-	dev_add_pack(&tpriv->pt);
 
 	ret = vlan_vid_add(priv->dev, htons(ETH_P_8021Q), tpriv->vlan_id);
 	if (ret)
 		goto cleanup;
 
+	attr.vlan = 1;
+	attr.dst = priv->dev->dev_addr;
+	attr.sport = 9;
+	attr.dport = 9;
+
+	stmmac_sft_add_pack(&tpriv->pt);
+
 	for (i = 0; i < 4; i++) {
-		attr.vlan = 1;
 		attr.vlan_id_out = tpriv->vlan_id + i;
-		attr.dst = priv->dev->dev_addr;
-		attr.sport = 9;
-		attr.dport = 9;
 
 		skb = stmmac_test_get_udp_skb(priv, &attr);
 		if (!skb) {
@@ -966,9 +1016,9 @@ static int __stmmac_test_vlanfilt(struct stmmac_priv *priv)
 	}
 
 vlan_del:
+	stmmac_sft_remove_pack(&tpriv->pt);
 	vlan_vid_del(priv->dev, htons(ETH_P_8021Q), tpriv->vlan_id);
 cleanup:
-	dev_remove_pack(&tpriv->pt);
 	kfree(tpriv);
 	return ret;
 }
@@ -1022,18 +1072,20 @@ static int __stmmac_test_dvlanfilt(struct stmmac_priv *priv)
 	 * HASH values.
 	 */
 	tpriv->vlan_id = 0x123;
-	dev_add_pack(&tpriv->pt);
 
 	ret = vlan_vid_add(priv->dev, htons(ETH_P_8021AD), tpriv->vlan_id);
 	if (ret)
 		goto cleanup;
 
+	attr.vlan = 2;
+	attr.dst = priv->dev->dev_addr;
+	attr.sport = 9;
+	attr.dport = 9;
+
+	stmmac_sft_add_pack(&tpriv->pt);
+
 	for (i = 0; i < 4; i++) {
-		attr.vlan = 2;
 		attr.vlan_id_out = tpriv->vlan_id + i;
-		attr.dst = priv->dev->dev_addr;
-		attr.sport = 9;
-		attr.dport = 9;
 
 		skb = stmmac_test_get_udp_skb(priv, &attr);
 		if (!skb) {
@@ -1060,9 +1112,9 @@ static int __stmmac_test_dvlanfilt(struct stmmac_priv *priv)
 	}
 
 vlan_del:
+	stmmac_sft_remove_pack(&tpriv->pt);
 	vlan_vid_del(priv->dev, htons(ETH_P_8021AD), tpriv->vlan_id);
 cleanup:
-	dev_remove_pack(&tpriv->pt);
 	kfree(tpriv);
 	return ret;
 }
@@ -1293,7 +1345,6 @@ static int stmmac_test_vlanoff_common(struct stmmac_priv *priv, bool svlan)
 	tpriv->pt.af_packet_priv = tpriv;
 	tpriv->packet = &attr;
 	tpriv->vlan_id = 0x123;
-	dev_add_pack(&tpriv->pt);
 
 	ret = vlan_vid_add(priv->dev, htons(proto), tpriv->vlan_id);
 	if (ret)
@@ -1301,6 +1352,8 @@ static int stmmac_test_vlanoff_common(struct stmmac_priv *priv, bool svlan)
 
 	attr.dst = priv->dev->dev_addr;
 
+	stmmac_sft_add_pack(&tpriv->pt);
+
 	skb = stmmac_test_get_udp_skb(priv, &attr);
 	if (!skb) {
 		ret = -ENOMEM;
@@ -1318,9 +1371,9 @@ static int stmmac_test_vlanoff_common(struct stmmac_priv *priv, bool svlan)
 	ret = tpriv->ok ? 0 : -ETIMEDOUT;
 
 vlan_del:
+	stmmac_sft_remove_pack(&tpriv->pt);
 	vlan_vid_del(priv->dev, htons(proto), tpriv->vlan_id);
 cleanup:
-	dev_remove_pack(&tpriv->pt);
 	kfree(tpriv);
 	return ret;
 }
-- 
2.55.0

[PATCH net v4 2/7] net: stmmac: selftests: Validate EEE based on the actual LPI timer value

[Maxime Chevallier]

The EEE selftest is a 2-step test :
 - It validates that we enter in LPI mode with the
   irq_tx_path_in_lpi_mode_n counter
 - It then validates that we exit LPI when sending a frame, with the
   irq_tx_path_exit_lpi_mode_n counter.

The current state of the test lacks 2 main things :

 - We don't know exactly when was the previous frame sent (it's from the
   previous selftest)

 - The timeout is hardcoded, while the LPI is entered after a
   user-configurable delay. On top of that, the timeout loop uses a
   pre-decrement iterator (--retries) that actually only iterate nine
   times, so 900ms while the default LPI value is 1 second.

Let's therefore make it more deterministic :

 - Send a frame at the beginning of the test
 - Wait for more than the lpi timer value, we timeout after about twice
   the value,
 - Then send another frame, and verify that we do go out of LPI, also
   with a timeout.

As LPI timer can get pretty high, bail out if LPI timer is over 5
seconds.

Note that the test's goal isn't to validate the LPI timer value itself,
only that we enter/leave LPI mode.

Fixes: 091810dbded9 ("net: stmmac: Introduce selftests support")
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 .../stmicro/stmmac/stmmac_selftests.c         | 44 ++++++++++++++++---
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
index 614b5995dec5..2f9f7746c40a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
@@ -30,6 +30,7 @@ struct stmmachdr {
 			      sizeof(struct stmmachdr))
 #define STMMAC_TEST_PKT_MAGIC	0xdeadcafecafedeadULL
 #define STMMAC_LB_TIMEOUT	msecs_to_jiffies(200)
+#define STMMAC_SFT_MAX_LPI	(5 * USEC_PER_SEC)
 
 struct stmmac_packet_attrs {
 	int vlan;
@@ -462,12 +463,16 @@ static int stmmac_test_mmc(struct stmmac_priv *priv)
 static int stmmac_test_eee(struct stmmac_priv *priv)
 {
 	struct stmmac_extra_stats *initial, *final;
-	int retries = 10;
+	unsigned long timeout, max_duration;
 	int ret;
 
 	if (!priv->dma_cap.eee || !priv->eee_active)
 		return -EOPNOTSUPP;
 
+	/* Bail out if the configured LPI timer is too long */
+	if (priv->tx_lpi_timer > STMMAC_SFT_MAX_LPI)
+		return -EOPNOTSUPP;
+
 	initial = kzalloc_obj(*initial);
 	if (!initial)
 		return -ENOMEM;
@@ -478,14 +483,21 @@ static int stmmac_test_eee(struct stmmac_priv *priv)
 		goto out_free_initial;
 	}
 
+	/* Snapshot stats, we want to count the in_lpi events. We may enter
+	 * LPI just after the packet was sent.
+	 */
 	memcpy(initial, &priv->xstats, sizeof(*initial));
 
+	/* Send a frame, then wait to enter LPI */
 	ret = stmmac_test_mac_loopback(priv);
 	if (ret)
 		goto out_free_final;
 
+	max_duration = usecs_to_jiffies(2 * priv->tx_lpi_timer);
+
 	/* We have no traffic in the line so, sooner or later it will go LPI */
-	while (--retries) {
+	timeout = jiffies + max_duration;
+	while (!time_after(jiffies, timeout)) {
 		memcpy(final, &priv->xstats, sizeof(*final));
 
 		if (final->irq_tx_path_in_lpi_mode_n >
@@ -494,20 +506,38 @@ static int stmmac_test_eee(struct stmmac_priv *priv)
 		msleep(100);
 	}
 
-	if (!retries) {
+	memcpy(final, &priv->xstats, sizeof(*final));
+	if (final->irq_tx_path_in_lpi_mode_n <=
+	    initial->irq_tx_path_in_lpi_mode_n) {
 		ret = -ETIMEDOUT;
 		goto out_free_final;
 	}
 
-	if (final->irq_tx_path_in_lpi_mode_n <=
-	    initial->irq_tx_path_in_lpi_mode_n) {
-		ret = -EINVAL;
+	/* Re-snapshot, as we want to measure exit_lpi events. We should be
+	 * in LPI right now.
+	 */
+	memcpy(initial, &priv->xstats, sizeof(*initial));
+
+	/* TX something so we go out of LPI */
+	ret = stmmac_test_mac_loopback(priv);
+	if (ret)
 		goto out_free_final;
+
+	/* Wait for the exit LPI interrupt */
+	timeout = jiffies + max_duration;
+	while (!time_after(jiffies, timeout)) {
+		memcpy(final, &priv->xstats, sizeof(*final));
+
+		if (final->irq_tx_path_exit_lpi_mode_n >
+		    initial->irq_tx_path_exit_lpi_mode_n)
+			break;
+		msleep(100);
 	}
 
+	memcpy(final, &priv->xstats, sizeof(*final));
 	if (final->irq_tx_path_exit_lpi_mode_n <=
 	    initial->irq_tx_path_exit_lpi_mode_n) {
-		ret = -EINVAL;
+		ret = -ETIMEDOUT;
 		goto out_free_final;
 	}
 
-- 
2.55.0

Re: [PATCH net v4 2/7] net: stmmac: selftests: Validate EEE based on the actual LPI timer value

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 0

- [Medium] In stmmac_test_eee() the deadline of both new polling loops is
  derived exclusively from priv->tx_lpi_timer (max_duration =…
- [Medium] stmmac_test_eee() infers the *current* LPI state from
  monotonic event counters instead of the actual MAC state.
- [Medium] stmmac_test_eee() validates priv->tx_lpi_timer against
  STMMAC_SFT_MAX_LPI (5 s) at stmmac_selftests.c:473 but never caches…

--

net: stmmac: selftests: Validate EEE based on the actual LPI timer value

The EEE selftest now sends a frame, waits up to roughly twice the configured
LPI timer for the enter-LPI interrupt counter to increase, then sends a
second frame and waits the same amount of time for the exit-LPI counter.
It also bails out with -EOPNOTSUPP when the configured LPI timer exceeds
five seconds.

A few questions below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> index 614b5995dec5f..2f9f7746c40ae 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> @@ -30,6 +30,7 @@ struct stmmachdr {
>  			      sizeof(struct stmmachdr))
>  #define STMMAC_TEST_PKT_MAGIC	0xdeadcafecafedeadULL
>  #define STMMAC_LB_TIMEOUT	msecs_to_jiffies(200)
> +#define STMMAC_SFT_MAX_LPI	(5 * USEC_PER_SEC)
>  
>  struct stmmac_packet_attrs {
>  	int vlan;
> @@ -462,12 +463,16 @@ static int stmmac_test_mmc(struct stmmac_priv *priv)
>  static int stmmac_test_eee(struct stmmac_priv *priv)
>  {
>  	struct stmmac_extra_stats *initial, *final;
> -	int retries = 10;
> +	unsigned long timeout, max_duration;
>  	int ret;
>  
>  	if (!priv->dma_cap.eee || !priv->eee_active)
>  		return -EOPNOTSUPP;
>  
> +	/* Bail out if the configured LPI timer is too long */
> +	if (priv->tx_lpi_timer > STMMAC_SFT_MAX_LPI)
> +		return -EOPNOTSUPP;
> +

[Severity: Medium]
Is it intentional that priv->tx_lpi_timer is read twice here, once for the
bound check and again later for max_duration, without caching the checked
value?

Between the two reads stmmac_test_eee() allocates two stats buffers and runs
a full loopback transmit/receive, which can take up to STMMAC_LB_TIMEOUT
(200 ms).

priv->tx_lpi_timer is written outside the selftest by
stmmac_mac_enable_tx_lpi() in stmmac_main.c:

	priv->tx_lpi_timer = timer;
	priv->eee_active = true;

That path is reached from phylink_link_up()->phylink_activate_lpi() inside
the phylink_resolve() work item, which takes only pl->phydev_mutex,
phy->lock and pl->state_mutex, not rtnl. The selftest itself runs with
rtnl_lock held from __dev_ethtool(), so the resolve work can update
priv->tx_lpi_timer in the middle of the test.

Given that neither ethnl_set_eee() nor phy_ethtool_set_eee() range-check
ETHTOOL_A_EEE_TX_LPI_TIMER, can the value used for max_duration therefore be
one that was never validated against STMMAC_SFT_MAX_LPI? For example, a
timer of 60000000 us gives 120 s per wait loop, and 4000000000 us wraps in
the u32 multiplication below to 3705032704 us, i.e. around 3705 s per loop,
all spent in msleep(100) with rtnl_lock held.

Would reading priv->tx_lpi_timer once into a local u32 and validating and
using that single copy (with 64-bit or otherwise bounded arithmetic for the
doubling) address this?

>  	initial = kzalloc_obj(*initial);
>  	if (!initial)
>  		return -ENOMEM;
> @@ -478,14 +483,21 @@ static int stmmac_test_eee(struct stmmac_priv *priv)
>  		goto out_free_initial;
>  	}
>  
> +	/* Snapshot stats, we want to count the in_lpi events. We may enter
> +	 * LPI just after the packet was sent.
> +	 */
>  	memcpy(initial, &priv->xstats, sizeof(*initial));
>  
> +	/* Send a frame, then wait to enter LPI */
>  	ret = stmmac_test_mac_loopback(priv);
>  	if (ret)
>  		goto out_free_final;
>  
> +	max_duration = usecs_to_jiffies(2 * priv->tx_lpi_timer);
> +

[Severity: Medium]
Does this deadline hold for small LPI timer values?

The patch adds an upper bail-out for priv->tx_lpi_timer but no lower bound,
and the value is user-configurable via ethtool --set-eee tx-lpi-timer, and
can also be 0 via the eee_timer module parameter used in stmmac_open().

For a timer of 0 us, _usecs_to_jiffies(0) is 0, so "timeout = jiffies" is
already at the boundary and the loop body may run zero or one time. For any
timer below ~200 ms the budget is smaller than the loop's own msleep(100)
granularity, so the loop degenerates to a single sample.

Independently of the granularity, stmmac_mac_enable_tx_lpi() programs the
link-status gate on every EEE enable:

	stmmac_set_eee_timer(priv, priv->hw, STMMAC_DEFAULT_LIT_LS,
			     STMMAC_DEFAULT_TWT_LS);

with STMMAC_DEFAULT_LIT_LS defined as 0x3E8 in common.h, and
dwmac4_set_eee_timer() documents it as:

	/* LS: minimum time (ms) for which the link
	 *  status from PHY should be ok before transmitting
	 *  the LPI pattern. */

So 1000 ms of stable link is required before LPI may be transmitted at all.
With a configured LPI timer below roughly 400 ms and a recent link-up (the
usual bring-link-up-then-run-ethtool-t sequence), does the enter-LPI
deadline expire before the hardware is even permitted to enter LPI, making
stmmac_test_eee() return -ETIMEDOUT and stmmac_selftest_run() set
ETH_TEST_FL_FAILED on working hardware? The previous fixed ~900 ms budget
covered those cases.

Would a floor on max_duration, or a lower bail-out mirroring the new upper
one, be appropriate?

>  	/* We have no traffic in the line so, sooner or later it will go LPI */
> -	while (--retries) {
> +	timeout = jiffies + max_duration;
> +	while (!time_after(jiffies, timeout)) {
>  		memcpy(final, &priv->xstats, sizeof(*final));
>  
>  		if (final->irq_tx_path_in_lpi_mode_n >
> @@ -494,20 +506,38 @@ static int stmmac_test_eee(struct stmmac_priv *priv)
>  		msleep(100);
>  	}
>  
> -	if (!retries) {
> +	memcpy(final, &priv->xstats, sizeof(*final));
> +	if (final->irq_tx_path_in_lpi_mode_n <=
> +	    initial->irq_tx_path_in_lpi_mode_n) {
>  		ret = -ETIMEDOUT;
>  		goto out_free_final;
>  	}
>  
> -	if (final->irq_tx_path_in_lpi_mode_n <=
> -	    initial->irq_tx_path_in_lpi_mode_n) {
> -		ret = -EINVAL;
> +	/* Re-snapshot, as we want to measure exit_lpi events. We should be
> +	 * in LPI right now.
> +	 */
> +	memcpy(initial, &priv->xstats, sizeof(*initial));
> +
> +	/* TX something so we go out of LPI */
> +	ret = stmmac_test_mac_loopback(priv);
> +	if (ret)
>  		goto out_free_final;

[Severity: Medium]
Can the "We should be in LPI right now" assumption be established from
irq_tx_path_in_lpi_mode_n?

That counter is a monotonic per-event counter bumped from the IRQ handler
and never decremented on exit, see dwmac4_irq_status():

	if (status & LPI_CTRL_STATUS_TLPIEN) {
		ret |= CORE_IRQ_TX_PATH_IN_LPI_MODE;
		x->irq_tx_path_in_lpi_mode_n++;
	}

The enter baseline is taken before the first loopback frame, so an entry
event that occurred in the window between the memcpy() and the transmit
inside stmmac_test_mac_loopback() satisfies the first comparison even though
the frame itself terminated that LPI interval:

stmmac_xmit()
	if (priv->tx_path_in_lpi_mode && priv->eee_sw_timer_en)
		stmmac_stop_sw_lpi(priv);

stmmac_stop_sw_lpi()
	timer_delete_sync(&priv->eee_ctrl_timer);
	stmmac_set_lpi_mode(priv, priv->hw, STMMAC_LPI_DISABLE, false, 0);
	priv->tx_path_in_lpi_mode = false;

The same applies when unrelated stack traffic (ARP/ND/MLD on an up
interface) wakes the TX path during the up-to-100 ms poll interval, and for
the LPITXA hardware path where queuing a frame leaves LPI immediately.

In that case the re-snapshot folds the matching exit event into the new exit
baseline, the second frame is sent while the MAC is already awake, no new TX
exit-LPI event is generated, and with no further traffic the second loop
drains its full budget and returns -ETIMEDOUT, which stmmac_selftest_run()
reports as ETH_TEST_FL_FAILED.

The driver already tracks the current state in priv->tx_path_in_lpi_mode
(and LPI_CTRL_STATUS is readable). Would checking the actual state, or
waiting for a fresh entry event after taking the exit baseline and before
sending the wake frame, make the two phases deterministic? As written, phase
one can also pass on an entry event that predates the test frame, so the
enter-after-frame sequence described in the commit message is not strictly
verified.

> +
> +	/* Wait for the exit LPI interrupt */
> +	timeout = jiffies + max_duration;
> +	while (!time_after(jiffies, timeout)) {
> +		memcpy(final, &priv->xstats, sizeof(*final));
> +
> +		if (final->irq_tx_path_exit_lpi_mode_n >
> +		    initial->irq_tx_path_exit_lpi_mode_n)
> +			break;
> +		msleep(100);
>  	}
>  
> +	memcpy(final, &priv->xstats, sizeof(*final));
>  	if (final->irq_tx_path_exit_lpi_mode_n <=
>  	    initial->irq_tx_path_exit_lpi_mode_n) {
> -		ret = -EINVAL;
> +		ret = -ETIMEDOUT;
>  		goto out_free_final;
>  	}
>  

Thanks,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917215339.2022523-1-maxime.chevallier%40bootlin.com

[PATCH net v4 3/7] net: stmmac: selftests: Check the dev->features for S-TAG offload testing

[Maxime Chevallier]

The S-TAG offload insertion incorrectly checks the dvlan (double vlan)
DMA cap, which is different than S-TAG support. Use
NETIF_F_HW_VLAN_STAG_TX to check if the feature is supported instead.

Note that this flag isn't set in stmmac yet, but contrary to ARP
offload, this is a feature that has a chance to get there eventually so
let's leave the selftest here for now. It'll report -EOPNOTSUPP in the
meantime.

Fixes: 091810dbded9 ("net: stmmac: Introduce selftests support")
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
index 2f9f7746c40a..de02c0da56dc 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
@@ -1415,7 +1415,7 @@ static int stmmac_test_vlanoff(struct stmmac_priv *priv)
 
 static int stmmac_test_svlanoff(struct stmmac_priv *priv)
 {
-	if (!priv->dma_cap.dvlan)
+	if (!(priv->dev->features & NETIF_F_HW_VLAN_STAG_TX))
 		return -EOPNOTSUPP;
 	return stmmac_test_vlanoff_common(priv, true);
 }
-- 
2.55.0

Re: [PATCH net v4 3/7] net: stmmac: selftests: Check the dev->features for S-TAG offload testing

[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] stmmac_test_svlanoff() is re-gated on NETIF_F_HW_VLAN_STAG_TX,
  but the test body it enables validates a *double-tagged* (QinQ) frame,…

--

net: stmmac: selftests: Check the dev->features for S-TAG offload testing

The S-TAG insertion selftest was gated on the dvlan (double VLAN) DMA
capability, which is not the same thing as S-TAG insertion support. This
commit switches the gate to NETIF_F_HW_VLAN_STAG_TX, and keeps the test
in tree so it starts working if the driver ever advertises that feature.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> index 2f9f7746c40ae..de02c0da56dc3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> @@ -1415,7 +1415,7 @@ static int stmmac_test_vlanoff(struct stmmac_priv *priv)
>  
>  static int stmmac_test_svlanoff(struct stmmac_priv *priv)
>  {
> -	if (!priv->dma_cap.dvlan)
> +	if (!(priv->dev->features & NETIF_F_HW_VLAN_STAG_TX))
>  		return -EOPNOTSUPP;
>  	return stmmac_test_vlanoff_common(priv, true);
>  }

[Severity: Medium]
Does the new gate match what the test body actually validates?  The gate
now keys on single S-TAG transmit offload, but the code it enables appears
to expect a double-tagged (QinQ) frame.

In stmmac_test_vlanoff_common() the frame is built untagged (attr is
`struct stmmac_packet_attrs attr = { }` and attr.vlan is never set, so
stmmac_test_get_udp_skb() skips its `if (attr->vlan)` block), and exactly
one acceleration tag is attached:

	__vlan_hwaccel_put_tag(skb, htons(proto), tpriv->vlan_id);
	skb->protocol = htons(proto);

while the ptype registered for the svlan case listens for 802.1Q:

	tpriv->pt.type = svlan ? htons(ETH_P_8021Q) : htons(ETH_P_IP);

And stmmac_test_vlan_validate() skips a 4-byte inner tag before the IP
header when double_vlan is set:

	ihdr = ip_hdr(skb);
	if (tpriv->double_vlan)
		ihdr = (struct iphdr *)(skb_network_header(skb) + 4);
	if (ihdr->protocol != IPPROTO_UDP)
		goto out;

With a conforming NETIF_F_HW_VLAN_STAG_TX implementation the looped-back
frame is DA|SA|0x88A8|TCI|0x0800|IP.  After skb_vlan_untag() the ethertype
is ETH_P_IP, so the ETH_P_8021Q ptype is never matched, and even if it
were, skb_network_header(skb) + 4 lands 4 bytes into the IP header and the
IPPROTO_UDP check fails.  Wouldn't that make the test report -ETIMEDOUT
the moment the feature bit is advertised?

Along the same lines, can the current transmit path ever produce the frame
the validator wants?  stmmac_vlan_insert() in stmmac_main.c ignores
skb->vlan_proto and hardcodes the inner tag and type:

	if (stmmac_set_desc_vlan_tag(priv, p, tag, 0x0, 0x0))
		return false;

and vlan_enable() in stmmac_vlan.c selects C-VLAN insertion only:

	value &= ~VLAN_CSVL; /* Only use CVLAN */

So the only way the retained test could pass seems to be an implementation
that wrongly inserts an extra inner C-TAG.

Would it make sense to also adjust the frame and the validation here (for
example setting attr.vlan for an inner C-TAG with pt.type = ETH_P_IP, or
dropping the double_vlan expectation for the S-TAG case) rather than only
swapping the capability bit, so the test matches the semantics of the
feature it is now gated on?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917215339.2022523-1-maxime.chevallier%40bootlin.com

[PATCH net v4 4/7] net: stmmac: selftests: Capture all packets for vlan checks

[Maxime Chevallier]

While we use vlan_vid_add to trigger the tag filtering machinery
in the driver, there's no netdev associated to the VLAN. This causes the
skb to arrive with empty skb->vlan_tci fields, as the packet is marked
OTHERHOST in __netif_receive_skb_core(), and we fail our validation.

Let's use the proxy mechanism introduced for DSA, that registers a
ETH_P_ALL packet handler that runs earlier, before the vlan netdev
lookup, then filters for the correct ethertype before passing an skb
clone to our validation function.

As we may receive external frames with the right tag from the outside,
let's move the address check in the vlan validation function earlier.

Fixes: 091810dbded9 ("net: stmmac: Introduce selftests support")
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 .../stmicro/stmmac/stmmac_selftests.c         | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
index de02c0da56dc..43b8411c5112 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
@@ -242,6 +242,7 @@ struct stmmac_test_priv {
 	__be16 packet_type;
 	int (*func)(struct sk_buff *skb, struct net_device *ndev,
 		    struct packet_type *pt, struct net_device *orig_ndev);
+	bool capture_all;
 	int double_vlan;
 	int vlan_id;
 	int ok;
@@ -344,13 +345,15 @@ static void stmmac_sft_add_pack(struct packet_type *pt)
 {
 	struct stmmac_test_priv *tpriv = pt->af_packet_priv;
 
-	if (netdev_uses_dsa(tpriv->pt.dev)) {
+	if (netdev_uses_dsa(tpriv->pt.dev) || tpriv->capture_all) {
 		tpriv->packet_type = tpriv->pt.type;
 		tpriv->func = tpriv->pt.func;
 
 		/* DSA conduit will report ETH_P_XDSA, so our packet handler
 		 * won't match. Let's register a ETH_P_ALL match and filter
-		 * manually in stmmac_sft_filter.
+		 * manually in stmmac_sft_filter. This is also useful for
+		 * VLAN tests, to capture packets otherwise marked as
+		 * OTHERHOST.
 		 */
 		tpriv->pt.type = htons(ETH_P_ALL);
 		tpriv->pt.func = stmmac_sft_filter;
@@ -943,6 +946,11 @@ static int stmmac_test_vlan_validate(struct sk_buff *skb,
 		goto out;
 	if (skb_headlen(skb) < (STMMAC_TEST_PKT_SIZE - ETH_HLEN))
 		goto out;
+
+	ehdr = (struct ethhdr *)skb_mac_header(skb);
+	if (!ether_addr_equal_unaligned(ehdr->h_dest, tpriv->packet->dst))
+		goto out;
+
 	if (tpriv->vlan_id) {
 		if (skb->vlan_proto != htons(proto))
 			goto out;
@@ -954,10 +962,6 @@ static int stmmac_test_vlan_validate(struct sk_buff *skb,
 		}
 	}
 
-	ehdr = (struct ethhdr *)skb_mac_header(skb);
-	if (!ether_addr_equal_unaligned(ehdr->h_dest, tpriv->packet->dst))
-		goto out;
-
 	ihdr = ip_hdr(skb);
 	if (tpriv->double_vlan)
 		ihdr = (struct iphdr *)(skb_network_header(skb) + 4);
@@ -999,6 +1003,7 @@ static int __stmmac_test_vlanfilt(struct stmmac_priv *priv)
 	tpriv->pt.dev = priv->dev;
 	tpriv->pt.af_packet_priv = tpriv;
 	tpriv->packet = &attr;
+	tpriv->capture_all = true;
 
 	/*
 	 * As we use HASH filtering, false positives may appear. This is a
@@ -1095,6 +1100,7 @@ static int __stmmac_test_dvlanfilt(struct stmmac_priv *priv)
 	tpriv->pt.dev = priv->dev;
 	tpriv->pt.af_packet_priv = tpriv;
 	tpriv->packet = &attr;
+	tpriv->capture_all = true;
 
 	/*
 	 * As we use HASH filtering, false positives may appear. This is a
@@ -1375,6 +1381,7 @@ static int stmmac_test_vlanoff_common(struct stmmac_priv *priv, bool svlan)
 	tpriv->pt.af_packet_priv = tpriv;
 	tpriv->packet = &attr;
 	tpriv->vlan_id = 0x123;
+	tpriv->capture_all = true;
 
 	ret = vlan_vid_add(priv->dev, htons(proto), tpriv->vlan_id);
 	if (ret)
-- 
2.55.0

Re: [PATCH net v4 4/7] net: stmmac: selftests: Capture all packets for vlan checks

[netdev-bot+sashiko]

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

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

- [High] The "Means filter did not work" branch that this patch newly
  makes reachable encodes a detected filtering failure the same way the…
- [Medium] Making the VLAN selftest handler an ETH_P_ALL tap exposes the
  test's unsynchronized shared state to arbitrary concurrently received…
- [Medium] The VLAN validator now parses untrusted frames with an
  attacker-controlled offset and an insufficient length guard.

--

net: stmmac: selftests: Capture all packets for vlan checks

This reuses the DSA proxy mechanism for the VLAN selftests, registering an
ETH_P_ALL handler so that frames marked OTHERHOST still reach
stmmac_test_vlan_validate(). The destination address check inside the
validation function is moved earlier since external frames can now be seen.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> index de02c0da56dc3..43b8411c51120 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c

[ ... ]

> @@ -344,13 +345,15 @@ static void stmmac_sft_add_pack(struct packet_type *pt)
>  {
>  	struct stmmac_test_priv *tpriv = pt->af_packet_priv;
>  
> -	if (netdev_uses_dsa(tpriv->pt.dev)) {
> +	if (netdev_uses_dsa(tpriv->pt.dev) || tpriv->capture_all) {
>  		tpriv->packet_type = tpriv->pt.type;
>  		tpriv->func = tpriv->pt.func;
>  
>  		/* DSA conduit will report ETH_P_XDSA, so our packet handler
>  		 * won't match. Let's register a ETH_P_ALL match and filter
> -		 * manually in stmmac_sft_filter.
> +		 * manually in stmmac_sft_filter. This is also useful for
> +		 * VLAN tests, to capture packets otherwise marked as
> +		 * OTHERHOST.
>  		 */
>  		tpriv->pt.type = htons(ETH_P_ALL);
>  		tpriv->pt.func = stmmac_sft_filter;

> @@ -943,6 +946,11 @@ static int stmmac_test_vlan_validate(struct sk_buff *skb,
>  		goto out;
>  	if (skb_headlen(skb) < (STMMAC_TEST_PKT_SIZE - ETH_HLEN))
>  		goto out;
> +
> +	ehdr = (struct ethhdr *)skb_mac_header(skb);
> +	if (!ether_addr_equal_unaligned(ehdr->h_dest, tpriv->packet->dst))
> +		goto out;
> +
>  	if (tpriv->vlan_id) {
>  		if (skb->vlan_proto != htons(proto))
>  			goto out;

[Severity: High]

With the ETH_P_ALL registration the vid comparison further down in
stmmac_test_vlan_validate() becomes reachable on non-DSA devices, since
the ptype_all walk in __netif_receive_skb_core() happens before
__vlan_hwaccel_clear_tag() and skb->vlan_proto is no longer zero:

	if (tpriv->vlan_id) {
		if (skb->vlan_proto != htons(proto))
			goto out;
		if (skb->vlan_tci != tpriv->vlan_id) {
			/* Means filter did not work. */
			tpriv->ok = false;
			complete(&tpriv->comp);
			goto out;
		}
	}

Is the ok = false encoding here distinguishable from "nothing was
received"?  The callers in __stmmac_test_vlanfilt() and
__stmmac_test_dvlanfilt() do:

		wait_for_completion_timeout(&tpriv->comp, STMMAC_LB_TIMEOUT);
		ret = tpriv->ok ? 0 : -ETIMEDOUT;
		if (ret && !i) {
			goto vlan_del;
		} else if (!ret && i) {
			ret = -EINVAL;
			goto vlan_del;
		} else {
			ret = 0;
		}

For iterations i = 1..3 the frame is sent with VID tpriv->vlan_id + i and
is expected to be dropped by the hardware filter.  If the filter leaks the
frame, its tci cannot equal tpriv->vlan_id, so it can only take the
"filter did not work" branch, which yields ret = -ETIMEDOUT, and the else
arm then rewrites that to ret = 0.

Does this mean a real VLAN filter leak is reported as a pass for every
i > 0?

Conversely, at i == 0 any tagged frame addressed to our MAC that passes
the hash filter (the in-code comment above acknowledges false positives)
also lands in that branch and aborts the test with -ETIMEDOUT.

Would a distinct state, for example leaving ok = true or adding a separate
"unexpected frame" flag consumed by the !ret && i arm, be better here?

>  		}
>  	}
>  
> -	ehdr = (struct ethhdr *)skb_mac_header(skb);
> -	if (!ether_addr_equal_unaligned(ehdr->h_dest, tpriv->packet->dst))
> -		goto out;
> -
>  	ihdr = ip_hdr(skb);
>  	if (tpriv->double_vlan)
>  		ihdr = (struct iphdr *)(skb_network_header(skb) + 4);

[Severity: Medium]

Now that frames from the wire reach this parsing code, can the offset
arithmetic below read past the validated data?

	uhdr = (struct udphdr *)((u8 *)ihdr + 4 * ihdr->ihl);
	if (uhdr->dest != htons(tpriv->packet->dport))
		goto out;

	shdr = (struct stmmachdr *)((u8 *)uhdr + sizeof(*uhdr));
	if (shdr->magic != cpu_to_be64(STMMAC_TEST_PKT_MAGIC))

The only length guard is:

	if (skb_headlen(skb) < (STMMAC_TEST_PKT_SIZE - ETH_HLEN))
		goto out;

and STMMAC_TEST_PKT_SIZE is:

#define STMMAC_TEST_PKT_SIZE (sizeof(struct ethhdr) + sizeof(struct iphdr) + \
			      sizeof(struct stmmachdr))

so it guarantees 33 bytes after the MAC header, with no allowance for
struct udphdr and none for the extra 4 bytes of the double_vlan path.
ihdr->ihl comes from the wire and can be up to 15, so uhdr can sit up to
60 bytes past ihdr and shdr->magic can be read roughly 84 bytes past the
network header.

The proxy itself does no validation either, it clones after an ethertype
comparison only:

	struct ethhdr *hdr = eth_hdr(skb);

	if (hdr->h_proto == tpriv->packet_type) {
		struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);

A 60-byte frame with ihl = 15 and protocol = UDP addressed to our MAC gives
skb_headlen() == 46, passes the 33-byte check, and then reads about 30
bytes beyond skb->tail.  Should a pskb_may_pull() or an explicit
remaining-length check against 4 * ihdr->ihl + sizeof(*uhdr) +
sizeof(*shdr) be added before these dereferences?

The same unchecked ihl arithmetic already exists in
stmmac_test_loopback_validate(), so that part is not new, but this change
is what exposes the VLAN validator to external frames.

> @@ -999,6 +1003,7 @@ static int __stmmac_test_vlanfilt(struct stmmac_priv *priv)
>  	tpriv->pt.dev = priv->dev;
>  	tpriv->pt.af_packet_priv = tpriv;
>  	tpriv->packet = &attr;
> +	tpriv->capture_all = true;

[Severity: Medium]

Does the 4-iteration loop in __stmmac_test_vlanfilt() need a
reinit_completion()?  init_completion() is called once before the loop and
the loop only resets the verdict:

		tpriv->ok = false;
	}

With the promiscuous tap, unrelated received frames addressed to our MAC
can now call complete(&tpriv->comp) from the vid-mismatch branch of
stmmac_test_vlan_validate().  Every surplus complete() leaves done > 0, so
the next iteration's wait_for_completion_timeout() returns immediately
without ever waiting for its own frame.

Also, tpriv->ok is a plain int written from NAPI softirq context in
stmmac_test_vlan_validate() and written/read from process context in the
loop, with no common lock and no READ_ONCE/WRITE_ONCE.  If a softirq write
of ok = true lands after the process-context reset, the caller can observe
ok == true in an iteration where a timeout is expected and turn that into
a bogus -EINVAL.

> @@ -1095,6 +1100,7 @@ static int __stmmac_test_dvlanfilt(struct stmmac_priv *priv)
>  	tpriv->pt.dev = priv->dev;
>  	tpriv->pt.af_packet_priv = tpriv;
>  	tpriv->packet = &attr;
> +	tpriv->capture_all = true;

[Severity: Medium]

The same two points apply to __stmmac_test_dvlanfilt(): the loop resets
tpriv->ok without re-arming the completion, and the verdict encoding
question above applies to its identical ret = tpriv->ok ? 0 : -ETIMEDOUT
handling.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917215339.2022523-1-maxime.chevallier%40bootlin.com

[PATCH net v4 5/7] net: stmmac: dwmac4: Use the correct bufzise when the len is exactly 8K

[Maxime Chevallier]

DMA bufsize selection isn't made on the MTU but the actual frame length,
so including the L2 header. On DWMAC4, if the len is exactly BUF_SIZE_8KiB,
the next larger size is incorrectly selected.

Lets fix the comparison and while at it, rename the parameter from len
to mtu.

Fixes: c3efed5ad1b0 ("net: stmmac: Enable dwmac4 jumbo frame more than 8KiB").
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c | 4 ++--
 drivers/net/ethernet/stmicro/stmmac/hwif.h         | 2 +-
 drivers/net/ethernet/stmicro/stmmac/ring_mode.c    | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
index 2994df41ec2c..c6a8f8d73501 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
@@ -474,11 +474,11 @@ static void dwmac4_set_sarc(struct dma_desc *p, u32 sarc_type)
 					  sarc_type));
 }
 
-static int set_16kib_bfsize(int mtu)
+static int set_16kib_bfsize(int len)
 {
 	int ret = 0;
 
-	if (unlikely(mtu >= BUF_SIZE_8KiB))
+	if (unlikely(len > BUF_SIZE_8KiB))
 		ret = BUF_SIZE_16KiB;
 	return ret;
 }
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index 04dafec021b4..de7aa6d28148 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -540,7 +540,7 @@ struct stmmac_mode_ops {
 	bool (*is_jumbo_frm)(unsigned int len, bool enh_desc);
 	int (*jumbo_frm)(struct stmmac_tx_queue *tx_q, struct sk_buff *skb,
 			 int csum);
-	int (*set_16kib_bfsize)(int mtu);
+	int (*set_16kib_bfsize)(int len);
 	void (*init_desc3)(struct dma_desc *p);
 	void (*refill_desc3)(struct stmmac_rx_queue *rx_q, struct dma_desc *p);
 	void (*clean_desc3)(struct stmmac_tx_queue *tx_q, struct dma_desc *p);
diff --git a/drivers/net/ethernet/stmicro/stmmac/ring_mode.c b/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
index f7949419eb9f..d2f0c321661d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
+++ b/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
@@ -124,10 +124,10 @@ static void clean_desc3(struct stmmac_tx_queue *tx_q, struct dma_desc *p)
 		p->des3 = 0;
 }
 
-static int set_16kib_bfsize(int mtu)
+static int set_16kib_bfsize(int len)
 {
 	int ret = 0;
-	if (unlikely(mtu > BUF_SIZE_8KiB))
+	if (unlikely(len > BUF_SIZE_8KiB))
 		ret = BUF_SIZE_16KiB;
 	return ret;
 }
-- 
2.55.0

Re: [PATCH net v4 5/7] net: stmmac: dwmac4: Use the correct bufzise when the len is exactly 8K

[Nicolai Buchwitz]

Hi Maxime

On 17.9.2026 23:53, Maxime Chevallier wrote:
> DMA bufsize selection isn't made on the MTU but the actual frame 
> length,
> so including the L2 header. On DWMAC4, if the len is exactly 
> BUF_SIZE_8KiB,
> the next larger size is incorrectly selected.
> 
> Lets fix the comparison and while at it, rename the parameter from len
> to mtu.

Isn't it "[...] from mtu to len"?

> 
> Fixes: c3efed5ad1b0 ("net: stmmac: Enable dwmac4 jumbo frame more than 
> 8KiB").
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c | 4 ++--
>  drivers/net/ethernet/stmicro/stmmac/hwif.h         | 2 +-
>  drivers/net/ethernet/stmicro/stmmac/ring_mode.c    | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c 
> b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> index 2994df41ec2c..c6a8f8d73501 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> @@ -474,11 +474,11 @@ static void dwmac4_set_sarc(struct dma_desc *p, 
> u32 sarc_type)
>  					  sarc_type));
>  }
> 
> -static int set_16kib_bfsize(int mtu)
> +static int set_16kib_bfsize(int len)
>  {
>  	int ret = 0;
> 
> -	if (unlikely(mtu >= BUF_SIZE_8KiB))
> +	if (unlikely(len > BUF_SIZE_8KiB))

Should we drop set_16kib_bfsize() instead?

After 6/7 it looks to me like the same test as the first branch of
stmmac_set_bfsize(). The caller ends up there anyway when it returns 0
and chain_mode_ops doesn't have it at all.

>  		ret = BUF_SIZE_16KiB;
>  	return ret;
>  }
> diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h 
> b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> index 04dafec021b4..de7aa6d28148 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> @@ -540,7 +540,7 @@ struct stmmac_mode_ops {
>  	bool (*is_jumbo_frm)(unsigned int len, bool enh_desc);
>  	int (*jumbo_frm)(struct stmmac_tx_queue *tx_q, struct sk_buff *skb,
>  			 int csum);
> -	int (*set_16kib_bfsize)(int mtu);
> +	int (*set_16kib_bfsize)(int len);
>  	void (*init_desc3)(struct dma_desc *p);
>  	void (*refill_desc3)(struct stmmac_rx_queue *rx_q, struct dma_desc 
> *p);
>  	void (*clean_desc3)(struct stmmac_tx_queue *tx_q, struct dma_desc 
> *p);
> diff --git a/drivers/net/ethernet/stmicro/stmmac/ring_mode.c 
> b/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
> index f7949419eb9f..d2f0c321661d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
> @@ -124,10 +124,10 @@ static void clean_desc3(struct stmmac_tx_queue 
> *tx_q, struct dma_desc *p)
>  		p->des3 = 0;
>  }
> 
> -static int set_16kib_bfsize(int mtu)
> +static int set_16kib_bfsize(int len)
>  {
>  	int ret = 0;
> -	if (unlikely(mtu > BUF_SIZE_8KiB))
> +	if (unlikely(len > BUF_SIZE_8KiB))
>  		ret = BUF_SIZE_16KiB;
>  	return ret;
>  }

regards
Nicolai

Re: [PATCH net v4 5/7] net: stmmac: dwmac4: Use the correct bufzise when the len is exactly 8K

[Maxime Chevallier]

On 9/18/26 14:25, Nicolai Buchwitz wrote:
> Hi Maxime
> 
> On 17.9.2026 23:53, Maxime Chevallier wrote:
>> DMA bufsize selection isn't made on the MTU but the actual frame length,
>> so including the L2 header. On DWMAC4, if the len is exactly BUF_SIZE_8KiB,
>> the next larger size is incorrectly selected.
>>
>> Lets fix the comparison and while at it, rename the parameter from len
>> to mtu.
> 
> Isn't it "[...] from mtu to len"?

:(

Bah, if respin is required I'll fix this

>>
>> Fixes: c3efed5ad1b0 ("net: stmmac: Enable dwmac4 jumbo frame more than 8KiB").
>> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
>> ---
>>  drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c | 4 ++--
>>  drivers/net/ethernet/stmicro/stmmac/hwif.h         | 2 +-
>>  drivers/net/ethernet/stmicro/stmmac/ring_mode.c    | 4 ++--
>>  3 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
>> index 2994df41ec2c..c6a8f8d73501 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
>> @@ -474,11 +474,11 @@ static void dwmac4_set_sarc(struct dma_desc *p, u32 sarc_type)
>>                        sarc_type));
>>  }
>>
>> -static int set_16kib_bfsize(int mtu)
>> +static int set_16kib_bfsize(int len)
>>  {
>>      int ret = 0;
>>
>> -    if (unlikely(mtu >= BUF_SIZE_8KiB))
>> +    if (unlikely(len > BUF_SIZE_8KiB))
> 
> Should we drop set_16kib_bfsize() instead?
> 
> After 6/7 it looks to me like the same test as the first branch of
> stmmac_set_bfsize(). The caller ends up there anyway when it returns 0
> and chain_mode_ops doesn't have it at all.

True yeah :) however maybe we can wait for this to be merged and have the
cleanup in net-next ?

Sashiko is getting crazy with the "pre-existing issues" everytime we touch
stmmac, cleanups are long overdue and this could be included in there :(

Maxime

Re: [PATCH net v4 5/7] net: stmmac: dwmac4: Use the correct bufzise when the len is exactly 8K

[Nicolai Buchwitz]

On September 18, 2026 2:45:10 PM GMT+02:00, Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:
>
>
>On 9/18/26 14:25, Nicolai Buchwitz wrote:
>> Hi Maxime
>> 
>> On 17.9.2026 23:53, Maxime Chevallier wrote:
>>> DMA bufsize selection isn't made on the MTU but the actual frame length,
>>> so including the L2 header. On DWMAC4, if the len is exactly BUF_SIZE_8KiB,
>>> the next larger size is incorrectly selected.
>>>
>>> Lets fix the comparison and while at it, rename the parameter from len
>>> to mtu.
>> 
>> Isn't it "[...] from mtu to len"?
>
>:(
>
>Bah, if respin is required I'll fix this
>
>>>
>>> Fixes: c3efed5ad1b0 ("net: stmmac: Enable dwmac4 jumbo frame more than 8KiB").
>>> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
>>> ---
>>>  drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c | 4 ++--
>>>  drivers/net/ethernet/stmicro/stmmac/hwif.h         | 2 +-
>>>  drivers/net/ethernet/stmicro/stmmac/ring_mode.c    | 4 ++--
>>>  3 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
>>> index 2994df41ec2c..c6a8f8d73501 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
>>> @@ -474,11 +474,11 @@ static void dwmac4_set_sarc(struct dma_desc *p, u32 sarc_type)
>>>                        sarc_type));
>>>  }
>>>
>>> -static int set_16kib_bfsize(int mtu)
>>> +static int set_16kib_bfsize(int len)
>>>  {
>>>      int ret = 0;
>>>
>>> -    if (unlikely(mtu >= BUF_SIZE_8KiB))
>>> +    if (unlikely(len > BUF_SIZE_8KiB))
>> 
>> Should we drop set_16kib_bfsize() instead?
>> 
>> After 6/7 it looks to me like the same test as the first branch of
>> stmmac_set_bfsize(). The caller ends up there anyway when it returns 0
>> and chain_mode_ops doesn't have it at all.
>
>True yeah :) however maybe we can wait for this to be merged and have the
>cleanup in net-next ?
>
>Sashiko is getting crazy with the "pre-existing issues" everytime we touch
>stmmac, cleanups are long overdue and this could be included in there :(
>

Totally fine with me. Maybe the commit message can be fixed up on merge? 

>Maxime
>
>
FWIW

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Re: [PATCH net v4 5/7] net: stmmac: dwmac4: Use the correct bufzise when the len is exactly 8K

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] Taken in isolation, the only functional hunk of this patch
  (drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c:477-484,…
- [Low] Commit-message / diff mismatch in "net: stmmac: dwmac4: Use the
  correct bufzise when the len is exactly 8K": the changelog states…

--

net: stmmac: dwmac4: Use the correct bufzise when the len is exactly 8K

This changes the dwmac4 set_16kib_bfsize() comparison from >= BUF_SIZE_8KiB
to > BUF_SIZE_8KiB so that a frame length of exactly 8KiB does not select the
next larger DMA buffer size, and renames the callback parameter in
dwmac4_descs.c, ring_mode.c and the stmmac_mode_ops prototype.

No Link: tags in the commit header.

> net: stmmac: dwmac4: Use the correct bufzise when the len is exactly 8K

[Severity: Low]
This isn't a bug, but the subject has a typo: "bufzise" should be "bufsize".

>     Lets fix the comparison and while at it, rename the parameter from len
>     to mtu.

Does the changelog describe the rename in the right direction?  The diff
renames mtu to len, not len to mtu:

  -static int set_16kib_bfsize(int mtu)
  +static int set_16kib_bfsize(int len)

Same for the ops prototype in hwif.h and for ring_mode.c.  Should this read
"rename the parameter from mtu to len"?

Also, at this commit the caller still passes an MTU, not a frame length.
stmmac_setup_dma_desc() documents "@mtu: MTU to setup the dma queue and buf
with", is called with dev->mtu from stmmac_open() and with new_mtu from
stmmac_change_mtu(), keeps the comment "Returns 0 or BUF_SIZE_16KiB if mtu >
8KiB and dwmac4 or ring mode", and the sibling helper is still
stmmac_set_bfsize(int mtu).  Nothing on that path adds ETH_HLEN, VLAN or FCS
yet.  That unit mismatch is resolved by the following patch in the series
("net: stmmac: size the RX buffers from the frame length, not the MTU"),
which computes len = mtu + ETH_HLEN + 2 * VLAN_HLEN + ETH_FCS_LEN and renames
stmmac_set_bfsize()'s parameter too, but the inverted rename sentence in this
changelog would still be worth correcting on respin.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> index 2994df41ec2c4..c6a8f8d735015 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> @@ -474,11 +474,11 @@ static void dwmac4_set_sarc(struct dma_desc *p, u32 sarc_type)
>  					  sarc_type));
>  }
>  
> -static int set_16kib_bfsize(int mtu)
> +static int set_16kib_bfsize(int len)
>  {
>  	int ret = 0;
>  
> -	if (unlikely(mtu >= BUF_SIZE_8KiB))
> +	if (unlikely(len > BUF_SIZE_8KiB))
>  		ret = BUF_SIZE_16KiB;
>  	return ret;
>  }

[Severity: Low]
Taken on its own, does this hunk change the DMA buffer size the driver ends
up programming?

The only caller is stmmac_setup_dma_desc(), which falls back when the
callback returns anything below BUF_SIZE_16KiB:

	/* Returns 0 or BUF_SIZE_16KiB if mtu > 8KiB and dwmac4 or ring mode */
	bfsize = stmmac_set_16kib_bfsize(priv, mtu);
	if (bfsize < 0)
		bfsize = 0;

	if (bfsize < BUF_SIZE_16KiB)
		bfsize = stmmac_set_bfsize(mtu);

	dma_conf->dma_buf_sz = bfsize;

For the single input whose result changes here (exactly BUF_SIZE_8KiB) the
callback now returns 0, so the fallback runs, and stmmac_set_bfsize() at this
commit still uses the inclusive comparison:

static int stmmac_set_bfsize(int mtu)
{
	int ret;

	if (mtu >= BUF_SIZE_8KiB)
		ret = BUF_SIZE_16KiB;

so dma_buf_sz comes out as BUF_SIZE_16KiB both before and after this patch.
The boundary case is only actually fixed by the next patch in the series
("net: stmmac: size the RX buffers from the frame length, not the MTU"),
which switches stmmac_set_bfsize() to a strict > comparison and feeds both
helpers len = mtu + ETH_HLEN + 2 * VLAN_HLEN + ETH_FCS_LEN.

Given that, is the standalone Fixes: c3efed5ad1b0 tag on this commit right?
Backported alone to stable it would have no effect.  Would it make sense to
either squash the two changes or note the dependency?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> index 9314bcb85c221..857f7562c6c6d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> @@ -540,7 +540,7 @@ struct stmmac_mode_ops {
>  	bool (*is_jumbo_frm)(unsigned int len, bool enh_desc);
>  	int (*jumbo_frm)(struct stmmac_tx_queue *tx_q, struct sk_buff *skb,
>  			 int csum);
> -	int (*set_16kib_bfsize)(int mtu);
> +	int (*set_16kib_bfsize)(int len);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917215339.2022523-1-maxime.chevallier%40bootlin.com

[PATCH net v4 6/7] net: stmmac: size the RX buffers from the frame length, not the MTU

[Maxime Chevallier]

When picking the buffsize to use based on the MTU, we shouldn't check
only the MTU value, but also :
 - ETH_HLEN for the L2 header,
 - up to 2 VLAN tags,
 - the FCS,

The default bufsize is 1536 bytes, which is enough to contain all the
above so this hasn't surfaced before, but the addition of NET_IP_ALIGN
to the start of buffer address tripped the Jumbo selftest, leading to
this discovery.

With that, we don't need the '>=' checks on the buffer len, we can use
more consistent comparison operators in stmmac_set_bfsize.

Fixes: 286a83721720 ("stmmac: add CHAINED descriptor mode support (V4)")
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 20 ++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 1fb5f804ea23..d5a984ad864f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1536,17 +1536,17 @@ static unsigned int stmmac_rx_offset(struct stmmac_priv *priv)
 	return NET_SKB_PAD + NET_IP_ALIGN;
 }
 
-static int stmmac_set_bfsize(int mtu)
+static int stmmac_set_bfsize(int len)
 {
 	int ret;
 
-	if (mtu >= BUF_SIZE_8KiB)
+	if (len > BUF_SIZE_8KiB)
 		ret = BUF_SIZE_16KiB;
-	else if (mtu >= BUF_SIZE_4KiB)
+	else if (len > BUF_SIZE_4KiB)
 		ret = BUF_SIZE_8KiB;
-	else if (mtu >= BUF_SIZE_2KiB)
+	else if (len > BUF_SIZE_2KiB)
 		ret = BUF_SIZE_4KiB;
-	else if (mtu > DEFAULT_BUFSIZE)
+	else if (len > DEFAULT_BUFSIZE)
 		ret = BUF_SIZE_2KiB;
 	else
 		ret = DEFAULT_BUFSIZE;
@@ -4063,7 +4063,7 @@ static struct stmmac_dma_conf *
 stmmac_setup_dma_desc(struct stmmac_priv *priv, unsigned int mtu)
 {
 	struct stmmac_dma_conf *dma_conf;
-	int bfsize, ret;
+	int bfsize, len, ret;
 	u8 chan;
 
 	dma_conf = kzalloc_obj(*dma_conf);
@@ -4073,13 +4073,15 @@ stmmac_setup_dma_desc(struct stmmac_priv *priv, unsigned int mtu)
 		return ERR_PTR(-ENOMEM);
 	}
 
-	/* Returns 0 or BUF_SIZE_16KiB if mtu > 8KiB and dwmac4 or ring mode */
-	bfsize = stmmac_set_16kib_bfsize(priv, mtu);
+	len = mtu + ETH_HLEN + 2 * VLAN_HLEN + ETH_FCS_LEN;
+
+	/* Returns 0 or BUF_SIZE_16KiB if len > 8KiB and dwmac4 or ring mode */
+	bfsize = stmmac_set_16kib_bfsize(priv, len);
 	if (bfsize < 0)
 		bfsize = 0;
 
 	if (bfsize < BUF_SIZE_16KiB)
-		bfsize = stmmac_set_bfsize(mtu);
+		bfsize = stmmac_set_bfsize(len);
 
 	dma_conf->dma_buf_sz = bfsize;
 	/* Chose the tx/rx size from the already defined one in the
-- 
2.55.0

[PATCH net v4 7/7] net: stmmac: selftests: Account for alignment shift on dwmac1000 for Jumbo test

[Maxime Chevallier]

On dwmac1000, we currently only support single-descriptor frames. The
Jumbo test started failing when NET_IP_ALIGN was added to align the IP
header, as this tests tries to send the biggest possible frame.

On dwmac1000 the DMA transfer is aligned on 4-bytes, so adding a 2-byte
shift at the start-of-buffer address means it takes a whole extra 4-byte
DMA burst to receive the Jumbo packet, causing it to spill over the next
descriptor.

This doesn't seem to happen on dwmac4 and xgmac that appear to correctly
handle unaligned xfers (only tested on dwmac4)

Let's account for that in the Jumbo test, reduce the size of our big
packet by the align size.

Fixes: 23680bf5f8c6 ("net: stmmac: restore NET_IP_ALIGN in the RX DMA offset")
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
index 43b8411c5112..c25dc9f89270 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
@@ -1789,6 +1789,9 @@ static int __stmmac_test_jumbo(struct stmmac_priv *priv, u16 queue)
 	struct stmmac_packet_attrs attr = { };
 	int size = priv->dma_conf.dma_buf_sz;
 
+	if (!dwmac_is_xmac(priv->plat->core_type))
+		size -= NET_IP_ALIGN;
+
 	attr.dst = priv->dev->dev_addr;
 	attr.max_size = size - ETH_FCS_LEN;
 	attr.queue_mapping = queue;
-- 
2.55.0

Re: [PATCH net v4 7/7] net: stmmac: selftests: Account for alignment shift on dwmac1000 for Jumbo test

[netdev-bot+sashiko]

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

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

- [High] Incomplete fix: the NET_IP_ALIGN compensation is applied only to
  the selftest packet, while the driver keeps programming the full,…

--

net: stmmac: selftests: Account for alignment shift on dwmac1000 for Jumbo test

The Jumbo selftest sends a frame sized from dma_conf.dma_buf_sz, which
started failing on legacy cores after NET_IP_ALIGN was restored in the RX
DMA offset.  The patch shrinks the test frame by NET_IP_ALIGN on non-xmac
cores so the frame no longer spills into a second descriptor.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> index 43b8411c51120..c25dc9f89270c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> @@ -1789,6 +1789,9 @@ static int __stmmac_test_jumbo(struct stmmac_priv *priv, u16 queue)
>  	struct stmmac_packet_attrs attr = { };
>  	int size = priv->dma_conf.dma_buf_sz;
>  
> +	if (!dwmac_is_xmac(priv->plat->core_type))
> +		size -= NET_IP_ALIGN;
> +

[Severity: High]
If the hardware behaviour described in the changelog is accurate, does
this only hide the mismatch in __stmmac_test_jumbo() while leaving the
same shortfall on the receive path for real traffic?

The RX DMA start address is shifted by NET_IP_ALIGN:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_rx_offset() {
	if (stmmac_xdp_is_enabled(priv))
		return XDP_PACKET_HEADROOM + NET_IP_ALIGN;

	return NET_SKB_PAD + NET_IP_ALIGN;
}

but the descriptor buffer length programmed for that shifted address is
still the full dma_buf_sz:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_clear_rx_descriptors() {
		stmmac_init_rx_desc(priv, desc, priv->use_riwt,
				    priv->descriptor_mode,
				    (i == dma_conf->dma_rx_size - 1),
				    dma_conf->dma_buf_sz);
}

drivers/net/ethernet/stmicro/stmmac/enh_desc.c:enh_desc_init_rx_desc() {
	bfsize1 = min(bfsize, BUF_SIZE_8KiB);
	p->des1 |= cpu_to_le32(bfsize1 & ERDES1_BUFFER1_SIZE_MASK);
}

So on non-xmac cores the usable single-descriptor capacity is
dma_buf_sz - NET_IP_ALIGN, which is exactly what this patch encodes in the
test, but nothing reduces the value handed to the hardware.

Can a frame from the wire hit that same boundary?  The buffer size comes
from the MTU, and the rounding uses strict greater-than comparisons:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_setup_dma_desc() {
	len = mtu + ETH_HLEN + 2 * VLAN_HLEN + ETH_FCS_LEN;
	...
	if (bfsize < BUF_SIZE_16KiB)
		bfsize = stmmac_set_bfsize(len);
}

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_set_bfsize() {
	else if (len > DEFAULT_BUFSIZE)
		ret = BUF_SIZE_2KiB;
	else
		ret = DEFAULT_BUFSIZE;
}

With DEFAULT_BUFSIZE == 1536, an MTU of 1510 gives len == 1536 and
dma_buf_sz == 1536; MTU 2022 gives 2048, MTU 4070 gives 4096.  For those
MTUs a maximum-size double-tagged frame including FCS is exactly
dma_buf_sz bytes, so under the changelog's model it needs one more 4-byte
burst than the descriptor advertises and continues into the next
descriptor, where the legacy status handlers drop it:

drivers/net/ethernet/stmicro/stmmac/enh_desc.c:enh_desc_get_rx_status() {
	if (unlikely(!(rdes0 & RDES0_LAST_DESCRIPTOR))) {
		x->rx_length++;
		return discard_frame;
	}
}

drivers/net/ethernet/stmicro/stmmac/norm_desc.c:ndesc_get_rx_status() has
the same check.

Would it be better to subtract the offset misalignment from the buffer
length programmed into the descriptors (or add the NET_IP_ALIGN headroom
to the bfsize computation for non-xmac cores), so that dma_buf_sz keeps
matching what the hardware can actually store in one descriptor?

The patch also carries a Fixes: tag for 23680bf5f8c6, which invites a
stable backport.  Since the change adjusts only the selftest, does the
backport carry the tag without addressing the receive-side exposure?

One more consequence: __stmmac_test_jumbo() is the only in-tree probe of
the advertised single-descriptor maximum.  After this change, does a
future mismatch between the programmed buffer length and the real DMA
capacity stay undetected by the Jumbo test?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917215339.2022523-1-maxime.chevallier%40bootlin.com