Re: [PATCH net] net: ethernet: sun4i-emac: Fix IRQ error handling

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unconditional call to `dma_release_channel(db->rx_chan)` in the `emac_probe()` error path can cause a kernel panic if DMA configuration fails.
- [High] The driver unconditionally enables MAC interrupts during system resume, even if the network interface was logically down, which can permanently break the interface.
--

--- Patch [1]: [PATCH net] net: ethernet: sun4i-emac: Fix IRQ error handling ---
commit aeff88d2fcbccbc3cb84803b3c45048b12777cd3
Author: bui duc phuc <phucduc.bui@gmail.com>

net: ethernet: sun4i-emac: Fix IRQ error handling

irq_of_parse_and_map() returns 0 when parsing or mapping an IRQ fails.
The current code checks for -ENXIO and therefore does not detect the
failure.

Check for a zero return value and convert it to -ENXIO.

> diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c b/drivers/net/ethernet/allwinner/sun4i-emac.c
> index 42174249ef61d..942454e294882 100644
> --- a/drivers/net/ethernet/allwinner/sun4i-emac.c
> +++ b/drivers/net/ethernet/allwinner/sun4i-emac.c
> @@ -996,9 +996,9 @@ static int emac_probe(struct platform_device *pdev)
>  	/* fill in parameters for net-dev structure */
>  	ndev->base_addr = (unsigned long)db->membase;
>  	ndev->irq = irq_of_parse_and_map(np, 0);
> -	if (ndev->irq == -ENXIO) {
> +	if (!ndev->irq) {
>  		netdev_err(ndev, "No irq resource\n");
> -		ret = ndev->irq;
> +		ret = -ENXIO;
>  		goto out_iounmap;
>  	}
>  

[Severity: High]
This isn't a bug introduced by this patch, but is there a pre-existing panic
risk during error handling later in emac_probe()?

If emac_configure_dma() fails, it explicitly sets db->rx_chan to NULL:

drivers/net/ethernet/allwinner/sun4i-emac.c:emac_configure_dma() {
    ...
    db->rx_chan = NULL;
    ...
}

If a subsequent initialization step such as devm_clk_get() fails, the probe
error path unconditionally calls dma_release_channel():

drivers/net/ethernet/allwinner/sun4i-emac.c:emac_probe() {
    ...
out_dispose_mapping:
    irq_dispose_mapping(ndev->irq);
    dma_release_channel(db->rx_chan);
    ...
}

Will dma_release_channel() dereference the null channel pointer (via
chan->client_count) without checking for a null value first?

[Severity: High]
This is also a pre-existing issue, but does emac_resume() unconditionally
enable mac interrupts even if the network interface is down?

If the system goes to sleep while the interface is logically down (meaning
emac_open() was never called and no irq handler is registered), the resume
handler still calls emac_init_device():

drivers/net/ethernet/allwinner/sun4i-emac.c:emac_resume() {
    ...
    emac_init_device(ndev);
    ...
}

Which in turn enables hardware interrupts:

drivers/net/ethernet/allwinner/sun4i-emac.c:emac_init_device() {
    ...
    writel(reg_val, db->membase + EMAC_INT_CTL_REG);
    ...
}

If the hardware subsequently asserts an interrupt (for example, from broadcast
packets) but no handler is registered, will this cause an interrupt storm that
forces the generic irq subsystem to permanently disable the interrupt line?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824100901.31675-1-phucduc.bui@gmail.com?part=1