[PATCH net-next] net: stmmac: Setup TBS only if HW supports it

[PATCH net-next] net: stmmac: Setup TBS only if HW supports it

[muhammad.nazim.amirul.nazle.asmade]

From: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>

stmmac_setup_dma_desc() unconditionally honors the per-queue tbs_en flag
set by platform glue drivers and marks the queue STMMAC_TBS_AVAIL, which
selects enhanced Tx descriptors and enables the EDSE bit in the DMA
channel. However, glue drivers set tbs_en in their setup path, which runs
before dma_cap is populated by stmmac_hw_init(), so they cannot themselves
validate against the TBSSEL hardware capability bit.

If tbs_en is set on a controller whose MAC IP was not synthesized with TBS
(MAC_HW_FEATURE3.TBSSEL == 0), the driver lays out the ring using the
enhanced descriptor stride while the DMA engine stays in basic-descriptor
mode, mismatching the descriptor format.

stmmac_enable_tbs() does read back the EDSE bit and return -EIO when it
does not stick, but that happens after stmmac_setup_dma_desc() has already
allocated the ring with the enhanced descriptor stride, and its return
value is discarded by the caller. Gating on dma_cap.tbssel in
stmmac_setup_dma_desc() prevents the enhanced descriptor allocation at the
earliest point, which is the only place the mismatch can be avoided
cleanly.

Only set STMMAC_TBS_AVAIL when the core has read TBSSEL from the HW
capability register.

Signed-off-by: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 07a6fab6460e..f604ac848f77 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4094,12 +4094,14 @@ stmmac_setup_dma_desc(struct stmmac_priv *priv, unsigned int mtu)
 		dma_conf->dma_rx_size = DMA_DEFAULT_RX_SIZE;
 
 	/* Earlier check for TBS */
-	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++) {
-		struct stmmac_tx_queue *tx_q = &dma_conf->tx_queue[chan];
-		int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;
+	if (priv->dma_cap.tbssel) {
+		for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++) {
+			struct stmmac_tx_queue *tx_q = &dma_conf->tx_queue[chan];
+			int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;
 
-		/* Setup per-TXQ tbs flag before TX descriptor alloc */
-		tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
+			/* Setup per-TXQ tbs flag before TX descriptor alloc */
+			tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
+		}
 	}
 
 	ret = alloc_dma_desc_resources(priv, dma_conf);
-- 
2.43.7

Re: [PATCH net-next] net: stmmac: Setup TBS only if HW supports it

[netdev-bot+sashiko]

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

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

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).

- [Medium] The patch is a bug fix (it prevents a descriptor-format
  mismatch where the Tx ring is laid out with the 32-byte `struct…
- [Medium] The new gate in stmmac_setup_dma_desc() (stmmac_main.c:4128)
  only checks the chip-wide capability bit `priv->dma_cap.tbssel`; it…

Pre-existing issues:
- [High] On a partial per-queue allocation failure the unwind helpers
  touch queues that were never allocated and dereference NULL.
- [Medium] In stmmac_setup_dma_desc() (stmmac_main.c:4138-4158), when
  alloc_dma_desc_resources() fails because the *TX* stage failed, the…
- [Low] `dma_cap.tbs_ch_num` is populated only by
  dwxgmac2_get_hw_feature() (dwxgmac2_dma.c:457).

--

net: stmmac: Setup TBS only if HW supports it

The change makes stmmac_setup_dma_desc() consult priv->dma_cap.tbssel
before honoring the per-queue tbs_en flag from platform glue, so that
STMMAC_TBS_AVAIL (and with it the enhanced Tx descriptor stride) is only
set when the MAC reports the TBSSEL hardware capability bit.

A few questions about the change and the surrounding allocation path
below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index bf9e7e4cb1c3..c2c88660e422 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4125,12 +4125,14 @@ stmmac_setup_dma_desc(struct stmmac_priv *priv, unsigned int mtu)
>  		dma_conf->dma_rx_size = DMA_DEFAULT_RX_SIZE;
>  
>  	/* Earlier check for TBS */
> -	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++) {
> -		struct stmmac_tx_queue *tx_q = &dma_conf->tx_queue[chan];
> -		int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;
> +	if (priv->dma_cap.tbssel) {

[Severity: Medium]
This reads as a fix for a real Tx-path descriptor format mismatch, but the
commit message carries only a Signed-off-by: trailer and no Fixes: tag, no
reported symptom and no affected controller.

Two anchors look plausible:

  579a25a854d4 ("net: stmmac: Initial support for TBS") introduced the
  unconditional

      tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;

  ba39b344e924 ("net: ethernet: stmicro: stmmac: generate stmmac dma conf
  before open") moved it into stmmac_setup_dma_desc().

Could a Fixes: tag and a note on which controller and symptom were observed
be added, so the target tree (net vs net-next) and stable backporting are
decidable?

> +		for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++) {
> +			struct stmmac_tx_queue *tx_q = &dma_conf->tx_queue[chan];
> +			int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;
>  
> -		/* Setup per-TXQ tbs flag before TX descriptor alloc */
> -		tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
> +			/* Setup per-TXQ tbs flag before TX descriptor alloc */
> +			tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
> +		}
>  	}

[Severity: Medium]
Is a chip-wide TBSSEL check sufficient here, given that TBS is a per-channel
property in hardware?

dwxgmac2_get_hw_feature() decodes a channel count next to the selector bit:

    dma_cap->tbs_ch_num = ((hw_cap & XGMAC_HWFEAT_TBSCH) >> 28) + 1;
    dma_cap->tbssel = (hw_cap & XGMAC_HWFEAT_TBSSEL) >> 27;

and common.h documents tbs_ch_num as "Number of DMA channels enabled for
TBS". dwmac-socfpga.c also states this explicitly for a shipping part:

    default:
        /* Tx Queues 0 - 5 doesn't support TBS on Agilex5 */
        break;

Meanwhile dwmac-intel.c, dwmac-imx.c, dwmac-mediatek.c,
dwmac-qcom-ethqos.c and stmmac_pci.c set tbs_en on every Tx queue above 0.
On a core where TBSSEL reads 1 but only the top channels are synthesized for
TBS, doesn't STMMAC_TBS_AVAIL still get set on the non-TBS channels, so
stmmac_get_tx_desc_size() still returns sizeof(struct dma_edesc) and the ring
is still allocated with the enhanced stride while EDSE cannot stick on those
channels?

tbs_ch_num currently has no consumer outside stmmac_dma_cap_show(), so
nothing in the allocation path looks at it.

The -EIO produced by dwmac4_enable_tbs():

    value = readl(ioaddr + DMA_CHAN_TX_CONTROL(dwmac4_addrs,
                                               chan)) & DMA_CONTROL_EDSE;
    if (en && !value)
        return -EIO;

is discarded at both call sites, stmmac_hw_setup() and
stmmac_enable_tx_queue():

    stmmac_enable_tbs(priv, priv->ioaddr, enable, chan);

so a residual per-channel mismatch would be neither prevented nor logged.
Does that make the commit message statement

    "Gating on dma_cap.tbssel in stmmac_setup_dma_desc() prevents the
     enhanced descriptor allocation at the earliest point, which is the only
     place the mismatch can be avoided cleanly."

too strong, since the same place could also gate per channel and/or act on
the stmmac_enable_tbs() return value?

Separately, a platform that requested tbs_en on a core without TBSSEL now
silently loses it. Would a netdev_warn() there help platform owners notice
the behaviour change?

[Severity: Low]
This is a pre-existing issue and not introduced by this patch, but it is
related to the capability the gate now depends on: tbs_ch_num is populated
only by dwxgmac2_get_hw_feature(). dwmac4_get_hw_feature() sets only

    dma_cap->tbssel = (hw_cap & GMAC_HW_FEAT_TBSSEL) >> 27;

and leaves tbs_ch_num at 0, while stmmac_dma_cap_show() prints both fields:

    seq_printf(seq, "\tNumber of DMA Channels Enabled for TBS: %d\n",
               priv->dma_cap.tbs_ch_num);

So on a dwmac4-class core with TBSSEL == 1, debugfs reports TBS as supported
immediately followed by a TBS channel count of 0. Should
dwmac4_get_hw_feature() decode the channel count too?

>  
>  	ret = alloc_dma_desc_resources(priv, dma_conf);

[Severity: Medium]
This isn't a bug introduced by this patch, but it sits in the path the patch
touches: when alloc_dma_desc_resources() fails on the Tx stage, are the Rx
page pools leaked?

    static int alloc_dma_desc_resources(struct stmmac_priv *priv,
                                        struct stmmac_dma_conf *dma_conf)
    {
            int ret = alloc_dma_rx_desc_resources(priv, dma_conf);

            if (ret)
                    return ret;

            ret = alloc_dma_tx_desc_resources(priv, dma_conf);

            return ret;
    }

alloc_dma_tx_desc_resources() unwinds only the Tx side, and back in
stmmac_setup_dma_desc() the error path is:

    alloc_error:
            kfree(dma_conf);
            return ERR_PTR(ret);

free_dma_desc_resources() is bypassed, and dma_conf is the only reference to
the Rx page pools, buf_pool arrays, coherent Rx rings and xdp_rxq_info
registrations at that point, since the memcpy into priv->dma_conf happens
later in __stmmac_open().

[Severity: High]
This isn't a bug introduced by this patch either, but on the same allocation
path: can the partial-failure unwind dereference NULL?

alloc_dma_tx_desc_resources() does:

            ret = __alloc_dma_tx_desc_resources(priv, dma_conf, queue);
            if (ret)
                    goto err_dma;
    ...
    err_dma:
            free_dma_tx_desc_resources(priv, dma_conf);

free_dma_tx_desc_resources() then walks every Tx queue, including those never
allocated, and __free_dma_tx_desc_resources() unconditionally does:

            /* Release the DMA TX socket buffers */
            dma_free_tx_skbufs(priv, dma_conf, queue);

which reaches stmmac_free_tx_buffer():

            if (tx_q->tx_skbuff_dma[i].buf &&
                tx_q->tx_skbuff_dma[i].buf_type != STMMAC_TXBUF_T_XDP_TX) {

For the queue whose first kzalloc_objs() failed, and for all higher queues
that were never reached, tx_skbuff_dma is still NULL because dma_conf comes
from kzalloc. Doesn't that oops? And if it were guarded, the following

            dma_free_coherent(priv->device, size, addr, tx_q->dma_tx_phy);

would be called with a NULL address and a zero dma handle for a ring that was
never allocated.

The Rx side looks the same: __free_dma_rx_desc_resources() unconditionally
calls dma_free_rx_skbufs(), and stmmac_free_rx_buffer() dereferences
rx_q->buf_pool[i] with buf_pool == NULL.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918023530.31971-1-muhammad.nazim.amirul.nazle.asmade%40altera.com