Re: [PATCH net-next v5 3/3] net: xilinx: axienet: Fix IRQ error handling

[netdev-bot+sashiko@kernel.org]

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

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

- [High] Incomplete fix: the axistream-connected branch of
  axienet_probe() still cannot request a probe retry, and this patch now…

Pre-existing issues:
- [Medium] DT binding and driver disagree on interrupt index ordering.

--

net: xilinx: axienet: Fix IRQ error handling

This changes the DMA IRQ validation in axienet_probe() so that a zero
result from irq_of_parse_and_map() returns -EINVAL instead of -ENOMEM,
and adds explicit propagation of negative error codes coming from
platform_get_irq().

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 3927ababf833..782f903d318f 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -2971,10 +2971,14 @@ static int axienet_probe(struct platform_device *pdev)
>  			dev_err(&pdev->dev, "could not map DMA regs\n");
>  			return PTR_ERR(lp->dma_regs);
>  		}
> -		if (lp->rx_irq <= 0 || lp->tx_irq <= 0) {
> +		if (!lp->rx_irq || !lp->tx_irq) {
>  			dev_err(&pdev->dev, "could not determine irqs\n");
> -			return -ENOMEM;
> +			return -EINVAL;
>  		}

[Severity: High]
Does turning the zero case into a permanent -EINVAL remove the chance of a
probe retry for the axistream-connected branch?

In that branch the IRQs come from:

		lp->rx_irq = irq_of_parse_and_map(np, 1);
		lp->tx_irq = irq_of_parse_and_map(np, 0);

irq_of_parse_and_map() folds every failure into 0, including the case where
the interrupt provider's IRQ domain is not registered yet:

drivers/of/irq.c:irq_of_parse_and_map() {
	if (of_irq_parse_one(dev, index, &oirq))
		return 0;

	return irq_create_of_mapping(&oirq);
}

irq_create_of_mapping() also returns 0 when no domain is found, so
"not ready yet" and "malformed DT" are indistinguishable here, and
axienet_probe() now converts both into a terminal -EINVAL with no
-EPROBE_DEFER path.

Would of_irq_get(np, index) be a better fit? It is documented to return
-EPROBE_DEFER if the IRQ domain is not yet created, and 0 only when the
interrupt is genuinely absent, which would let a single pair of checks

		if (irq < 0)
			return irq;
		if (!irq)
			return -EINVAL;

cover both the axistream and the platform_get_irq() branches.

Could the commit message also mention that the deferral case stays
unhandled with irq_of_parse_and_map(), or use of_irq_get() so that it is?

> +		if (lp->rx_irq < 0)
> +			return lp->rx_irq;
> +		if (lp->tx_irq < 0)
> +			return lp->tx_irq;

Are these two checks reachable for the axistream-connected branch?
irq_of_parse_and_map() returns an unsigned int virq, so they only ever fire
for the values obtained from platform_get_irq(), which makes the added
error handling asymmetric between the two IRQ sources.

>  		if (lp->eth_irq < 0 && lp->eth_irq != -ENXIO)
>  			return lp->eth_irq;
>

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but since
the patch hardens validation on exactly this branch, it may be worth
reconciling: the driver and the binding disagree on the interrupt index
order.

The direct-resource branch of axienet_probe() reads:

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

Documentation/devicetree/bindings/net/xlnx,axi-ethernet.yaml describes the
opposite positional order:

  interrupts:
    items:
      - description: Ethernet core interrupt
      - description: Tx DMA interrupt
      - description: Rx DMA interrupt

The same probe function treats index 0 on the ethernet node as the Ethernet
core IRQ in the axistream and dmas branches:

		lp->eth_irq = platform_get_irq_optional(pdev, 0);

so index 0 has two meanings inside one function and at most one of them can
match the binding.

The binding text is also self-inconsistent: it says the Ethernet core
interrupt is optional while placing it first in a positional items list with
minItems: 1, which is only expressible when the optional entry is last, as
the driver assumes.

For a three-interrupt DT written from the documented order, all three values
are positive, so neither !lp->rx_irq || !lp->tx_irq nor the new < 0 tests
reject it, and axienet_init_legacy_dma() goes on to request_irq() the
Ethernet core line as a DMA handler. Should the binding or the driver be
corrected so the two agree?

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