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

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

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?
-- 
This is an AI-generated review.