[PATCH net-next v4] net: stmmac: dwxgmac2: enable L2 split header support

[PATCH net-next v4] net: stmmac: dwxgmac2: enable L2 split header support

[Sagar Cheluvegowda]

XGMAC2 doesn't program the MAC extended configuration registers
needed for L2 header splitting when split-header mode is enabled,
so tagged AV packets can't use the split-header path. Configure
split mode (SPLM) and header saving (SAVE_EN) in XGMAC_EXT_CFG1,
extending the dwmac4 sequence added by commit d10f1a4e44c3 ("net:
stmmac: Programming sequence for VLAN packets with split header")
to XGMAC2.

Also fix the RX header-length calculation for split-header
descriptors. Per the XGMAC2 descriptor format, the header
length is only defined on the first descriptor (FD set), and RDES3's
L34T and L2T fields are only valid on the last descriptor (LD set);
on a non-last descriptor they can hold stale write-back data from a
previous packet. Read the header length only on the first descriptor:
from RDES2[9:0] directly for non-last descriptors, and via L34T/L2T
on the last descriptor: RDES2[9:0] for IP packets (L34T set) and
RDES2[9:2] for non-IP packets (L2T set).

Finally, disable the variable-length preamble when L2 splitting
is enabled. A variable-length preamble shifts the position of the
Ethernet header, making the L2 split offset non-deterministic.

Co-developed-by: Abhishek Chauhan <abhishek.chauhan@oss.qualcomm.com>
Signed-off-by: Abhishek Chauhan <abhishek.chauhan@oss.qualcomm.com>
Co-developed-by: Sarosh Hasan <sarosh.hasan@oss.qualcomm.com>
Signed-off-by: Sarosh Hasan <sarosh.hasan@oss.qualcomm.com>
Signed-off-by: Sagar Cheluvegowda <sagar.cheluvegowda@oss.qualcomm.com>
---
v4:
  - Only read the header length on the first descriptor (FD=1)
v3: https://lore.kernel.org/netdev/20261013192759.776537-1-sagar.cheluvegowda@oss.qualcomm.com/
  - Use zero-padded 0x00000140 for XGMAC_EXT_CFG0
v2: https://lore.kernel.org/netdev/20261007183905.2582759-1-sagar.cheluvegowda@oss.qualcomm.com/
  - Add XGMAC_CONFIG1_SPLM_MASK and use it in XGMAC_CONFIG1_SPLM()
v1: https://lore.kernel.org/netdev/20260922232116.889630-1-sagar.cheluvegowda@oss.qualcomm.com/

 drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h   |  9 +++++++++
 .../net/ethernet/stmicro/stmmac/dwxgmac2_descs.c | 16 +++++++++++++++-
 .../net/ethernet/stmicro/stmmac/dwxgmac2_dma.c   | 10 ++++++++++
 3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
index f8ab347f7b5b..7f43a0b17083 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
@@ -167,6 +167,12 @@
 #define XGMAC_HW_FEATURE4		0x0000012c
 #define XGMAC_HWFEAT_EASP		BIT(4)
 #define XGMAC_HWFEAT_PCSEL		GENMASK(1, 0)
+#define XGMAC_EXT_CFG0			0x00000140
+#define XGMAC_EXT_CFG0_VPRE		BIT(8)
+#define XGMAC_EXT_CFG1			0x00000144
+#define XGMAC_CONFIG1_SAVE_EN		BIT(24)
+#define XGMAC_CONFIG1_SPLM_MASK		GENMASK(9, 8)
+#define XGMAC_CONFIG1_SPLM(v)		FIELD_PREP(XGMAC_CONFIG1_SPLM_MASK, v)
 #define XGMAC_MAC_DPP_FSM_INT_STATUS	0x00000150
 #define XGMAC_MAC_FSM_CONTROL		0x00000158
 #define XGMAC_PRTYEN			BIT(1)
@@ -434,13 +440,16 @@
 #define XGMAC_RDES2_L4FM		BIT(28)
 #define XGMAC_RDES2_L3FM		BIT(27)
 #define XGMAC_RDES2_HL			GENMASK(9, 0)
+#define XGMAC_RDES2_NONIPHL		GENMASK(9, 2)
 #define XGMAC_RDES3_OWN			BIT(31)
 #define XGMAC_RDES3_CTXT		BIT(30)
 #define XGMAC_RDES3_IOC			BIT(30)
+#define XGMAC_RDES3_FD			BIT(29)
 #define XGMAC_RDES3_LD			BIT(28)
 #define XGMAC_RDES3_CDA			BIT(27)
 #define XGMAC_RDES3_RSV			BIT(26)
 #define XGMAC_RDES3_L34T		GENMASK(23, 20)
+#define XGMAC_RDES3_L2T			GENMASK(19, 16)
 #define XGMAC_RDES3_ET_LT		GENMASK(19, 16)
 #define XGMAC_L34T_IP4TCP		0x1
 #define XGMAC_L34T_IP4UDP		0x2
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
index 6719ac6e395b..b28aa39f9947 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
@@ -292,8 +292,22 @@ static int dwxgmac2_get_rx_hash(struct dma_desc *p, u32 *hash,
 
 static void dwxgmac2_get_rx_header_len(struct dma_desc *p, unsigned int *len)
 {
-	if (le32_to_cpu(p->des3) & XGMAC_RDES3_L34T)
+	u32 rdes3 = le32_to_cpu(p->des3);
+
+	/* HL is only defined on the first descriptor */
+	if (!(rdes3 & XGMAC_RDES3_FD))
+		return;
+
+	/* when FD=1 and LD=0, HL is RDES2[9:0] */
+	if (!(rdes3 & XGMAC_RDES3_LD)) {
+		*len = le32_to_cpu(p->des2) & XGMAC_RDES2_HL;
+		return;
+	}
+
+	if (rdes3 & XGMAC_RDES3_L34T)
 		*len = le32_to_cpu(p->des2) & XGMAC_RDES2_HL;
+	else if (rdes3 & XGMAC_RDES3_L2T)
+		*len = (le32_to_cpu(p->des2) & XGMAC_RDES2_NONIPHL) >> 2;
 }
 
 static void dwxgmac2_set_sec_addr(struct dma_desc *p, dma_addr_t addr, bool is_valid)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
index ff83858ebc1f..0a9c787305a4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
@@ -561,6 +561,16 @@ static void dwxgmac2_enable_sph(struct stmmac_priv *priv, void __iomem *ioaddr,
 	value |= XGMAC_CONFIG_HDSMS_256; /* Segment max 256 bytes */
 	writel(value, ioaddr + XGMAC_RX_CONFIG);
 
+	value = readl(ioaddr + XGMAC_EXT_CFG1);
+	value |= XGMAC_CONFIG1_SPLM(1);
+	value |= XGMAC_CONFIG1_SAVE_EN;
+	writel(value, ioaddr + XGMAC_EXT_CFG1);
+
+	/* Disable variable preambles to keep the L2 split offset deterministic. */
+	value = readl(ioaddr + XGMAC_EXT_CFG0);
+	value &= ~XGMAC_EXT_CFG0_VPRE;
+	writel(value, ioaddr + XGMAC_EXT_CFG0);
+
 	value = readl(ioaddr + XGMAC_DMA_CH_CONTROL(chan));
 	if (en)
 		value |= XGMAC_SPH;
-- 
2.34.1

Re: [PATCH net-next v4] net: stmmac: dwxgmac2: enable L2 split header support

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] On XLGMAC cores, dwxgmac2_enable_sph() now corrupts the RX queue
  enable register.
- [Medium] dwxgmac2_enable_sph() programs MAC-global split and preamble
  settings regardless of `en`.
- [Low] The patch includes a fix for an existing XGMAC split-header bug
  inside a feature patch, with no Fixes: tag.
- [Low] dwxgmac2_enable_sph() sets the 2-bit SPLM field
  (XGMAC_CONFIG1_SPLM_MASK = GENMASK(9,8)) with `value |=…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the stmmac
  RX path has no dma_rmb() between seeing RDES3.OWN cleared and reading…

--

net: stmmac: dwxgmac2: enable L2 split header support

This patch makes dwxgmac2_enable_sph() program SPLM and SAVE_EN in
XGMAC_EXT_CFG1 and clear the variable preamble bit in XGMAC_EXT_CFG0. It
also changes dwxgmac2_get_rx_header_len() so the header length is read
only on the first descriptor. For non-IP (L2T) packets on the last
descriptor, the length comes from RDES2[9:2].

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
> index 6719ac6e395bc..b28aa39f99472 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
> @@ -292,8 +292,22 @@ static int dwxgmac2_get_rx_hash(struct dma_desc *p, u32 *hash,
>  
>  static void dwxgmac2_get_rx_header_len(struct dma_desc *p, unsigned int *len)
>  {
> -	if (le32_to_cpu(p->des3) & XGMAC_RDES3_L34T)
> +	u32 rdes3 = le32_to_cpu(p->des3);
> +
> +	/* HL is only defined on the first descriptor */
> +	if (!(rdes3 & XGMAC_RDES3_FD))
> +		return;
> +
> +	/* when FD=1 and LD=0, HL is RDES2[9:0] */
> +	if (!(rdes3 & XGMAC_RDES3_LD)) {
> +		*len = le32_to_cpu(p->des2) & XGMAC_RDES2_HL;
> +		return;
> +	}

[Severity: Low]
Should this FD/LD handling go in its own patch with a Fixes: tag?

The commit message says L34T can hold stale data on a non-last
descriptor. If so, the old code:

	if (le32_to_cpu(p->des3) & XGMAC_RDES3_L34T)
		*len = le32_to_cpu(p->des2) & XGMAC_RDES2_HL;

could return hlen == 0 for the first descriptor of a multi-descriptor IP
frame with SPH active. stmmac_rx_buf1_len() would then take this path:

	/* Not last descriptor and not split header: buffer1 is fully filled */
	if (status & rx_not_ls)
		return priv->dma_conf.dma_buf_sz;

even though buffer 1 only holds the header.

That looks like an existing XGMAC split-header bug in the code from
commit 96147375d49f ("net: stmmac: xgmac: Only get SPH header len if
available"). With the fix bundled into this feature patch, stable trees
would also need the new EXT_CFG0/EXT_CFG1 programming to pick it up.

> +
> +	if (rdes3 & XGMAC_RDES3_L34T)
>  		*len = le32_to_cpu(p->des2) & XGMAC_RDES2_HL;
> +	else if (rdes3 & XGMAC_RDES3_L2T)
> +		*len = (le32_to_cpu(p->des2) & XGMAC_RDES2_NONIPHL) >> 2;
>  }

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but
is a dma_rmb() missing between the OWN check and the read of des2 here?

dwxgmac2_get_rx_status() loads des3 and des2, then checks OWN:

	u32 rdes3 = le32_to_cpu(p->des3);
	u32 rdes2 = le32_to_cpu(p->des2);

	if (unlikely(rdes3 & XGMAC_RDES3_OWN))
		return dma_own;

stmmac_rx() then calls stmmac_rx_buf1_len()->dwxgmac2_get_rx_header_len()
with no barrier in between. The TX completion path in stmmac_main.c calls
dma_rmb() after its own check, and dwxgmac2_rx_check_timestamp() also
uses one.

On weakly ordered CPUs, could the RDES2 load complete before OWN is seen
as cleared? That would give a stale HL and a wrong buf1_len.

The new FD/LD logic doesn't depend on this ordering.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> index ff83858ebc1f3..0a9c787305a4e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> @@ -561,6 +561,16 @@ static void dwxgmac2_enable_sph(struct stmmac_priv *priv, void __iomem *ioaddr,
>  	value |= XGMAC_CONFIG_HDSMS_256; /* Segment max 256 bytes */
>  	writel(value, ioaddr + XGMAC_RX_CONFIG);
>  
> +	value = readl(ioaddr + XGMAC_EXT_CFG1);
> +	value |= XGMAC_CONFIG1_SPLM(1);

[Severity: Low]
SPLM is a two-bit field (GENMASK(9, 8)). Should it be cleared with
XGMAC_CONFIG1_SPLM_MASK before the OR, the way HDSMS is handled just
above?

	value &= ~XGMAC_CONFIG_HDSMS;
	value |= XGMAC_CONFIG_HDSMS_256;

If bit 9 were already set, the field would end up as 3 instead of mode 1.

In stmmac_hw_setup(), the SWR from stmmac_init_dma_engine()->
stmmac_reset() runs before this. So in practice the result depends on
the register's reset value.

> +	value |= XGMAC_CONFIG1_SAVE_EN;
> +	writel(value, ioaddr + XGMAC_EXT_CFG1);
> +
> +	/* Disable variable preambles to keep the L2 split offset deterministic. */
> +	value = readl(ioaddr + XGMAC_EXT_CFG0);
> +	value &= ~XGMAC_EXT_CFG0_VPRE;
> +	writel(value, ioaddr + XGMAC_EXT_CFG0);

[Severity: High]
Can this corrupt the RX queue enable register on XLGMAC cores?

The DWXLGMAC_CORE_2_00 entry in hwif.c uses the same DMA ops as XGMAC:

	.desc = &dwxgmac210_desc_ops,
	.dma = &dwxgmac210_dma_ops,
	.mac = &dwxlgmac2_ops,

so dwxgmac2_enable_sph() runs there too. On XLGMAC, dwxlgmac2.h defines
offset 0x140 as:

	#define XLGMAC_RXQ_ENABLE_CTRL0		0x00000140

and dwxlgmac2_rx_queue_enable() programs it like this:

	value = readl(ioaddr + XLGMAC_RXQ_ENABLE_CTRL0) & ~XGMAC_RXQEN(queue);
	if (mode == MTL_QUEUE_AVB)
		value |= 0x1 << XGMAC_RXQEN_SHIFT(queue);

BIT(8) is therefore the AVB enable for RX queue 4. Clearing
XGMAC_EXT_CFG0_VPRE would disable that queue.

The write to 0x144 also sets BIT(8) and BIT(24) in an XLGMAC register
that the driver doesn't describe.

stmmac_hw_setup() enables the MTL queues through
stmmac_mtl_configuration() first. Later it runs:

	sph_en = (priv->hw->rx_csum > 0) && priv->sph_active;
	for (chan = 0; chan < rx_cnt; chan++)
		stmmac_enable_sph(priv, priv->ioaddr, sph_en, chan);

There is no sph_capable or core check, so this would happen on every
XLGMAC open, whether or not split header is supported. Should this be
gated on the core type, or moved into a per-core MAC op?

> +
>  	value = readl(ioaddr + XGMAC_DMA_CH_CONTROL(chan));
>  	if (en)
>  		value |= XGMAC_SPH;

[Severity: Medium]
Only the per-channel XGMAC_SPH bit depends on en here. SPLM/SAVE_EN are
set and VPRE is cleared on every call, and nothing restores them.

stmmac_hw_setup() calls stmmac_enable_sph() for every RX channel, even
when sph_en is false or the core has no SPH support.
stmmac_set_features() calls it with sph_en == false when NETIF_F_RXCSUM
is cleared, and stmmac_xdp_open() also calls it.

Doesn't that leave the variable preamble disabled on every XGMAC device
after hw_setup? It would also stay disabled after split header is turned
off with ethtool.

The commit message says:

    Finally, disable the variable-length preamble when L2 splitting
    is enabled.

which doesn't seem to match this behavior.

Also, dwmac4_enable_sph(), which this extends, doesn't change the
preamble configuration. And these MAC-global read-modify-writes are
repeated once per RX channel.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015221215.1714047-1-sagar.cheluvegowda%40oss.qualcomm.com