Re: [PATCH net 3/3] net: stmmac: dwmac1000: stop hardware from dropping checksum-error packets

[netdev-bot+sashiko@kernel.org]

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

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

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] dwmac1000_dma_operation_mode_rx() now sets CSR6.DT
  (DMA_CONTROL_DT) on every dwmac1000 core, whichever descriptor format…

--

net: stmmac: dwmac1000: stop hardware from dropping checksum-error packets

This patch sets DMA_CONTROL_DT in dwmac1000_dma_operation_mode_rx(). With
that bit set, the dwmac1000 MAC hands up frames that fail the Rx checksum
offload check instead of dropping them. The aim is to make VXLAN over IPv6
with zero UDP checksums work, with the driver marking those frames
CHECKSUM_NONE.

Link: https://lore.kernel.org/all/20250625132117.1b3264e8@kernel.org/
Link: https://github.com/freifunk-gluon/gluon/issues/3783

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
> index 3ac7a7949529c..00c8d4dfb553b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
> @@ -129,6 +129,8 @@ static void dwmac1000_dma_operation_mode_rx(struct stmmac_priv *priv,
>  {
>  	u32 csr6 = readl(ioaddr + DMA_CHAN_CONTROL(channel));
>  
> +	csr6 |= DMA_CONTROL_DT;

[Severity: High]
Does this work on dwmac1000 cores that use normal descriptors?

DT is now set no matter which descriptor format is in use. Only the
enhanced descriptor parser turns checksum-only errors into csum_none, via
enh_desc_get_rx_status()->enh_desc_coe_rdes0().

stmmac_hw_init() reads enh_desc and rx_coe from separate HW capability
bits:

    priv->plat->enh_desc = priv->dma_cap.enh_desc;
    ...
    if (priv->dma_cap.rx_coe_type2)
        priv->plat->rx_coe = STMMAC_RX_COE_TYPE2;

stmmac_dwmac1_quirks() then picks ndesc_ops whenever enh_desc is 0. So the
driver allows a core with normal descriptors and Type-2 Rx COE.

ndesc_get_rx_status() can only return good_frame or discard_frame. It
never sets csum_none, and nothing in the driver checks
RDES0_PAYLOAD_CSUM_ERR from descs.h:

    if (unlikely(rdes0 & RDES0_ERROR_SUMMARY)) {
        ...
        if (unlikely(rdes0 & RDES0_IPC_CSUM_ERROR))
            x->ipc_csum_error++;
        ...
        ret = discard_frame;
    }

If ES is set for the checksum error, stmmac_rx() now drops the frame in
software and counts it as an rx error. Wouldn't VXLAN over IPv6 with zero
UDP checksums still be broken on these cores?

The comment in enh_desc_get_rx_status() says the databook does not
require ES for payload-only checksum errors. If ES is clear in that case,
ndesc_get_rx_status() returns good_frame, and stmmac_rx() does:

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

Could a frame with a bad L4 checksum, which the hardware used to drop, now
reach the stack marked CHECKSUM_UNNECESSARY?

Either way, this part of the commit message would not hold for normal
descriptors:

  "CoE remains enabled, so the driver still sees the error status and
   marks the skb CHECKSUM_NONE for the stack to verify."

Would it make sense to set DT only when enhanced descriptors are in use,
or to make ndesc_get_rx_status() report csum_none for checksum errors?

> +
>  	if (mode == SF_DMA_MODE) {
>  		pr_debug("GMAC: enable RX store and forward mode\n");
>  		csr6 |= DMA_CONTROL_RSF | DMA_CONTROL_DFF;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020190319.884375-1-opensource%40grische.xyz