diff mbox series

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

Message ID 20260824100901.31675-1-phucduc.bui@gmail.com (mailing list archive)
State New
Headers
Series [net] net: ethernet: sun4i-emac: Fix IRQ error handling |

Commit Message

Bui Duc Phuc Aug. 24, 2026, 10:09 a.m. UTC 
From: bui duc phuc <phucduc.bui@gmail.com>

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.

Fixes: 492205050d77 ("net: Add EMAC ethernet driver found on Allwinner A10 SoC's")
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 drivers/net/ethernet/allwinner/sun4i-emac.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

sashiko-bot@kernel.org Aug. 25, 2026, 10:09 a.m. UTC | #1 
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?
Andre Przywara Aug. 25, 2026, 10:32 a.m. UTC | #2 
Hi,

On 8/24/26 12:09, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> 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.

Yes, the return value is the IRQ number, or 0 in case of a missing IRQ.

> Fixes: 492205050d77 ("net: Add EMAC ethernet driver found on Allwinner A10 SoC's")
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Thanks,
Andre

> ---
>   drivers/net/ethernet/allwinner/sun4i-emac.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c b/drivers/net/ethernet/allwinner/sun4i-emac.c
> index 42174249ef61..942454e29488 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;
>   	}
>
Bui Duc Phuc Aug. 25, 2026, 10:44 a.m. UTC | #3 
Hi Andre,

Thank you for your review and the Reviewed-by tag.

>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>

Best regards,
Phuc
Bui Duc Phuc Aug. 25, 2026, 10:55 a.m. UTC | #4 
> 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.
> --

Both issues are pre-existing and are not introduced by this patch.
I will keep the scope of this patch limited to IRQ error handling and
will not address these issues here.

For the first issue, the cleanup could be separated so that
dma_release_channel() is only reached after
successful DMA configuration:
---------------------------------------------------------
-       if (emac_configure_dma(db))
+       ret = emac_configure_dma(db);
+       if (ret) {
               netdev_info(ndev, "configure dma failed. disable dma.\n");
+              goto out_dispose_mapping;
+       }
...
       if (IS_ERR(db->clk)) {
               ret = PTR_ERR(db->clk);
-              goto out_dispose_mapping;
+              goto out_release_dma;
       }
...
+out_release_dma:
+       dma_release_channel(db->rx_chan);
 out_dispose_mapping:
       irq_dispose_mapping(ndev->irq);
-      dma_release_channel(db->rx_chan);
------------------------------------------------------------

I will not include this change in the current patch.
These issues would be better addressed and tested by someone with
access to the hardware.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c b/drivers/net/ethernet/allwinner/sun4i-emac.c
index 42174249ef61..942454e29488 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;
 	}