[PATCH net v3 0/6] net: stmmac: More selftest-related fixes

[PATCH net v3 0/6] net: stmmac: More selftest-related fixes

[Maxime Chevallier]

Hi everyone,

This is V3 of stmmac selftest fixes, adding a cap on EEE test as the
timeout can get pretty long, found by Nicolai.

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.

I've continued expanding the test devices I'm running this on, more devices
should come in the future. With this series, _almost_ everything is
green, except for some flow control stuff that is either a false positive
or a real thing that needs investigating.

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.

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)

Tests are OK if return is 0 or -95 (-EOPNOTSUPP), tests are KO otherwise

Before the series :

Test                     imx  socfpga sun8i meson8b stm32 starV mcom ipq806x
MAC Loopback              0     0      0     0       0     0     0    -110
MMC Counters              0     0      -95   0       0     0     0    -110
EEE                       -95   -95    -95   -110    -110  -95   -95  -95
Hash Filter MC            0     0      -95   0       0     0     0    -95
Perfect Filter UC         -95   0      0     -95     -95   -95   -95  -95
MC Filter                 -95   0      -95   -95     -95   -95   -95  -95
UC Filter                 -95   0      -95   -95     -95   -95   -95  -95
Flow Control              -95   0      -110  0       -110  -95   -110 -110
RSS                       -95   -95    -95   -95     -95   -95   -95  -95
VLAN Filtering            -110  -95    -95   -95     -110  -110  -95  -95
VLAN Filtering (perf)     -110  -95    -95   -95     -110  -110  -95  -95
Double VLAN Filter        -110  -95    -95   -95     -110  -110  -95  -95
Double VLAN Filter (perf) -110  -95    -95   -95     -110  -110  -95  -95
Flexible RX Parser        0     -95    -95   -95     -95   -95   -95  -95
SA Insertion (desc)       0     -95    -95   -95     0     0     0    -95
SA Replacement (desc)     0     -95    -95   -95     0     0     0    -95
SA Insertion (reg         0     -95    -95   -95     0     0     0    -95
SA Replacement (reg)      0     -95    -95   -95     0     0     0    -95
VLAN TX Insertion         -110  -95    -95   -95     -110  -110  -110 -95
SVLAN TX Insertion        -110  -95    -95   -95     -110  -95   -110 -95
L3 DA Filtering           0     -95    -95   -95     -95   -95   -95  -95
L3 SA Filtering           0     -95    -95   -95     -95   -95   -95  -95
L4 DA TCP Filtering       0     -95    -95   -95     -95   -95   -95  -95
L4 SA TCP Filtering       0     -95    -95   -95     -95   -95   -95  -95
L4 DA UDP Filtering       0     -95    -95   -95     -95   -95   -95  -95
L4 SA UDP Filtering       0     -95    -95   -95     -95   -95   -95  -95
ARP Offload               -95   -95    -95   -95     -110  -110  -110 -95
Jumbo Frame               0     -110   -110  0       0     0     0    -110
Multichannel Jumbo        0     -95    -95   -95     -95   -95   -95  -95
Split Header             --95   -95    -95   -95     -95   -95   0    -95
TBS (ETF Scheduler)      --95   -95    -95   -95     -95   -95   -95  -95

ARP offload's still there as this was a net-next patch and I've ran these
checks on the net tree.

Jumbo frame tests on dwmac1000 started failing after :

commit 23680bf5f8c6 ("net: stmmac: restore NET_IP_ALIGN in the RX DMA offset")

This commit is OK though, it just made the selftest reveal the cracks
hiding beneath the surface of MTU/bufsz handling.

After this series :

Test                     imx  socfpga sun8i meson8b stm32 starV mcom ipq806x
MAC Loopback              0      0     0       0     0     0     0    0
MMC Counters              0      0     -95     0     0     0     0    0
EEE                       -95    -95   -95     0     0     -95   -95  -95
Hash Filter MC            0      0     -95     0     0     0     0    -95
Perfect Filter UC         -95    0     0       -95   -95   -95   -95  -95
MC Filter                 -95    0     -95     -95   -95   -95   -95  -95
UC Filter                 -95    0     -95     -95   -95   -95   -95  -95
Flow Control              -95    0     -110    0     -110  -95   -110 -110
RSS                       -95    -95   -95     -95   -95   -95   -95  -95
VLAN Filtering            0      -95   -95     -95   0     0     -95  -95
VLAN Filtering (perf)     0      -95   -95     -95   0     0     -95  -95
Double VLAN Filter        0      -95   -95     -95   0     0     -95  -95
Double VLAN Filter (perf) 0      -95   -95     -95   0     0     -95  -95
Flexible RX Parser        0      -95   -95     -95   -95   -95   -95  -95
SA Insertion (desc)       0      -95   -95     -95   0     0     0    -95
SA Replacement (desc)     0      -95   -95     -95   0     0     0    -95
SA Insertion (reg         0      -95   -95     -95   0     0     0    -95
SA Replacement (reg)      0      -95   -95     -95   0     0     0    -95
VLAN TX Insertion         0      -95   -95     -95   0     0     0    -95
SVLAN TX Insertion        -95    -95   -95     -95   -95   -95   -95  -95
L3 DA Filtering           0      -95   -95     -95   -95   -95   -95  -95
L3 SA Filtering           0      -95   -95     -95   -95   -95   -95  -95
L4 DA TCP Filtering       0      -95   -95     -95   -95   -95   -95  -95
L4 SA TCP Filtering       0      -95   -95     -95   -95   -95   -95  -95
L4 DA UDP Filtering       0      -95   -95     -95   -95   -95   -95  -95
L4 SA UDP Filtering       0      -95   -95     -95   -95   -95   -95  -95
ARP Offload               -95    -95   -95     -95   -110  -110  -110 -95
Jumbo Frame               0      0     0       0     0     0     0    0
Multichannel Jumbo        0      -95   -95     -95   -95   -95   -95  -95
Split Header              -95    -95   -95     -95   -95   -95   0    -95
TBS (ETF Scheduler)       -95    -95   -95     -95   -95   -95   -95  -95

ARP can be ignored, and Flow control needs some more attention, but
we're getting there :)

Thanks,

Maxime

V3:
 - 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 (6):
  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: size the RX buffers from the frame length, not the MTU
  net: stmmac: selftests: Account for alignment shift on dwmac1000 for
    Jumbo test

 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  20 +--
 .../stmicro/stmmac/stmmac_selftests.c         | 121 +++++++++++++++---
 2 files changed, 114 insertions(+), 27 deletions(-)

-- 
2.55.0

[PATCH net v3 1/6] 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.

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         | 68 ++++++++++++++++---
 1 file changed, 58 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
index 6372ec7c3f31..6503678b646e 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,7 +976,7 @@ static int __stmmac_test_vlanfilt(struct stmmac_priv *priv)
 	 * HASH values.
 	 */
 	tpriv->vlan_id = 0x123;
-	dev_add_pack(&tpriv->pt);
+	stmmac_sft_add_pack(&tpriv->pt);
 
 	ret = vlan_vid_add(priv->dev, htons(ETH_P_8021Q), tpriv->vlan_id);
 	if (ret)
@@ -968,7 +1016,7 @@ static int __stmmac_test_vlanfilt(struct stmmac_priv *priv)
 vlan_del:
 	vlan_vid_del(priv->dev, htons(ETH_P_8021Q), tpriv->vlan_id);
 cleanup:
-	dev_remove_pack(&tpriv->pt);
+	stmmac_sft_remove_pack(&tpriv->pt);
 	kfree(tpriv);
 	return ret;
 }
@@ -1022,7 +1070,7 @@ static int __stmmac_test_dvlanfilt(struct stmmac_priv *priv)
 	 * HASH values.
 	 */
 	tpriv->vlan_id = 0x123;
-	dev_add_pack(&tpriv->pt);
+	stmmac_sft_add_pack(&tpriv->pt);
 
 	ret = vlan_vid_add(priv->dev, htons(ETH_P_8021AD), tpriv->vlan_id);
 	if (ret)
@@ -1062,7 +1110,7 @@ static int __stmmac_test_dvlanfilt(struct stmmac_priv *priv)
 vlan_del:
 	vlan_vid_del(priv->dev, htons(ETH_P_8021AD), tpriv->vlan_id);
 cleanup:
-	dev_remove_pack(&tpriv->pt);
+	stmmac_sft_remove_pack(&tpriv->pt);
 	kfree(tpriv);
 	return ret;
 }
@@ -1293,7 +1341,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;
-	dev_add_pack(&tpriv->pt);
+	stmmac_sft_add_pack(&tpriv->pt);
 
 	ret = vlan_vid_add(priv->dev, htons(proto), tpriv->vlan_id);
 	if (ret)
@@ -1320,7 +1368,7 @@ static int stmmac_test_vlanoff_common(struct stmmac_priv *priv, bool svlan)
 vlan_del:
 	vlan_vid_del(priv->dev, htons(proto), tpriv->vlan_id);
 cleanup:
-	dev_remove_pack(&tpriv->pt);
+	stmmac_sft_remove_pack(&tpriv->pt);
 	kfree(tpriv);
 	return ret;
 }
-- 
2.55.0

[PATCH net v3 2/6] 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 6503678b646e..df28741abf2b 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

[PATCH net v3 3/6] 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 df28741abf2b..7f5837d1fed9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
@@ -1410,7 +1410,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

[PATCH net v3 4/6] 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.

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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
index 7f5837d1fed9..315c3cf82230 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,7 +345,7 @@ 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;
 
@@ -999,6 +1000,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
@@ -1093,6 +1095,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
@@ -1371,6 +1374,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;
 	stmmac_sft_add_pack(&tpriv->pt);
 
 	ret = vlan_vid_add(priv->dev, htons(proto), tpriv->vlan_id);
-- 
2.55.0

[PATCH net v3 5/6] 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 NET_IP_ALIGN

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 62c3441911e7..29b970574a38 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 + NET_IP_ALIGN;
+
+	/* 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 v3 6/6] 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 315c3cf82230..cd7dc99095a0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
@@ -1781,6 +1781,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 v3 1/6] net: stmmac: selftests: Support running selftests on DSA conduits

[Paolo Abeni]

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/
---
net: stmmac: selftests: Support running selftests on DSA conduits

The patch wraps the selftests' dev_add_pack()/dev_remove_pack() calls in
new stmmac_sft_add_pack()/stmmac_sft_remove_pack() helpers.  When the
interface is a DSA conduit, an ETH_P_ALL packet_type is registered instead
and a proxy handler, stmmac_sft_filter(), compares the ethernet header's
h_proto before forwarding a clone to the original validator.

A few questions below, one on the commit message and three on the code.

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

[Severity: Medium]
Is this description accurate?  ARP offload does look implemented in two
places:

drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:
	static void dwmac4_set_arp_offload(struct mac_device_info *hw, bool en,
					   u32 addr)

drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:
	static void dwxgmac2_set_arp_offload(struct mac_device_info *hw, bool en,
					     u32 addr)

dwmac4_set_arp_offload() is wired into dwmac4_ops, dwmac410_ops and
dwmac510_ops, and dwxgmac2_set_arp_offload() into dwxgmac210_ops and
dwxlgmac2_ops.

stmmac_test_arpoffload() only bails out early with:

	if (!priv->dma_cap.arpoffsel)
		return -EOPNOTSUPP;

so on those cores with arpoffsel set the test really runs, and it is still
listed in stmmac_selftests as "ARP Offload".  Since it keeps the raw
dev_add_pack(&tpriv->pt) with pt.type = htons(ETH_P_ARP), does it still
report a spurious failure in ethtool -t on a DSA conduit, for the same
ETH_P_XDSA reason this patch fixes elsewhere?  Would switching that pair to
stmmac_sft_add_pack()/stmmac_sft_remove_pack() cover it as well?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> index 6372ec7c3f317..6503678b646ef 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c

[ ... ]

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

[Severity: Medium]
This compares the on-wire ethertype against the value the test registered
in pt.type, but for the VLAN tests pt.type is the protocol *after* the tag
has been removed:

__stmmac_test_vlanfilt()      pt.type = htons(ETH_P_IP), frame is 802.1Q
__stmmac_test_dvlanfilt()     pt.type = htons(ETH_P_8021Q), outer tag is
                              ETH_P_8021AD
stmmac_test_vlanoff_common()  pt.type = ETH_P_IP or ETH_P_8021Q

On a normal interface the core removes the tag before the taps run:

net/core/dev.c:__netif_receive_skb_core() {
	...
	if (eth_type_vlan(skb->protocol)) {
		skb = skb_vlan_untag(skb);
	...
	list_for_each_entry_rcu(ptype, &skb->dev->ptype_all, list) {
	...
}

but on a DSA conduit skb->protocol is ETH_P_XDSA, so eth_type_vlan() is
false and no untagging happens there.  Doesn't that leave the driver as the
only tag remover, and isn't that conditional?

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_rx_vlan() {
	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)) {
	...
}

NETIF_F_HW_VLAN_CTAG_RX is placed in hw_features on xmac cores, so it is
user-toggleable.  After "ethtool -K <conduit> rxvlan off", or with
CONFIG_VLAN_8021Q disabled where neither feature nor hw_vlan_en is ever
set, the frame should reach the filter with h_proto == 0x8100, the
comparison against ETH_P_IP fails, the frame is freed here and the C-VLAN
tests time out.  Would matching on the inner protocol (or skipping any
VLAN tags before the comparison) be more robust?

> +
> +	kfree_skb(skb);
> +	return ret;
> +}

[Severity: Medium]
This isn't a bug introduced by this patch, but the validators reached from
here derive header offsets from the received IPv4 ihl field while only
checking for 33 linear bytes:

stmmac_test_loopback_validate() {
	...
	if (skb_headlen(skb) < (STMMAC_TEST_PKT_SIZE - ETH_HLEN))
		goto out;
	...
	uhdr = (struct udphdr *)((u8 *)ihdr + 4 * ihdr->ihl);
	...
	shdr = (struct stmmachdr *)((u8 *)uhdr + sizeof(*uhdr));
	...
	if (shdr->magic != cpu_to_be64(STMMAC_TEST_PKT_MAGIC))
	...
}

With ihl == 15 the shdr->magic read lands roughly 90 bytes past the checked
bound.  The same pattern exists in stmmac_test_vlan_validate().  This was
already reachable before the patch through the ETH_P_IP ptype registration
on priv->dev, and the read stays inside the RX buffer, so the practical
effect is stale bytes and possibly a wrong verdict.  Since the new
ETH_P_ALL tap widens the set of frames that get here, would it be
reasonable to add a length check on the derived offsets along with it?

> +
> +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)
>  {

[ ... ]

> @@ -928,7 +976,7 @@ static int __stmmac_test_vlanfilt(struct stmmac_priv *priv)
>  	 * HASH values.
>  	 */
>  	tpriv->vlan_id = 0x123;
> -	dev_add_pack(&tpriv->pt);
> +	stmmac_sft_add_pack(&tpriv->pt);
>  
>  	ret = vlan_vid_add(priv->dev, htons(ETH_P_8021Q), tpriv->vlan_id);
>  	if (ret)

[Severity: High]
Can an incoming frame from the switch oops here?  The handler is now
installed as an ETH_P_ALL tap while attr.dst is still NULL:

__stmmac_test_vlanfilt() {
	struct stmmac_packet_attrs attr = { };
	...
	tpriv->packet = &attr;
	tpriv->vlan_id = 0x123;
	stmmac_sft_add_pack(&tpriv->pt);

	ret = vlan_vid_add(priv->dev, htons(ETH_P_8021Q), tpriv->vlan_id);
	...
	for (i = 0; i < 4; i++) {
		...
		attr.dst = priv->dev->dev_addr;

and the VLAN validator dereferences packet->dst without a NULL check,
unlike stmmac_test_loopback_validate() which guards it with "if (dst)":

stmmac_test_vlan_validate() {
	...
	if (tpriv->vlan_id) {
		if (skb->vlan_proto != htons(proto))
			goto out;
		if (skb->vlan_tci != tpriv->vlan_id) {
	...
	ehdr = (struct ethhdr *)skb_mac_header(skb);
	if (!ether_addr_equal_unaligned(ehdr->h_dest, tpriv->packet->dst))
	...
}

The ETH_P_ALL tap runs before the core's __vlan_hwaccel_clear_tag(), so a
received IPv4 frame that stmmac_rx_vlan() stripped into the hwaccel fields
with vlan_proto == ETH_P_8021Q and vlan_tci == 0x123 passes the h_proto
check in stmmac_sft_filter(), passes the vlan_proto/vlan_tci checks, and
then calls ether_addr_equal_unaligned(ehdr->h_dest, NULL).

Before the patch the core cleared the hwaccel tag before ETH_P_IP ptype
dispatch, so foreign frames were rejected by the vlan_proto check and this
window was not reachable from the wire.  Would setting attr.dst before
stmmac_sft_add_pack(), or adding a NULL guard in the validator, close it?

> @@ -1022,7 +1070,7 @@ static int __stmmac_test_dvlanfilt(struct stmmac_priv *priv)
>  	 * HASH values.
>  	 */
>  	tpriv->vlan_id = 0x123;
> -	dev_add_pack(&tpriv->pt);
> +	stmmac_sft_add_pack(&tpriv->pt);
>  
>  	ret = vlan_vid_add(priv->dev, htons(ETH_P_8021AD), tpriv->vlan_id);
>  	if (ret)

[ ... ]

> @@ -1293,7 +1341,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;
> -	dev_add_pack(&tpriv->pt);
> +	stmmac_sft_add_pack(&tpriv->pt);
>  
>  	ret = vlan_vid_add(priv->dev, htons(proto), tpriv->vlan_id);
>  	if (ret)

[Severity: High]
Same ordering as __stmmac_test_vlanfilt() here: the tap is registered, then
vlan_vid_add() runs, and only afterwards does the function do
"attr.dst = priv->dev->dev_addr;", so tpriv->packet->dst is NULL for the
whole window.

[ ... ]
-- 
This is an AI-generated review.

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

[Paolo Abeni]

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/
---
net: stmmac: selftests: Capture all packets for vlan checks

The VLAN selftests use vlan_vid_add() without a VLAN netdev, so the test
frame is marked PACKET_OTHERHOST and reaches the handler with a cleared
skb->vlan_tci.  This patch reuses the DSA proxy mechanism to register an
ETH_P_ALL handler for the three VLAN tests so validation sees the tag.

A few questions about the new capture_all path below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> index 7f5837d1fed94..315c3cf82230e 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,7 +345,7 @@ 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.
>  		 */

[Severity: Low]
This isn't a bug, but the comment now only describes one of the two reasons
for taking this branch.  For the three VLAN tests pt.type is ETH_P_IP or
ETH_P_8021Q, which would match at ptype_specific just fine, so the
"DSA conduit will report ETH_P_XDSA" rationale does not apply there.

Could the comment also mention the capture_all reason (the tap has to run
at ptype_all so skb->vlan_tci is still populated), and could the new
capture_all field get a one-line comment stating that it must be set before
stmmac_sft_add_pack() is called?

>  		tpriv->pt.type = htons(ETH_P_ALL);
>  		tpriv->pt.func = stmmac_sft_filter;
>  		tpriv->pt.ignore_outgoing = true;
>  	}

[Severity: Medium]
With pt.type set to ETH_P_ALL on priv->dev, stmmac_test_vlan_validate() now
sees every ingress frame, and it declares the test failed before it has
established that the frame is a test frame:

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

	ehdr = (struct ethhdr *)skb_mac_header(skb);
	if (!ether_addr_equal_unaligned(ehdr->h_dest, tpriv->packet->dst))
		goto out;

The h_dest and magic checks come after this.  Can any ordinary C-tagged
IPv4 frame on the link (any VID other than 0x123, at least 33 bytes of
linear data) now abort the VLAN Filter, Double VLAN Filter and VLAN Offload
tests with a spurious failure for i == 0?  Before this patch the tag was
cleared for such frames, so the vlan_proto check bailed out first.

Would it make sense to move the vlan_tci comparison after the destination
MAC and magic checks so only test frames can set the verdict?

[Severity: Low]
This isn't a bug introduced by this patch, but the ETH_P_ALL tap widens the
set of frames that reach the header parsing in stmmac_test_vlan_validate(),
which derives offsets from the untrusted IPv4 ihl with only a fixed length
check:

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

	shdr = (struct stmmachdr *)((u8 *)uhdr + sizeof(*uhdr));

That check covers 33 bytes, which does not include the UDP header, the
extra +4 double-VLAN offset, or an ihl larger than 5.  A frame with ihl = 15
makes uhdr->dest be read well past the validated area (and past skb->len for
a small frame).  The same arithmetic exists in
stmmac_test_loopback_validate(), so this predates the patch, but would a
pskb_may_pull() style length check based on the actual ihl be worth adding
here?

> @@ -999,6 +1000,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]
This is a pre-existing issue, but the patch makes it observable.  The
"Means filter did not work" path in stmmac_test_vlan_validate() reports
failure through the same tpriv->ok = false state that a timeout produces,
and the loop here treats that state as the expected outcome for i > 0:

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

So a frame admitted with a non-configured VID (0x124..0x126) is
indistinguishable from a correctly dropped frame, and the test still passes
even if the hardware admits every VID.  Before this patch the mismatch
branch was unreachable because skb->vlan_tci was always cleared; now it is
reachable but its verdict is discarded.  Could the return value of
wait_for_completion_timeout() be checked, or a separate "packet was
admitted" flag be used, so the two cases can be told apart?  The same
pattern is in __stmmac_test_dvlanfilt().

[Severity: Medium]
Also pre-existing rather than introduced here: tpriv->ok, tpriv->comp and
the on-stack attr are shared between the softirq validator and this loop
with no serialization, and init_completion() is only called once, outside
the loop.  With capture_all, an extra complete() is now reachable from the
"filter did not work" path for any tagged frame with a different VID.

Since complete() increments comp.done and wait_for_completion_timeout()
consumes only one, does a surplus completion in one iteration make the next
wait_for_completion_timeout() return immediately and take the verdict from a
stale tpriv->ok?  Would a reinit_completion() at the top of each iteration
help here?

> @@ -1093,6 +1095,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: High]
Does this actually fix the Double VLAN Filter test on the cores that can run
it?  The test requires priv->dma_cap.vlhash, i.e. the xmac cores, where
probe sets priv->hw->hw_vlan_en = true and stmmac_hw_setup() programs
VLAN_TAG_STRIP_ALL together with VLAN_TAG_CTRL_EVLRXS, while
vlan_vid_add(ETH_P_8021AD, ...) enables S-VLAN recognition.

The MAC-stripped outer tag is then reported by vlan_rx_hw() in
stmmac_vlan.c:

	__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vid);

which hardcodes ETH_P_8021Q, while the validator with double_vlan set wants
ETH_P_8021AD:

	proto = tpriv->double_vlan ? ETH_P_8021AD : ETH_P_8021Q;
	...
		if (skb->vlan_proto != htons(proto))
			goto out;

So the frame does reach the validator now, but the vlan_proto check takes
"goto out" without calling complete(), and the test still returns -ETIMEDOUT
for i == 0 on working hardware unless RX VLAN stripping is turned off.  Is
the intent that vlan_rx_hw() report the real tag protocol, or that the
validator accept ETH_P_8021Q for the stripped outer tag?

> @@ -1371,6 +1374,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;
>  	stmmac_sft_add_pack(&tpriv->pt);
>  
>  	ret = vlan_vid_add(priv->dev, htons(proto), tpriv->vlan_id);

[Severity: High]
Can this dereference a NULL pointer in softirq context?  attr is
zero-initialized, so attr.dst is NULL, and the ETH_P_ALL handler is
published by stmmac_sft_add_pack() before attr.dst is assigned:

	tpriv->capture_all = true;
	stmmac_sft_add_pack(&tpriv->pt);

	ret = vlan_vid_add(priv->dev, htons(proto), tpriv->vlan_id);
	if (ret)
		goto cleanup;

	attr.dst = priv->dev->dev_addr;

vlan_vid_add() is not instantaneous here: it does
pm_runtime_resume_and_get() and a register poll in vlan_write_filter().  A
frame arriving in that window with vlan_proto ETH_P_8021Q and vlan_tci equal
to the hard-coded 0x123 passes the tag checks and falls into:

	ehdr = (struct ethhdr *)skb_mac_header(skb);
	if (!ether_addr_equal_unaligned(ehdr->h_dest, tpriv->packet->dst))
		goto out;

which reads 6 bytes from address 0.  stmmac_test_loopback_validate() guards
the same comparison with "if (dst)", so the pointer does not look like an
invariant.  The same ordering exists in __stmmac_test_vlanfilt() and
__stmmac_test_dvlanfilt(), where attr.dst is only set inside the loop.
Would assigning attr.dst before stmmac_sft_add_pack(), or adding the NULL
check to this validator, close that window?
-- 
This is an AI-generated review.

Re: [PATCH net v3 5/6] net: stmmac: size the RX buffers from the frame length, not the MTU

[Paolo Abeni]

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/
---
net: stmmac: size the RX buffers from the frame length, not the MTU

The patch stops feeding the bare MTU into the RX buffer size selection and
instead computes a frame length that includes the L2 header, two VLAN tags,
the FCS and NET_IP_ALIGN.  stmmac_set_bfsize() is renamed to take a length
and its boundary comparisons are switched from '>=' to '>'.

A few questions below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 62c3441911e71..29b970574a380 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;

[Severity: High]
This isn't a bug introduced by this patch, but the 16 KiB selection here is
reachable in configurations whose descriptors cannot express a buffer that
large, and this patch moves the threshold about 28 bytes of MTU lower.

In chain mode mac->mode points at chain_mode_ops, which does not implement
set_16kib_bfsize, so stmmac_do_callback() returns -EINVAL, bfsize is forced
to 0 and stmmac_set_bfsize(len) runs unconditionally.  For enhanced
descriptors the programmed buffer 1 is clamped:

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

and for normal descriptors:

drivers/net/ethernet/stmicro/stmmac/norm_desc.c:ndesc_init_rx_desc() {
	bfsize1 = min(bfsize, BUF_SIZE_2KiB - 1);
	p->des1 |= cpu_to_le32(bfsize1 & RDES1_BUFFER1_SIZE_MASK);
}

while the RX path still reports the software value for every non-last
descriptor:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_rx_buf1_len() {
	/* First descriptor, not last descriptor and not split header */
	if (status & rx_not_ls)
		return priv->dma_conf.dma_buf_sz;
}

That value then drives dma_sync_single_range_for_cpu() and
skb_add_rx_frag().  Can this hand the stack more bytes than the engine
wrote, padding the frame with recycled page pool contents?  Chain mode is
reachable through the chain_mode module parameter and is forced for
dwmac-sun8i, which uses normal descriptors.

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

[Severity: High]
This is a pre-existing issue and not something this patch introduces, but
the new thresholds widen the MTU range in which it applies (the step up to
BUF_SIZE_2KiB/BUF_SIZE_4KiB now happens roughly 28 bytes of MTU earlier).

For normal descriptors in ring mode, a second data buffer is advertised
whose bus address is never programmed:

drivers/net/ethernet/stmicro/stmmac/descs_com.h:ndesc_rx_set_on_ring() {
	if (bfsize >= BUF_SIZE_2KiB) {
		int bfsize2;

		bfsize2 = min(bfsize - BUF_SIZE_2KiB + 1, BUF_SIZE_2KiB - 1);
		p->des1 |= cpu_to_le32(FIELD_PREP(RDES1_BUFFER2_SIZE_MASK,
						  bfsize2));
	}
}

RDES3 is only ever written through ring_mode.c init_desc3()/refill_desc3(),
and the driver only calls those when dma_buf_sz equals BUF_SIZE_16KiB:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_init_rx_buffers() {
	if (dma_conf->dma_buf_sz == BUF_SIZE_16KiB)
		stmmac_init_desc3(priv, p);
}

Devices on ndesc_ops are capped at max_mtu = SKB_MAX_HEAD(NET_SKB_PAD +
NET_IP_ALIGN), so they can only pick 2048 or 4096 and RDES3 stays 0 from
dma_alloc_coherent().  Since the second-address-chained bit is not set in
ring mode, does the RX DMA spill the part of the frame beyond the 2047-byte
buffer 1 to bus address 0?  This is driven by an inbound frame on cores
using normal descriptors (MAC100, or GMAC with plat->enh_desc == 0) with an
MTU above ~2 KiB.

>  	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 + NET_IP_ALIGN;

[Severity: Low]
Should NET_IP_ALIGN be part of this sum?  It is head room that lives
outside the window described by dma_buf_sz, and it is already reserved
separately:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:__alloc_dma_rx_desc_resources() {
	dma_buf_sz_pad = stmmac_rx_offset(priv) + dma_conf->dma_buf_sz +
			 SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
	...
	pp_params.offset = stmmac_rx_offset(priv);
	pp_params.max_len = dma_conf->dma_buf_sz;
}

with stmmac_rx_offset() returning NET_SKB_PAD + NET_IP_ALIGN, and
stmmac_init_rx_buffers() programming the descriptor at page dma address
plus buf->page_offset == stmmac_rx_offset(priv).

Counting it again inside len makes the selected buffer class, and therefore
the page pool order, depend on the architecture for a 2-byte wide MTU
window at each boundary.  For example MTU 4069/4070 with NET_IP_ALIGN == 2
gives len 4097/4098 and selects BUF_SIZE_8KiB, whereas the same MTU on
x86 (NET_IP_ALIGN == 0) selects BUF_SIZE_4KiB.

The commit message also lists "the NET_IP_ALIGN" alongside ETH_HLEN, the
VLAN tags and the FCS as if it were part of the frame.  Would it be more
accurate to describe it as head room, or drop the term?

> +
> +	/* 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);

[Severity: Medium]
The argument handed to the set_16kib_bfsize callback changes unit here from
MTU to frame length, but neither the ops prototype nor the two
implementations were updated.

hwif.h still declares:

	int (*set_16kib_bfsize)(int mtu);

dwmac4_descs.c:set_16kib_bfsize() keeps the inclusive test:

	if (unlikely(mtu >= BUF_SIZE_8KiB))
		ret = BUF_SIZE_16KiB;

while ring_mode.c:set_16kib_bfsize() uses the other operator:

	if (unlikely(mtu > BUF_SIZE_8KiB))
		ret = BUF_SIZE_16KiB;

Since dwmac4/dwmac5 use dwmac4_ring_mode_ops, does the new comment "Returns
0 or BUF_SIZE_16KiB if len > 8KiB" match what actually happens on those
cores?  A len of exactly BUF_SIZE_8KiB (MTU 8160 with NET_IP_ALIGN == 2)
fits an 8188-byte buffer and stmmac_set_bfsize() would return
BUF_SIZE_8KiB, but the unchanged '>=' returns BUF_SIZE_16KiB and the
'if (bfsize < BUF_SIZE_16KiB)' guard then skips the corrected helper, so
dma_buf_sz becomes 16368 instead of 8188.  That roughly doubles the pages
backing each RX buffer (order 2 to order 3) across the default
dma_rx_size of 512.

The commit message says the change lets us "use more consistent comparison
operators in stmmac_set_bfsize" - should the two set_16kib_bfsize
implementations and the 'mtu' parameter name be converted at the same time?
-- 
This is an AI-generated review.

Re: [PATCH net v3 0/6] net: stmmac: More selftest-related fixes

[Paolo Abeni]

On 9/11/26 11:20 PM, Maxime Chevallier wrote:
> Hi everyone,
> 
> This is V3 of stmmac selftest fixes, adding a cap on EEE test as the
> timeout can get pretty long, found by Nicolai.
> 
> 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.
> 
> I've continued expanding the test devices I'm running this on, more devices
> should come in the future. With this series, _almost_ everything is
> green, except for some flow control stuff that is either a false positive
> or a real thing that needs investigating.
> 
> 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.
> 
> 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)
> 
> Tests are OK if return is 0 or -95 (-EOPNOTSUPP), tests are KO otherwise
> 
> Before the series :
> 
> Test                     imx  socfpga sun8i meson8b stm32 starV mcom ipq806x
> MAC Loopback              0     0      0     0       0     0     0    -110
> MMC Counters              0     0      -95   0       0     0     0    -110
> EEE                       -95   -95    -95   -110    -110  -95   -95  -95
> Hash Filter MC            0     0      -95   0       0     0     0    -95
> Perfect Filter UC         -95   0      0     -95     -95   -95   -95  -95
> MC Filter                 -95   0      -95   -95     -95   -95   -95  -95
> UC Filter                 -95   0      -95   -95     -95   -95   -95  -95
> Flow Control              -95   0      -110  0       -110  -95   -110 -110
> RSS                       -95   -95    -95   -95     -95   -95   -95  -95
> VLAN Filtering            -110  -95    -95   -95     -110  -110  -95  -95
> VLAN Filtering (perf)     -110  -95    -95   -95     -110  -110  -95  -95
> Double VLAN Filter        -110  -95    -95   -95     -110  -110  -95  -95
> Double VLAN Filter (perf) -110  -95    -95   -95     -110  -110  -95  -95
> Flexible RX Parser        0     -95    -95   -95     -95   -95   -95  -95
> SA Insertion (desc)       0     -95    -95   -95     0     0     0    -95
> SA Replacement (desc)     0     -95    -95   -95     0     0     0    -95
> SA Insertion (reg         0     -95    -95   -95     0     0     0    -95
> SA Replacement (reg)      0     -95    -95   -95     0     0     0    -95
> VLAN TX Insertion         -110  -95    -95   -95     -110  -110  -110 -95
> SVLAN TX Insertion        -110  -95    -95   -95     -110  -95   -110 -95
> L3 DA Filtering           0     -95    -95   -95     -95   -95   -95  -95
> L3 SA Filtering           0     -95    -95   -95     -95   -95   -95  -95
> L4 DA TCP Filtering       0     -95    -95   -95     -95   -95   -95  -95
> L4 SA TCP Filtering       0     -95    -95   -95     -95   -95   -95  -95
> L4 DA UDP Filtering       0     -95    -95   -95     -95   -95   -95  -95
> L4 SA UDP Filtering       0     -95    -95   -95     -95   -95   -95  -95
> ARP Offload               -95   -95    -95   -95     -110  -110  -110 -95
> Jumbo Frame               0     -110   -110  0       0     0     0    -110
> Multichannel Jumbo        0     -95    -95   -95     -95   -95   -95  -95
> Split Header             --95   -95    -95   -95     -95   -95   0    -95
> TBS (ETF Scheduler)      --95   -95    -95   -95     -95   -95   -95  -95
> 
> ARP offload's still there as this was a net-next patch and I've ran these
> checks on the net tree.
> 
> Jumbo frame tests on dwmac1000 started failing after :
> 
> commit 23680bf5f8c6 ("net: stmmac: restore NET_IP_ALIGN in the RX DMA offset")
> 
> This commit is OK though, it just made the selftest reveal the cracks
> hiding beneath the surface of MTU/bufsz handling.
> 
> After this series :
> 
> Test                     imx  socfpga sun8i meson8b stm32 starV mcom ipq806x
> MAC Loopback              0      0     0       0     0     0     0    0
> MMC Counters              0      0     -95     0     0     0     0    0
> EEE                       -95    -95   -95     0     0     -95   -95  -95
> Hash Filter MC            0      0     -95     0     0     0     0    -95
> Perfect Filter UC         -95    0     0       -95   -95   -95   -95  -95
> MC Filter                 -95    0     -95     -95   -95   -95   -95  -95
> UC Filter                 -95    0     -95     -95   -95   -95   -95  -95
> Flow Control              -95    0     -110    0     -110  -95   -110 -110
> RSS                       -95    -95   -95     -95   -95   -95   -95  -95
> VLAN Filtering            0      -95   -95     -95   0     0     -95  -95
> VLAN Filtering (perf)     0      -95   -95     -95   0     0     -95  -95
> Double VLAN Filter        0      -95   -95     -95   0     0     -95  -95
> Double VLAN Filter (perf) 0      -95   -95     -95   0     0     -95  -95
> Flexible RX Parser        0      -95   -95     -95   -95   -95   -95  -95
> SA Insertion (desc)       0      -95   -95     -95   0     0     0    -95
> SA Replacement (desc)     0      -95   -95     -95   0     0     0    -95
> SA Insertion (reg         0      -95   -95     -95   0     0     0    -95
> SA Replacement (reg)      0      -95   -95     -95   0     0     0    -95
> VLAN TX Insertion         0      -95   -95     -95   0     0     0    -95
> SVLAN TX Insertion        -95    -95   -95     -95   -95   -95   -95  -95
> L3 DA Filtering           0      -95   -95     -95   -95   -95   -95  -95
> L3 SA Filtering           0      -95   -95     -95   -95   -95   -95  -95
> L4 DA TCP Filtering       0      -95   -95     -95   -95   -95   -95  -95
> L4 SA TCP Filtering       0      -95   -95     -95   -95   -95   -95  -95
> L4 DA UDP Filtering       0      -95   -95     -95   -95   -95   -95  -95
> L4 SA UDP Filtering       0      -95   -95     -95   -95   -95   -95  -95
> ARP Offload               -95    -95   -95     -95   -110  -110  -110 -95
> Jumbo Frame               0      0     0       0     0     0     0    0
> Multichannel Jumbo        0      -95   -95     -95   -95   -95   -95  -95
> Split Header              -95    -95   -95     -95   -95   -95   0    -95
> TBS (ETF Scheduler)       -95    -95   -95     -95   -95   -95   -95  -95
> 
> ARP can be ignored, and Flow control needs some more attention, but
> we're getting there :)
> 
> Thanks,
> 
> Maxime
> 
> V3:
>  - 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 (6):
>   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: size the RX buffers from the frame length, not the MTU
>   net: stmmac: selftests: Account for alignment shift on dwmac1000 for
>     Jumbo test
> 
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c |  20 +--
>  .../stmicro/stmmac/stmmac_selftests.c         | 121 +++++++++++++++---
>  2 files changed, 114 insertions(+), 27 deletions(-)

It looks like that the possible ETIMEDOUT reported on patch 4/6 is
contradicted by your testing, but the other not pre-existing 'high prio'
findings from clashiko look actionable to me.

/P

Re: [PATCH net v3 1/6] net: stmmac: selftests: Support running selftests on DSA conduits

[Maxime Chevallier]

On 9/15/26 10:45, Paolo Abeni wrote:
> 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/
> ---
> net: stmmac: selftests: Support running selftests on DSA conduits
> 
> The patch wraps the selftests' dev_add_pack()/dev_remove_pack() calls in
> new stmmac_sft_add_pack()/stmmac_sft_remove_pack() helpers.  When the
> interface is a DSA conduit, an ETH_P_ALL packet_type is registered instead
> and a proxy handler, stmmac_sft_filter(), compares the ethernet header's
> h_proto before forwarding a clone to the original validator.
> 
> A few questions below, one on the commit message and three on the code.
> 
>> The ARP offload's packet handler is left alone, this feature is just not
>> implemented in stmmac and due for removal.
> 
> [Severity: Medium]
> Is this description accurate?  ARP offload does look implemented in two
> places:

ARM offload is gone on net-next, it has never been implemented properly as
only half the machinery is there : the feature can be enabled/disabled, but
nothing ever programmed the IPv4 address :

https://lore.kernel.org/netdev/20260831071351.354255-1-maxime.chevallier@bootlin.com/

The ethtool selftests don't really bring anything, and were failing on all
platforms that support this, I don't think it's worth backporting ARP offload
removal or the stmmac_sft_add_pack machinery :(

> 
> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:
> 	static void dwmac4_set_arp_offload(struct mac_device_info *hw, bool en,
> 					   u32 addr)
> 
> drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:
> 	static void dwxgmac2_set_arp_offload(struct mac_device_info *hw, bool en,
> 					     u32 addr)
> 
> dwmac4_set_arp_offload() is wired into dwmac4_ops, dwmac410_ops and
> dwmac510_ops, and dwxgmac2_set_arp_offload() into dwxgmac210_ops and
> dwxlgmac2_ops.
> 
> stmmac_test_arpoffload() only bails out early with:
> 
> 	if (!priv->dma_cap.arpoffsel)
> 		return -EOPNOTSUPP;
> 
> so on those cores with arpoffsel set the test really runs, and it is still
> listed in stmmac_selftests as "ARP Offload".  Since it keeps the raw
> dev_add_pack(&tpriv->pt) with pt.type = htons(ETH_P_ARP), does it still
> report a spurious failure in ethtool -t on a DSA conduit, for the same
> ETH_P_XDSA reason this patch fixes elsewhere?  Would switching that pair to
> stmmac_sft_add_pack()/stmmac_sft_remove_pack() cover it as well?
> 
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
>> index 6372ec7c3f317..6503678b646ef 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> 
> [ ... ]
> 
>> @@ -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);
>> +	}
> 
> [Severity: Medium]
> This compares the on-wire ethertype against the value the test registered
> in pt.type, but for the VLAN tests pt.type is the protocol *after* the tag
> has been removed:
> 
> __stmmac_test_vlanfilt()      pt.type = htons(ETH_P_IP), frame is 802.1Q
> __stmmac_test_dvlanfilt()     pt.type = htons(ETH_P_8021Q), outer tag is
>                               ETH_P_8021AD
> stmmac_test_vlanoff_common()  pt.type = ETH_P_IP or ETH_P_8021Q
> 
> On a normal interface the core removes the tag before the taps run:
> 
> net/core/dev.c:__netif_receive_skb_core() {
> 	...
> 	if (eth_type_vlan(skb->protocol)) {
> 		skb = skb_vlan_untag(skb);
> 	...
> 	list_for_each_entry_rcu(ptype, &skb->dev->ptype_all, list) {
> 	...
> }
> 
> but on a DSA conduit skb->protocol is ETH_P_XDSA, so eth_type_vlan() is
> false and no untagging happens there.  Doesn't that leave the driver as the
> only tag remover, and isn't that conditional?

I'm failing to understand that sentence here :( Does this have to do with
the fact that stmmac has a SW tag stripping fallback ? if so, Peilin He is
working on it :)

> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_rx_vlan() {
> 	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)) {
> 	...
> }
> 
> NETIF_F_HW_VLAN_CTAG_RX is placed in hw_features on xmac cores, so it is
> user-toggleable.  After "ethtool -K <conduit> rxvlan off", or with
> CONFIG_VLAN_8021Q disabled where neither feature nor hw_vlan_en is ever
> set, the frame should reach the filter with h_proto == 0x8100, the
> comparison against ETH_P_IP fails, the frame is freed here and the C-VLAN
> tests time out.  Would matching on the inner protocol (or skipping any
> VLAN tags before the comparison) be more robust?

That's being reworked to some extent by Ovidiu and Peilin HE, this is something
I can work on as a follow up once things settle on the VLAN front.

> 
>> +
>> +	kfree_skb(skb);
>> +	return ret;
>> +}
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but the validators reached from
> here derive header offsets from the received IPv4 ihl field while only
> checking for 33 linear bytes:
> 
> stmmac_test_loopback_validate() {
> 	...
> 	if (skb_headlen(skb) < (STMMAC_TEST_PKT_SIZE - ETH_HLEN))
> 		goto out;
> 	...
> 	uhdr = (struct udphdr *)((u8 *)ihdr + 4 * ihdr->ihl);
> 	...
> 	shdr = (struct stmmachdr *)((u8 *)uhdr + sizeof(*uhdr));
> 	...
> 	if (shdr->magic != cpu_to_be64(STMMAC_TEST_PKT_MAGIC))
> 	...
> }
> 
> With ihl == 15 the shdr->magic read lands roughly 90 bytes past the checked
> bound.  The same pattern exists in stmmac_test_vlan_validate().  This was
> already reachable before the patch through the ETH_P_IP ptype registration
> on priv->dev, and the read stays inside the RX buffer, so the practical
> effect is stale bytes and possibly a wrong verdict.  Since the new
> ETH_P_ALL tap widens the set of frames that get here, would it be
> reasonable to add a length check on the derived offsets along with it?

Not introduced by this patch, I'm ok with sending a followup.

> 
>> +
>> +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)
>>  {
> 
> [ ... ]
> 
>> @@ -928,7 +976,7 @@ static int __stmmac_test_vlanfilt(struct stmmac_priv *priv)
>>  	 * HASH values.
>>  	 */
>>  	tpriv->vlan_id = 0x123;
>> -	dev_add_pack(&tpriv->pt);
>> +	stmmac_sft_add_pack(&tpriv->pt);
>>  
>>  	ret = vlan_vid_add(priv->dev, htons(ETH_P_8021Q), tpriv->vlan_id);
>>  	if (ret)
> 
> [Severity: High]
> Can an incoming frame from the switch oops here?  The handler is now
> installed as an ETH_P_ALL tap while attr.dst is still NULL:
> 
> __stmmac_test_vlanfilt() {
> 	struct stmmac_packet_attrs attr = { };
> 	...
> 	tpriv->packet = &attr;
> 	tpriv->vlan_id = 0x123;
> 	stmmac_sft_add_pack(&tpriv->pt);
> 
> 	ret = vlan_vid_add(priv->dev, htons(ETH_P_8021Q), tpriv->vlan_id);
> 	...
> 	for (i = 0; i < 4; i++) {
> 		...
> 		attr.dst = priv->dev->dev_addr;
> 
> and the VLAN validator dereferences packet->dst without a NULL check,
> unlike stmmac_test_loopback_validate() which guards it with "if (dst)":
> 
> stmmac_test_vlan_validate() {
> 	...
> 	if (tpriv->vlan_id) {
> 		if (skb->vlan_proto != htons(proto))
> 			goto out;
> 		if (skb->vlan_tci != tpriv->vlan_id) {
> 	...
> 	ehdr = (struct ethhdr *)skb_mac_header(skb);
> 	if (!ether_addr_equal_unaligned(ehdr->h_dest, tpriv->packet->dst))
> 	...
> }
> 
> The ETH_P_ALL tap runs before the core's __vlan_hwaccel_clear_tag(), so a
> received IPv4 frame that stmmac_rx_vlan() stripped into the hwaccel fields
> with vlan_proto == ETH_P_8021Q and vlan_tci == 0x123 passes the h_proto
> check in stmmac_sft_filter(), passes the vlan_proto/vlan_tci checks, and
> then calls ether_addr_equal_unaligned(ehdr->h_dest, NULL).
> 
> Before the patch the core cleared the hwaccel tag before ETH_P_IP ptype
> dispatch, so foreign frames were rejected by the vlan_proto check and this
> window was not reachable from the wire.  Would setting attr.dst before
> stmmac_sft_add_pack(), or adding a NULL guard in the validator, close it?

Hm this is a real issue, but not critical IMO as, well, nobody seems to be
running these selftests, and even less so with DSA. I can send a V4 though,
or work on that as a followup.

> 
>> @@ -1022,7 +1070,7 @@ static int __stmmac_test_dvlanfilt(struct stmmac_priv *priv)
>>  	 * HASH values.
>>  	 */
>>  	tpriv->vlan_id = 0x123;
>> -	dev_add_pack(&tpriv->pt);
>> +	stmmac_sft_add_pack(&tpriv->pt);
>>  
>>  	ret = vlan_vid_add(priv->dev, htons(ETH_P_8021AD), tpriv->vlan_id);
>>  	if (ret)
> 
> [ ... ]
> 
>> @@ -1293,7 +1341,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;
>> -	dev_add_pack(&tpriv->pt);
>> +	stmmac_sft_add_pack(&tpriv->pt);
>>  
>>  	ret = vlan_vid_add(priv->dev, htons(proto), tpriv->vlan_id);
>>  	if (ret)
> 
> [Severity: High]
> Same ordering as __stmmac_test_vlanfilt() here: the tap is registered, then
> vlan_vid_add() runs, and only afterwards does the function do
> "attr.dst = priv->dev->dev_addr;", so tpriv->packet->dst is NULL for the
> whole window.
Yup, same answer as above :)

Maxime

> 
> [ ... ]

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

[Maxime Chevallier]

On 9/15/26 10:45, Paolo Abeni wrote:
> 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/
> ---
> net: stmmac: selftests: Capture all packets for vlan checks
> 
> The VLAN selftests use vlan_vid_add() without a VLAN netdev, so the test
> frame is marked PACKET_OTHERHOST and reaches the handler with a cleared
> skb->vlan_tci.  This patch reuses the DSA proxy mechanism to register an
> ETH_P_ALL handler for the three VLAN tests so validation sees the tag.
> 
> A few questions about the new capture_all path below.
> 
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
>> index 7f5837d1fed94..315c3cf82230e 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;Hi,
>>  	int ok;
>> @@ -344,7 +345,7 @@ 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.
>>  		 */
> 
> [Severity: Low]
> This isn't a bug, but the comment now only describes one of the two reasons
> for taking this branch.  For the three VLAN tests pt.type is ETH_P_IP or
> ETH_P_8021Q, which would match at ptype_specific just fine, so the
> "DSA conduit will report ETH_P_XDSA" rationale does not apply there.
> 
> Could the comment also mention the capture_all reason (the tap has to run
> at ptype_all so skb->vlan_tci is still populated), and could the new
> capture_all field get a one-line comment stating that it must be set before
> stmmac_sft_add_pack() is called?
> 
>>  		tpriv->pt.type = htons(ETH_P_ALL);
>>  		tpriv->pt.func = stmmac_sft_filter;
>>  		tpriv->pt.ignore_outgoing = true;
>>  	}
> 
> [Severity: Medium]
> With pt.type set to ETH_P_ALL on priv->dev, stmmac_test_vlan_validate() now
> sees every ingress frame, and it declares the test failed before it has
> established that the frame is a test frame:
> 
> 	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;
> 		}
> 	}
> 
> 	ehdr = (struct ethhdr *)skb_mac_header(skb);
> 	if (!ether_addr_equal_unaligned(ehdr->h_dest, tpriv->packet->dst))
> 		goto out;
> 
> The h_dest and magic checks come after this.  Can any ordinary C-tagged
> IPv4 frame on the link (any VID other than 0x123, at least 33 bytes of
> linear data) now abort the VLAN Filter, Double VLAN Filter and VLAN Offload
> tests with a spurious failure for i == 0?  Before this patch the tag was
> cleared for such frames, so the vlan_proto check bailed out first.
> 
> Would it make sense to move the vlan_tci comparison after the destination
> MAC and magic checks so only test frames can set the verdict?

The ethtool selftests as they currently exists are very dependent on external
factors :
 - How was the link negociated (pause, eee)
 - Incoming traffic

So yes, ingress traffic may cause the selftest to fail :( I can move that check
around though for V4 :)

> 
> [Severity: Low]
> This isn't a bug introduced by this patch, but the ETH_P_ALL tap widens the
> set of frames that reach the header parsing in stmmac_test_vlan_validate(),
> which derives offsets from the untrusted IPv4 ihl with only a fixed length
> check:
> 
> 	if (skb_headlen(skb) < (STMMAC_TEST_PKT_SIZE - ETH_HLEN))
> 		goto out;
> 	...
> 	uhdr = (struct udphdr *)((u8 *)ihdr + 4 * ihdr->ihl);
> 	if (uhdr->dest != htons(tpriv->packet->dport))
> 		goto out;
> 
> 	shdr = (struct stmmachdr *)((u8 *)uhdr + sizeof(*uhdr));
> 
> That check covers 33 bytes, which does not include the UDP header, the
> extra +4 double-VLAN offset, or an ihl larger than 5.  A frame with ihl = 15
> makes uhdr->dest be read well past the validated area (and past skb->len for
> a small frame).  The same arithmetic exists in
> stmmac_test_loopback_validate(), so this predates the patch, but would a
> pskb_may_pull() style length check based on the actual ihl be worth adding
> here?
> 
>> @@ -999,6 +1000,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]
> This is a pre-existing issue, but the patch makes it observable.  The
> "Means filter did not work" path in stmmac_test_vlan_validate() reports
> failure through the same tpriv->ok = false state that a timeout produces,
> and the loop here treats that state as the expected outcome for i > 0:
> 
> 	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;
> 	}
> 
> So a frame admitted with a non-configured VID (0x124..0x126) is
> indistinguishable from a correctly dropped frame, and the test still passes
> even if the hardware admits every VID.  Before this patch the mismatch
> branch was unreachable because skb->vlan_tci was always cleared; now it is
> reachable but its verdict is discarded.  Could the return value of
> wait_for_completion_timeout() be checked, or a separate "packet was
> admitted" flag be used, so the two cases can be told apart?  The same
> pattern is in __stmmac_test_dvlanfilt().
> 
> [Severity: Medium]
> Also pre-existing rather than introduced here: tpriv->ok, tpriv->comp and
> the on-stack attr are shared between the softirq validator and this loop
> with no serialization, and init_completion() is only called once, outside
> the loop.  With capture_all, an extra complete() is now reachable from the
> "filter did not work" path for any tagged frame with a different VID.
> 
> Since complete() increments comp.done and wait_for_completion_timeout()
> consumes only one, does a surplus completion in one iteration make the next
> wait_for_completion_timeout() return immediately and take the verdict from a
> stale tpriv->ok?  Would a reinit_completion() at the top of each iteration
> help here?
> 
>> @@ -1093,6 +1095,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: High]
> Does this actually fix the Double VLAN Filter test on the cores that can run
> it?  The test requires priv->dma_cap.vlhash, i.e. the xmac cores, where
> probe sets priv->hw->hw_vlan_en = true and stmmac_hw_setup() programs
> VLAN_TAG_STRIP_ALL together with VLAN_TAG_CTRL_EVLRXS, while
> vlan_vid_add(ETH_P_8021AD, ...) enables S-VLAN recognition.
> 
> The MAC-stripped outer tag is then reported by vlan_rx_hw() in
> stmmac_vlan.c:
> 
> 	__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vid);
> 
> which hardcodes ETH_P_8021Q, while the validator with double_vlan set wants
> ETH_P_8021AD:
> 
> 	proto = tpriv->double_vlan ? ETH_P_8021AD : ETH_P_8021Q;
> 	...
> 		if (skb->vlan_proto != htons(proto))
> 			goto out;
> 
> So the frame does reach the validator now, but the vlan_proto check takes
> "goto out" without calling complete(), and the test still returns -ETIMEDOUT
> for i == 0 on working hardware unless RX VLAN stripping is turned off.  Is
> the intent that vlan_rx_hw() report the real tag protocol, or that the
> validator accept ETH_P_8021Q for the stripped outer tag?

Heh so since I sent this series, I've received more hardware, including
an dwxgmac2 platform ( finally \o/ )

And yeah this test fails on dwxgmac2 even with this patch :(

I'll address that in V4


> 
>> @@ -1371,6 +1374,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;
>>  	stmmac_sft_add_pack(&tpriv->pt);
>>  
>>  	ret = vlan_vid_add(priv->dev, htons(proto), tpriv->vlan_id);
> 
> [Severity: High]
> Can this dereference a NULL pointer in softirq context?  attr is
> zero-initialized, so attr.dst is NULL, and the ETH_P_ALL handler is
> published by stmmac_sft_add_pack() before attr.dst is assigned:
> 
> 	tpriv->capture_all = true;
> 	stmmac_sft_add_pack(&tpriv->pt);
> 
> 	ret = vlan_vid_add(priv->dev, htons(proto), tpriv->vlan_id);
> 	if (ret)
> 		goto cleanup;
> 
> 	attr.dst = priv->dev->dev_addr;
> 
> vlan_vid_add() is not instantaneous here: it does
> pm_runtime_resume_and_get() and a register poll in vlan_write_filter().  A
> frame arriving in that window with vlan_proto ETH_P_8021Q and vlan_tci equal
> to the hard-coded 0x123 passes the tag checks and falls into:
> 
> 	ehdr = (struct ethhdr *)skb_mac_header(skb);
> 	if (!ether_addr_equal_unaligned(ehdr->h_dest, tpriv->packet->dst))
> 		goto out;
> 
> which reads 6 bytes from address 0.  stmmac_test_loopback_validate() guards
> the same comparison with "if (dst)", so the pointer does not look like an
> invariant.  The same ordering exists in __stmmac_test_vlanfilt() and
> __stmmac_test_dvlanfilt(), where attr.dst is only set inside the loop.
> Would assigning attr.dst before stmmac_sft_add_pack(), or adding the NULL
> check to this validator, close that window?

Good point, I'll send a V4 then,

Maxime