* Re: [PATCH net-next v2] net: stmmac: dwxgmac2: enable L2 split header support
2026-10-07 18:39 [PATCH net-next v2] net: stmmac: dwxgmac2: enable L2 split header support Sagar Cheluvegowda
@ 2026-10-07 18:45 ` netdev-bot+sinfo
2026-10-08 10:14 ` Maxime Chevallier
2026-10-11 19:13 ` netdev-bot+sashiko
2 siblings, 0 replies; 4+ messages in thread
From: netdev-bot+sinfo @ 2026-10-07 18:45 UTC (permalink / raw)
To: Sagar Cheluvegowda
Cc: Maxime Chevallier, Andrew Lunn, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Alexandre Torgue,
netdev, linux-arm-msm, linux-stm32, linux-arm-kernel,
linux-kernel, Lorenzo Bianconi, Hongmei Xu, Abhishek Chauhan,
Sarosh Hasan
Hi!
This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:
- How the issue was discovered, e.g. hit in production, hit during
development, syzbot report, manual code inspection, LLM or static
analysis tool scan.
- Whether the issue was actually triggered, or is only theoretical
(e.g. found by code inspection). If it was triggered please include
the symptoms, like the stack trace or error messages.
- What hardware the change was tested on. For driver fixes please
mention the device (and if relevant firmware version) used for
testing, or say that the change was not tested on real hardware.
Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.
The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next v2] net: stmmac: dwxgmac2: enable L2 split header support
2026-10-07 18:39 [PATCH net-next v2] net: stmmac: dwxgmac2: enable L2 split header support Sagar Cheluvegowda
2026-10-07 18:45 ` netdev-bot+sinfo
@ 2026-10-08 10:14 ` Maxime Chevallier
2026-10-11 19:13 ` netdev-bot+sashiko
2 siblings, 0 replies; 4+ messages in thread
From: Maxime Chevallier @ 2026-10-08 10:14 UTC (permalink / raw)
To: Sagar Cheluvegowda, Andrew Lunn, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Alexandre Torgue
Cc: netdev, linux-arm-msm, linux-stm32, linux-arm-kernel,
linux-kernel, Lorenzo Bianconi, Hongmei Xu, Abhishek Chauhan,
Sarosh Hasan
Hi,
On 10/7/26 20:39, Sagar Cheluvegowda wrote:
> 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, 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 from RDES2[9:0] directly
> for non-last descriptors, and only consult 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>
> ---
> v2:
> - 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 | 8 ++++++++
> drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c | 12 +++++++++++-
> drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c | 10 ++++++++++
> 3 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> index f8ab347f7b5b..fb29dd489714 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 0x140
Let's keep it coherent and use 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,6 +440,7 @@
> #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)
> @@ -441,6 +448,7 @@
> #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..65ac5403ec58 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
> @@ -292,8 +292,18 @@ 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);
> +
> + /* 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;
The rest looks good to me :)
Maxime
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net-next v2] net: stmmac: dwxgmac2: enable L2 split header support
2026-10-07 18:39 [PATCH net-next v2] net: stmmac: dwxgmac2: enable L2 split header support Sagar Cheluvegowda
2026-10-07 18:45 ` netdev-bot+sinfo
2026-10-08 10:14 ` Maxime Chevallier
@ 2026-10-11 19:13 ` netdev-bot+sashiko
2 siblings, 0 replies; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-10-11 19:13 UTC (permalink / raw)
To: sagar.cheluvegowda
Cc: maxime.chevallier, andrew+netdev, davem, edumazet, kuba, pabeni,
mcoquelin.stm32, alexandre.torgue, netdev, linux-arm-msm,
linux-stm32, linux-arm-kernel, linux-kernel, lorenzo.bianconi,
hongmei.xu, abhishek.chauhan, sarosh.hasan
Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.
Critical: 0 · High: 2 · Medium: 2 (2) · 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() writes to the wrong
registers.
- [High] net: stmmac: incorrect shift in non-IP header length calculation
- [Medium] dwxgmac2_enable_sph() programs the MAC-wide split
configuration regardless of `en` or split-header capability, which…
- [Medium] stmmac: missing mask clear before setting XGMAC_CONFIG1_SPLM
field
- [Low] The commit message presents the RX header-length change in
dwxgmac2_get_rx_header_len() as a fix for an existing defect: RDES3…
- [Low] net: stmmac: dwxgmac2: missing mask when setting multi-bit SPLM
field
Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the error
handling in stmmac_rx() can lose the partial-packet state.
- [Medium] This problem wasn't introduced by this patch, but the XGMAC RX
path reads descriptor write-back fields without a DMA read barrier…
--
net: stmmac: dwxgmac2: enable L2 split header support
This patch makes dwxgmac2_enable_sph() set SPLM and SAVE_EN in
XGMAC_EXT_CFG1 and clear VPRE in XGMAC_EXT_CFG0, so that L2 header
splitting works on XGMAC2. It also changes dwxgmac2_get_rx_header_len().
On non-last descriptors it now reads the header length from RDES2[9:0]
directly. On the last descriptor it uses RDES2[9:2] for non-IP packets
that have L2T set.
[ ... ]
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
> index 6719ac6e395bc..65ac5403ec583 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
> @@ -292,8 +292,18 @@ 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);
> +
> + /* when FD=1 and LD=0, HL is RDES2[9:0] */
> + if (!(rdes3 & XGMAC_RDES3_LD)) {
[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. The
comment says FD=1, but the code never checks FD. Can a continuation
descriptor get here and be treated as a first descriptor?
In stmmac_rx(), suppose napi_build_skb() fails on the first descriptor
of a multi-descriptor packet:
stmmac_rx() {
...
skb = napi_build_skb(page_address(buf->page),
rx_q->napi_skb_frag_size);
if (!skb) {
page_pool_recycle_direct(rx_q->page_pool,
buf->page);
buf->page = NULL;
rx_dropped++;
count++;
goto drain_data;
}
...
}
error is not set, and the code goes back to read_again. If the next
descriptor is still owned by the DMA, status is dma_own and the loop
exits. The state save is then skipped, because status has no
rx_not_ls and skb is NULL:
if (status & rx_not_ls || skb) {
rx_q->state_saved = true;
...
}
On the next poll, len and error are both 0 even though the descriptor
is a continuation. That bypasses this guard in stmmac_rx_buf1_len():
if (priv->sph_active && len)
return 0;
dwxgmac2_get_rx_header_len() is then called on a descriptor with FD
clear.
Would buffer 1 of that continuation be handled as the header of a new
packet, so a malformed packet reaches the stack? Split IP packets
could already hit this before the patch. The patch only changes which
RDES2 field gets read here.
> + *len = le32_to_cpu(p->des2) & XGMAC_RDES2_HL;
> + return;
> + }
[Severity: Medium]
This isn't a bug introduced by this patch, but is there an ordering
problem between the OWN check and this RDES2 read?
dwxgmac2_get_rx_status() checks OWN first:
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 reads des2 again through
stmmac_rx_buf1_len()->stmmac_get_rx_header_len()->
dwxgmac2_get_rx_header_len(). There is no dma_rmb() between the two
reads on this path.
On weakly ordered platforms, could the RDES2 load complete before the
RDES3 load that sees OWN cleared, and return a stale header length?
This patch now reads RDES2 in more cases (non-last descriptors and L2T
packets).
> +
> + 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: High]
Does this shift turn a byte count into a dword count?
If RDES2[9:2] holds the non-IP header length in units of 4 bytes, the
value masked with XGMAC_RDES2_NONIPHL is already the length in bytes.
Shifting it right by 2 then gives the number of dwords instead.
stmmac_rx_buf1_len() passes hlen straight back to stmmac_rx() as a
byte count:
stmmac_get_rx_header_len(priv, p, &hlen);
if (priv->sph_active && hlen) {
priv->xstats.rx_split_hdr_pkt_n++;
return hlen;
}
stmmac_rx() then uses buf1_len for dma_sync_single_for_cpu(),
xdp_prepare_buff() and the skb_put() on the head buffer. For a 16-byte
header this returns 4, so only 4 bytes of the Ethernet header land in
the linear part of the skb, and buffer 2 is attached after them as a
frag. The rest of the header is lost.
Wouldn't stmmac_rx_vlan() and eth_type_trans() then read past the
4-byte linear area, and the ETH_HLEN pull in eth_type_trans() hit the
BUG() in __skb_pull()?
The L34T case right above uses RDES2[9:0] as a byte count with no
shift. Can you confirm against the databook whether the non-IP case
really needs the >> 2?
[Severity: Low]
The commit message describes this change as a fix for an existing
defect:
Also fix the RX header-length calculation for split-header
descriptors. Per the XGMAC2 descriptor format, 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.
The L34T-gated read being replaced came from commit 96147375d49f
("net: stmmac: xgmac: Only get SPH header len if available").
If that premise holds, existing XGMAC SPH users can get hlen == 0 on a
split first descriptor. They then fall through to this in
stmmac_rx_buf1_len():
if (status & rx_not_ls)
return priv->dma_conf.dma_buf_sz;
Should this part go in its own patch with a Fixes: tag? Then it could
be backported without the new L2 split and VPRE changes.
> 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);
> + 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: Medium]
This read-modify-write keeps whatever was in bits 9:8 of
XGMAC_EXT_CFG1 and ORs in SPLM=1 on top. If a bootloader or earlier
firmware left SPLM at 2, the register ends up with SPLM=3 instead of
the L2 offset mode this patch wants.
dwxgmac2_enable_sph() is called from stmmac_hw_setup() for every RX
channel, and again from the XDP and stmmac_set_features() paths. None
of those calls would ever correct the field, so the MAC would stay in
whatever split mode the combined value selects. What does SPLM=3 do on
XGMAC2? If it is reserved or selects a different split point, the
header length the hardware reports would no longer match what
dwxgmac2_get_rx_header_len() expects.
dwmac4_enable_sph() looks to have the same pattern for GMAC_EXT_CFG1,
so copying it here carries the issue over. Should the field be cleared
with XGMAC_CONFIG1_SPLM_MASK before XGMAC_CONFIG1_SPLM(1) is applied?
[Severity: Low]
XGMAC_CONFIG1_SPLM_MASK is a two-bit field, GENMASK(9, 8), but the
old contents of that field are never cleared before the new value is
ORed in. If bit 9 is already set when dwxgmac2_enable_sph() runs, the
field ends up as 3 rather than 1.
The reset value is probably 0, so this may not matter in practice.
Still, the usual pattern for FIELD_PREP() in a read-modify-write is to
clear the mask first:
value &= ~XGMAC_CONFIG1_SPLM_MASK;
value |= XGMAC_CONFIG1_SPLM(1);
The v2 changelog says the mask was added for XGMAC_CONFIG1_SPLM().
Could it also be used here to clear the field?
[Severity: High]
Can this clear an RX queue enable bit on XLGMAC? The DWXLGMAC_ID entry
in stmmac_hw[] in hwif.c uses the XGMAC DMA ops:
.desc = &dwxgmac210_desc_ops,
.dma = &dwxgmac210_dma_ops,
.mac = &dwxlgmac2_ops,
That means dwxgmac2_enable_sph() also runs on XLGMAC. On that core,
offset 0x140 is the RX queue enable register in dwxlgmac2.h:
#define XLGMAC_RXQ_ENABLE_CTRL0 0x00000140
dwxlgmac2_rx_queue_enable() programs a 2-bit enable field per queue in
that register:
if (mode == MTL_QUEUE_AVB)
value |= 0x1 << XGMAC_RXQEN_SHIFT(queue);
For queue 4 in AVB mode this is BIT(8). That is the same bit this code
clears as XGMAC_EXT_CFG0_VPRE.
stmmac_hw_setup() enables the RX queues in stmmac_mtl_configuration().
After that, it calls stmmac_enable_sph() for every RX channel, without
checking sph_capable or the core type:
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);
Would an AVB-mode RX queue 4 on XLGMAC end up disabled after every
open, every XDP reconfiguration, and every stmmac_set_features() call?
The write to 0x144 also sets bits 8 and 24 on XLGMAC, and the driver
doesn't define what that offset holds on that core.
Should these MAC register writes move into the per-core MAC ops, or be
gated on the core ID?
> value = readl(ioaddr + XGMAC_DMA_CH_CONTROL(chan));
> if (en)
> value |= XGMAC_SPH;
[Severity: Medium]
The XGMAC_EXT_CFG1 and XGMAC_EXT_CFG0 writes above run whether or not
en is set. Only the per-channel XGMAC_SPH bit depends on en. The
commit message says:
Finally, disable the variable-length preamble when L2 splitting
is enabled.
stmmac_hw_setup() and the XDP setup path call stmmac_enable_sph() for
every RX channel without checking priv->sph_capable. Does that mean
every XGMAC instance gets SPLM and SAVE_EN set and VPRE cleared? That
would include cores without SPH, STMMAC_FLAG_SPH_DISABLE setups, setups
with XDP attached, and setups with RX checksum off.
The en == false path, for example stmmac_set_features() after ethtool
-K rx off, also never clears SPLM/SAVE_EN or restores VPRE. The dwmac4
sequence sets SPLM/SAVE_EN unconditionally too, but the VPRE change is
new.
Should these settings be applied only when en is true, and undone
otherwise?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007183905.2582759-1-sagar.cheluvegowda%40oss.qualcomm.com
^ permalink raw reply [flat|nested] 4+ messages in thread