Re: [PATCH net-next v5 2/3] net: xilinx: axienet: Handle optional IRQ return value correctly

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] net: xilinx: axienet: Handle optional IRQ return value
  correctly — the patch carries a `Fixes: 522856cefaf0` tag and a…

Pre-existing issues:
- [Medium] DT binding vs driver interrupt indexing mismatch (pre-
  existing, not touched by this patch).

--

net: xilinx: axienet: Handle optional IRQ return value correctly

This changes the informational check on lp->eth_irq in axienet_probe()
from "<= 0" to "< 0", on the grounds that platform_get_irq_optional()
never returns 0. The patch carries a Fixes tag pointing at 522856cefaf0.

No Link: tags were present in the commit header.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 207aecb5f8b9..3927ababf833 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -3052,7 +3052,7 @@ static int axienet_probe(struct platform_device *pdev)
>  		ndev->ethtool_ops = &axienet_ethtool_ops;
>  	}
>  	/* Check for Ethernet core IRQ (optional) */
> -	if (lp->eth_irq <= 0)
> +	if (lp->eth_irq < 0)
>  		dev_info(&pdev->dev, "Ethernet core IRQ not defined\n");

[Severity: Medium]

Is this change a functional no-op, and if so, is the Fixes tag warranted?

The commit message itself states:

    Errors other than -ENXIO are handled before this check,
    so -ENXIO is the only error value that can reach this point.

Combined with platform_get_irq_optional() never returning 0:

drivers/base/platform.c:platform_get_irq_affinity() {
	...
out_not_found:
	ret = -ENXIO;
out:
	if (WARN(!ret, "0 is an invalid IRQ number\n"))
		return -EINVAL;
	...
}

the set of values lp->eth_irq can hold when reaching this dev_info() is
{positive IRQ number, -ENXIO}. Over that set, "<= 0" and "< 0" select
exactly the same values, so no configuration behaves differently before or
after this patch.

Would it be better to drop the Fixes tag, note that no functional change is
intended, and avoid a stable backport of a change with no observable effect?

Alternatively, writing the test as:

	if (lp->eth_irq == -ENXIO)
		dev_info(&pdev->dev, "Ethernet core IRQ not defined\n");

would state the reachable condition directly.

There is also a consistency question. Every other test of this field in
the same file still uses "> 0", for example in axienet_device_reset():

	axienet_iow(lp, XAE_IE_OFFSET, lp->eth_irq > 0 ?

and in axienet_init_legacy_dma():

	if (lp->eth_irq > 0) {
		ret = request_irq(lp->eth_irq, axienet_eth_irq, IRQF_SHARED,

If treating 0 as an undefined IRQ is the problem being addressed, why are
the sites that actually request and enable the interrupt left using the
complementary "> 0" form, making this the only "< 0" test of eth_irq?

[Severity: Medium]

This isn't a bug introduced by this patch, but while looking at where
lp->eth_irq comes from, the interrupt indexing in axienet_probe() and the
binding appear to disagree.

Documentation/devicetree/bindings/net/xlnx,axi-ethernet.yaml describes a
fixed item order:

  interrupts:
    items:
      - description: Ethernet core interrupt
      - description: Tx DMA interrupt
      - description: Rx DMA interrupt
    ...
    minItems: 1

but the branch in axienet_probe() that reads the DMA resources from the
Ethernet node uses:

			lp->rx_irq = platform_get_irq(pdev, 1);
			lp->tx_irq = platform_get_irq(pdev, 0);
			lp->eth_irq = platform_get_irq_optional(pdev, 2);

that is, index 0 = Tx DMA, 1 = Rx DMA, 2 = Ethernet core.

For a device tree written strictly to the binding in that shape, would
axienet_eth_irq() end up bound to a DMA line, and the DMA handlers to the
wrong lines?

The same pair also seems to disagree on minItems: 1, since this path
requires index 1 unconditionally:

		if (!lp->rx_irq || !lp->tx_irq) {
			dev_err(&pdev->dev, "could not determine irqs\n");
			return -EINVAL;
		}

The axistream-connected and dmas paths use platform_get_irq_optional(pdev, 0)
for the Ethernet core interrupt and match the binding, and both yaml
examples are consistent with the driver, so only the
DMA-resources-on-the-Ethernet-node shape looks affected. There is no
in-tree DTS for xlnx,axi-ethernet-* to settle which side is correct.
Should the binding or the driver indexing be corrected in a separate patch?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903073252.339250-1-phucduc.bui%40gmail.com