Re: [PATCH net-next v4] net: stmmac: Add support for TX/RX channel interrupt

[Nazle Asmade, Muhammad Nazim Amirul]

On 12/5/2026 4:17 pm, Paolo Abeni wrote:
> 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://netdev-ai.bots.linux.dev/sashiko/
> ---
> net: stmmac: Add support for TX/RX channel interrupt
>
> This change enables per-channel TX/RX interrupt registration for the
> stmmac driver by looking up interrupts with dma_rxN / dma_txN names
> from the device tree. The commit message documents the new names via
> an example interrupts / interrupt-names stanza.
>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> index 5cae2aa72906..8f8dc7f08f20 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> @@ -732,7 +732,10 @@ static int stmmac_pltfr_get_irq_array(struct platform_device *pdev,
>>   int stmmac_get_platform_resources(struct platform_device *pdev,
>>                                struct stmmac_resources *stmmac_res)
>>   {
>> +    char irq_name[9];
>>      int ret;
>> +    int irq;
>> +    int i;
>
> [Low]
> Is irq_name[9] intentionally sized to the exact minimum for "dma_rxN\0"
> with a single-digit N?
>
> MTL_MAX_RX_QUEUES and MTL_MAX_TX_QUEUES are both 8 today, so single
> digits fit, but if either constant is raised past 9 the snprintf()
> below would silently truncate (for example "dma_rx10" becoming
> "dma_rx1") and alias onto another channel name.
>
> The sibling helper stmmac_pltfr_get_irq_array() in the same file uses
> a 16-byte buffer and explicitly rejects truncation:
>
>       if (snprintf(name, sizeof(name), fmt, i) >= sizeof(name))
>               return -EINVAL;
>
> Would it be worth matching that pattern here, so the two code paths
> behave consistently and stay correct if the queue counts grow?
>
>>
>>      memset(stmmac_res, 0, sizeof(*stmmac_res));
>>
>> @@ -767,6 +770,30 @@ int stmmac_get_platform_resources(struct platform_device *pdev,
>>              dev_info(&pdev->dev, "IRQ sfty not found\n");
>>      }
>>
>> +    /* For RX Channel */
>> +    for (i = 0; i < MTL_MAX_RX_QUEUES; i++) {
>> +            snprintf(irq_name, sizeof(irq_name), "dma_rx%i", i);
>> +            irq = platform_get_irq_byname_optional(pdev, irq_name);
>> +            if (irq == -ENXIO)
>> +                    break;
>> +            else if (irq < 0)
>> +                    return irq;
>> +
>> +            stmmac_res->rx_irq[i] = irq;
>> +    }
>> +
>> +    /* For TX Channel */
>> +    for (i = 0; i < MTL_MAX_TX_QUEUES; i++) {
>> +            snprintf(irq_name, sizeof(irq_name), "dma_tx%i", i);
>> +            irq = platform_get_irq_byname_optional(pdev, irq_name);
>> +            if (irq == -ENXIO)
>> +                    break;
>> +            else if (irq < 0)
>> +                    return irq;
>> +
>> +            stmmac_res->tx_irq[i] = irq;
>> +    }
>> +
>>      stmmac_res->addr = devm_platform_ioremap_resource(pdev, 0);
>>
>>      if (IS_ERR(stmmac_res->addr))
>
> [High, Low]
> Can this new block actually take effect on a DT that uses the new
> dma_rxN / dma_txN names shown in the commit message?
>
> The tail of stmmac_get_platform_resources() (unchanged by this patch)
> still calls stmmac_pltfr_get_irq_array() for the legacy names right
> after these loops:
>
>       /* TX channels irq */
>       ret = stmmac_pltfr_get_irq_array(pdev, "tx-queue-%d",
>                                        stmmac_res->tx_irq,
>                                        MTL_MAX_TX_QUEUES);
>       ...
>       /* RX channels irq */
>       ret = stmmac_pltfr_get_irq_array(pdev, "rx-queue-%d",
>                                        stmmac_res->rx_irq,
>                                        MTL_MAX_RX_QUEUES);
>
> And stmmac_pltfr_get_irq_array() unconditionally overwrites the slot,
> zeroing it if the legacy name is absent:
>
>       irqs[i] = platform_get_irq_byname_optional(pdev, name);
>       ...
>       if (irqs[i] <= 0) {
>               dev_dbg(&pdev->dev, "IRQ %s not found\n", name);
>
>               /* Stop silently on first unset irq */
>               irqs[i] = 0;
>               break;
>       }
>
> On a DT that only provides dma_rx0 / dma_tx0 (the naming this patch
> introduces), tx-queue-0 and rx-queue-0 are not present, so both
> stmmac_res->rx_irq[0] and stmmac_res->tx_irq[0] end up back at 0 after
> the legacy helper runs.
>
> The MSI registration path in stmmac_main.c treats 0 as "no IRQ":
>
>       if (priv->rx_irq[i] == 0)
>               continue;
>
> so channel 0 is silently skipped for both RX and TX. Is that the
> intended behaviour, or should the legacy call be removed / made
> mutually exclusive with the new names (or gated on the slot being
> still 0)?
>
> Relatedly, could the commit message say why both naming schemes need
> to exist in the same probe path? Right now the new dma_rxN / dma_txN
> names and the pre-existing rx-queue-N / tx-queue-N names are both
> consulted with no documented precedence or fallback, which leaves
> platform authors without a clear contract.
>
> [Medium]
> Does the DT binding need to be updated alongside this driver change?
>
> Documentation/devicetree/bindings/net/snps,dwmac.yaml currently only
> enumerates the per-channel DMA IRQ names as:
>
>       - ^rx-queue-[0-7]$
>       - ^tx-queue-[0-7]$
>
> The new dma_rxN / dma_txN names consumed here and advertised in the
> commit message do not appear in the schema, so a DT using the example
> stanza would fail dtbs_check. Should this series include a matching
> update to snps,dwmac.yaml so the binding and the driver land together
> and DT maintainers can ack the new names?
Hi Paolo, this patch will need to re-post back with another patches in
series after reviewing comment from Jakub. Will re-send back soon. Thanks!