Re: [PATCH net-next v2 3/3] include/stmmac: Increase max DMA/MTL channel count from 8 to 16

[Paolo Abeni]

From: AI Reviewer <ai@xxxxxxxxxxx>

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://sashiko.dev/
---
include/stmmac: Increase max DMA/MTL channel count from 8 to 16

Newer XGMAC hardware does support up to 16 DMA/MTL queues. Add support for
these after previous modifications of driver to accommodate for that.

> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> index 4430b967abdeb..a9d19e2c388c6 100644
> --- a/include/linux/stmmac.h
> +++ b/include/linux/stmmac.h
> @@ -15,9 +15,9 @@
>  #include <linux/platform_device.h>
>  #include <linux/phylink.h>
>  
> -#define MTL_MAX_RX_QUEUES	8
> -#define MTL_MAX_TX_QUEUES	8
> -#define STMMAC_CH_MAX		8
> +#define MTL_MAX_RX_QUEUES	16
> +#define MTL_MAX_TX_QUEUES	16
> +#define STMMAC_CH_MAX		16

[Severity: High]
Does increasing these limits to 16 cause undefined behavior and silent register
truncation in the hardware configuration functions when queue >= 8?

In dwxgmac2_core.c:dwxgmac2_tx_queue_prio(), when queue >= 8, the logic
only subtracts 4:

    if (queue >= 4)
        queue -= 4;

This leaves queue >= 4, which causes XGMAC_PSTC_SHIFT(queue) to shift by >= 32
bits, overflowing the 32-bit integer.

A similar regression occurs in dwmac4_core.c:dwmac4_tx_queue_priority() where
it shifts by GMAC_TXQCTRL_PSTQX_SHIFT(queue).

Additionally, in dwmac4_core.c:dwmac4_rx_queue_routing(), the queue variable
is masked against a 3-bit bitmask, which silently truncates any queue >= 8:

    value |= (queue << route_possibilities[packet-1].reg_shift) & 
             route_possibilities[packet - 1].reg_mask;

Finally, dwmac4_core.c:dwmac4_map_mtl_dma() only maps queues up to 7 (using
MAP0 and MAP1):

    if (queue < 4) {
    ...
    } else {
        value = readl(ioaddr + MTL_RXQ_DMA_MAP1);
        value &= ~MTL_RXQ_DMA_QXMDMACH_MASK(queue - 4);

For queue >= 8, the shift calculation overflows and corrupts register data.
Should these hardware configuration functions be updated to handle the expanded
queue limits?

[Severity: Critical]
This is a pre-existing issue, but does missing bounds checking on the device
tree properties expose an out-of-bounds array access during device probe?

In stmmac_platform.c:stmmac_mtl_setup(), the device tree property
snps,tx-queues-to-use is parsed and clamped only to 255 (U8_MAX):

    if (!of_property_read_u32(tx_node, "snps,tx-queues-to-use", &value)) {
        if (value > U8_MAX)
            value = U8_MAX;
        plat->tx_queues_to_use = value;
    }

Later in stmmac_main.c:stmmac_hw_setup(), it attempts to dynamically clamp
this value to the hardware capabilities:

    if (priv->dma_cap.number_tx_queues &&
        priv->plat->tx_queues_to_use > priv->dma_cap.number_tx_queues) {
        priv->plat->tx_queues_to_use = priv->dma_cap.number_tx_queues;
    }

However, if the hardware feature registers report 0 (which happens for older
GMAC versions), this check is bypassed. The driver fails to clamp the
requested queues against the static internal array limit MTL_MAX_TX_QUEUES.

As a result, in stmmac_main.c:init_dma_tx_desc_rings(), the initialization
loop iterates up to the unvalidated tx_queues_to_use:

    for (queue = 0; queue < tx_queue_cnt; queue++)
        __init_dma_tx_desc_rings(priv, dma_conf, queue);

This will overrun the bounds of arrays embedded in struct stmmac_priv (like
dma_conf->tx_queue) if the device tree configuration exceeds the static array
limits. Should we add a strict bounds check against MTL_MAX_TX_QUEUES and
MTL_MAX_RX_QUEUES during probe?
-- 
This is an AI-generated review.