[PATCH v2 0/7] net: ethernet: release managed IRQs before freeing netdevs

[PATCH v2 0/7] net: ethernet: release managed IRQs before freeing netdevs

[Jiale Yao]

Several Ethernet platform drivers request interrupts with
devm_request_irq() but allocate and free their netdevs manually.
Device-managed resources are released only after the driver's remove
callback returns, so these callbacks free the IRQ data while the interrupt
handlers can still be invoked. A late or shared interrupt in this window
can dereference freed memory.

For six drivers, make the netdev allocation device managed. Since each IRQ
is requested after its netdev is allocated, devres ordering releases the
IRQ before the netdev. SXGBE also keeps its hardware operations object
alive through the same ordering because its handlers dereference that
object directly.

RAVB takes a separate path because its ndo_stop() participates in runtime
PM teardown and its remove callback can encounter a resume failure before
unregister_netdev(). Keep its netdev manually managed, place its IRQs in a
dedicated devres group, and explicitly release that group after
unregistering the netdev. On a resume failure, continue the software
teardown without an unmatched runtime PM put.

Each patch handles one driver and is independently buildable.

Changes in v2:
- Keep commit message tags together without blank lines between them, as
  requested by Francesco.

Jiale Yao (7):
  net: macb: manage the netdev lifetime with devres
  net: fec: manage the netdev lifetime with devres
  net: hip04: manage the netdev lifetime with devres
  net: hisi_femac: manage the netdev lifetime with devres
  net: hix5hd2: manage the netdev lifetime with devres
  net: ravb: fix resource teardown ordering
  net: sxgbe: manage IRQ data lifetimes with devres

 drivers/net/ethernet/cadence/macb_main.c      | 17 +++++-------
 drivers/net/ethernet/freescale/fec_main.c     |  8 +++---
 drivers/net/ethernet/hisilicon/hip04_eth.c    |  4 +--
 drivers/net/ethernet/hisilicon/hisi_femac.c   | 15 +++++------
 drivers/net/ethernet/hisilicon/hix5hd2_gmac.c | 15 +++++------
 drivers/net/ethernet/renesas/ravb_main.c      | 23 +++++++++++-----
 .../net/ethernet/samsung/sxgbe/sxgbe_main.c   | 26 +++++++------------
 7 files changed, 49 insertions(+), 59 deletions(-)

-- 
2.34.1

[PATCH v2 1/7] net: macb: manage the netdev lifetime with devres

[Jiale Yao]

macb_remove() frees the netdev while its managed IRQs are only
released after the remove callback returns. An interrupt in that window
can dereference the freed netdev or queue data.

Allocate the netdev with devres as well. Since the IRQs are registered
later, devres releases them before freeing the netdev and closes the
lifetime gap.

Fixes: 0a4acf08ea62 ("net: macb: Use devm_request_irq()")
Cc: stable@vger.kernel.org
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index b8234ac4b602..ebf6ffb1cc4f 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -5812,7 +5812,8 @@ static int macb_probe(struct platform_device *pdev)
 		goto err_disable_clocks;
 	}
 
-	netdev = alloc_etherdev_mq(sizeof(*bp), num_queues);
+	netdev = devm_alloc_etherdev_mqs(&pdev->dev, sizeof(*bp),
+					 num_queues, num_queues);
 	if (!netdev) {
 		err = -ENOMEM;
 		goto err_disable_clocks;
@@ -5859,7 +5860,7 @@ static int macb_probe(struct platform_device *pdev)
 			IS_ENABLED(CONFIG_MACB_USE_HWSTAMP)) {
 		dev_err(&pdev->dev, "Timer adjust mode is not supported\n");
 		err = -EINVAL;
-		goto err_out_free_netdev;
+		goto err_disable_clocks;
 	}
 
 	/* By default we set to partial store and forward mode for zynqmp.
@@ -5893,7 +5894,7 @@ static int macb_probe(struct platform_device *pdev)
 		err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(44));
 		if (err) {
 			dev_err(&pdev->dev, "failed to set DMA mask\n");
-			goto err_out_free_netdev;
+			goto err_disable_clocks;
 		}
 		bp->caps |= MACB_CAPS_DMA_64B;
 	}
@@ -5903,7 +5904,7 @@ static int macb_probe(struct platform_device *pdev)
 	netdev->irq = platform_get_irq(pdev, 0);
 	if (netdev->irq < 0) {
 		err = netdev->irq;
-		goto err_out_free_netdev;
+		goto err_disable_clocks;
 	}
 
 	/* MTU range: 68 - 1518 or 10240 */
@@ -5932,7 +5933,7 @@ static int macb_probe(struct platform_device *pdev)
 
 	err = of_get_ethdev_address(np, bp->netdev);
 	if (err == -EPROBE_DEFER)
-		goto err_out_free_netdev;
+		goto err_disable_clocks;
 	else if (err)
 		macb_get_hwaddr(bp);
 
@@ -5946,7 +5947,7 @@ static int macb_probe(struct platform_device *pdev)
 	/* IP specific init */
 	err = macb_init(pdev, macb_config);
 	if (err)
-		goto err_out_free_netdev;
+		goto err_disable_clocks;
 
 	err = macb_mii_init(bp);
 	if (err)
@@ -5988,9 +5989,6 @@ static int macb_probe(struct platform_device *pdev)
 err_out_phy_exit:
 	phy_exit(bp->phy);
 
-err_out_free_netdev:
-	free_netdev(netdev);
-
 err_disable_clocks:
 	macb_clks_disable(pclk, hclk, tx_clk, rx_clk, tsu_clk);
 	pm_runtime_disable(&pdev->dev);
@@ -6024,7 +6022,6 @@ static void macb_remove(struct platform_device *pdev)
 		pm_runtime_dont_use_autosuspend(&pdev->dev);
 		pm_runtime_set_suspended(&pdev->dev);
 		phylink_destroy(bp->phylink);
-		free_netdev(netdev);
 	}
 }
 
-- 
2.34.1

[PATCH v2 2/7] net: fec: manage the netdev lifetime with devres

[Jiale Yao]

fec_drv_remove() frees the netdev before devres releases the managed
IRQs whose handlers use it as their data pointer. A late interrupt can
therefore access the freed netdev.

Allocate the netdev with devres so that the later IRQ registrations are
released first during teardown.

Fixes: 0d9b2ab1c376 ("fec: Use devm_request_irq()")
Cc: stable@vger.kernel.org
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 794ec427b0ee..23e794a31ce8 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -5219,8 +5219,9 @@ fec_probe(struct platform_device *pdev)
 	fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
 
 	/* Init network device */
-	ndev = alloc_etherdev_mqs(sizeof(struct fec_enet_private) +
-				  FEC_STATS_SIZE, num_tx_qs, num_rx_qs);
+	ndev = devm_alloc_etherdev_mqs(&pdev->dev,
+				       sizeof(struct fec_enet_private) +
+				       FEC_STATS_SIZE, num_tx_qs, num_rx_qs);
 	if (!ndev)
 		return -ENOMEM;
 
@@ -5480,8 +5481,6 @@ fec_probe(struct platform_device *pdev)
 failed_phy:
 	dev_id--;
 failed_ioremap:
-	free_netdev(ndev);
-
 	return ret;
 }
 
@@ -5522,7 +5521,6 @@ fec_drv_remove(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 
 	fec_enet_deinit(ndev);
-	free_netdev(ndev);
 }
 
 static int fec_suspend(struct device *dev)
-- 
2.34.1

RE: [PATCH v2 2/7] net: fec: manage the netdev lifetime with devres

[Wei Fang]

> Subject: [PATCH v2 2/7] net: fec: manage the netdev lifetime with devres

Please add target tree to the subject. Since this is a fix, the target tree should be net.

> 
> fec_drv_remove() frees the netdev before devres releases the managed
> IRQs whose handlers use it as their data pointer. A late interrupt can
> therefore access the freed netdev.
> 
> Allocate the netdev with devres so that the later IRQ registrations are
> released first during teardown.
> 
> Fixes: 0d9b2ab1c376 ("fec: Use devm_request_irq()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jiale Yao <yaojiale02@163.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 794ec427b0ee..23e794a31ce8 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -5219,8 +5219,9 @@ fec_probe(struct platform_device *pdev)
>  	fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
> 
>  	/* Init network device */
> -	ndev = alloc_etherdev_mqs(sizeof(struct fec_enet_private) +
> -				  FEC_STATS_SIZE, num_tx_qs, num_rx_qs);
> +	ndev = devm_alloc_etherdev_mqs(&pdev->dev,
> +				       sizeof(struct fec_enet_private) +
> +				       FEC_STATS_SIZE, num_tx_qs, num_rx_qs);
>  	if (!ndev)
>  		return -ENOMEM;
> 
> @@ -5480,8 +5481,6 @@ fec_probe(struct platform_device *pdev)
>  failed_phy:
>  	dev_id--;
>  failed_ioremap:

failed_ioremap is no longer needed, please remove it.

> -	free_netdev(ndev);
> -
>  	return ret;
>  }
> 
> @@ -5522,7 +5521,6 @@ fec_drv_remove(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
> 
>  	fec_enet_deinit(ndev);
> -	free_netdev(ndev);
>  }
> 
>  static int fec_suspend(struct device *dev)
> --
> 2.34.1

This patch just prevents the netdev from being freed in fec_drv_remove(),
but fec_enet_interrupt() could still be called after the removal, and
fec_enet_collect_events() will be called to access the registers, however,
the ipg clk has been disabled, the registers are not accessible at that point,
that is a problem.

I think the hardware interrupts should be disabled on the removal path
and disable_irq() should be called to disable the irqs.

BTW, do not repost a new version within 24 hours.
https://elixir.bootlin.com/linux/v7.3-rc4/source/Documentation/process/maintainer-netdev.rst#L15

There are other upstream rules in maintainer-netdev.rst, please
refer to them.

[PATCH v2 3/7] net: hip04: manage the netdev lifetime with devres

[Jiale Yao]

hip04_remove() frees the netdev before the managed IRQ is released
after the remove callback. Since the IRQ handler receives the netdev as
its data pointer, a late interrupt can access freed memory.

Use a managed netdev allocation. Devres then releases the IRQ, which is
registered later, before releasing the netdev.

Fixes: a41ea46a9a12 ("net: hisilicon: new hip04 ethernet driver")
Cc: stable@vger.kernel.org
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index fc2c47dcfaab..4a643dff22ab 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -905,7 +905,7 @@ static int hip04_mac_probe(struct platform_device *pdev)
 	int irq;
 	int ret;
 
-	ndev = alloc_etherdev(sizeof(struct hip04_priv));
+	ndev = devm_alloc_etherdev(d, sizeof(struct hip04_priv));
 	if (!ndev)
 		return -ENOMEM;
 
@@ -1021,7 +1021,6 @@ static int hip04_mac_probe(struct platform_device *pdev)
 	hip04_free_ring(ndev, d);
 init_fail:
 	of_node_put(priv->phy_node);
-	free_netdev(ndev);
 	return ret;
 }
 
@@ -1038,7 +1037,6 @@ static void hip04_remove(struct platform_device *pdev)
 	unregister_netdev(ndev);
 	of_node_put(priv->phy_node);
 	cancel_work_sync(&priv->tx_timeout_task);
-	free_netdev(ndev);
 }
 
 static const struct of_device_id hip04_mac_match[] = {
-- 
2.34.1

Re: [PATCH v2 3/7] net: hip04: manage the netdev lifetime with devres

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] hip04_remove()
  (drivers/net/ethernet/hisilicon/hip04_eth.c:1027-1040) still runs…

Pre-existing issues:
- [High] In hip04_mac_probe(), of_phy_connect() at line 981 succeeds and
  runs phy_attach_direct(), which sets phydev->attached_dev = ndev…
- [High] hip04_timeout() queues priv->tx_timeout_task on system_wq.
- [Medium] priv->tx_coalesce_timer is embedded in the netdev private
  area.
- [Medium] hip04_mac_probe() calls devm_request_irq() (line 972) before
  it masks PPE_INTEN or acks PPE_RINT, and before netif_napi_add() (line…

--

net: hip04: manage the netdev lifetime with devres

This patch switches hip04_mac_probe() to devm_alloc_etherdev(). It also
drops the explicit free_netdev() calls from the probe error path and from
hip04_remove(). The goal is for devres to release the IRQ taken with
devm_request_irq() before it releases the netdev.

> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
> index fc2c47dcfaabe..4a643dff22ab0 100644
> --- a/drivers/net/ethernet/hisilicon/hip04_eth.c
> +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
> @@ -905,7 +905,7 @@ static int hip04_mac_probe(struct platform_device *pdev)
>  	int irq;
>  	int ret;
>  
> -	ndev = alloc_etherdev(sizeof(struct hip04_priv));
> +	ndev = devm_alloc_etherdev(d, sizeof(struct hip04_priv));
>  	if (!ndev)
>  		return -ENOMEM;
>  

[Severity: Medium]
This is a pre-existing issue, but can hip04_mac_interrupt() run before
priv->napi is initialized?

hip04_mac_probe() calls devm_request_irq() before netif_napi_add(), with
of_phy_connect() (which can sleep) in between. Nothing in probe masks
PPE_INTEN or acks PPE_RINT beforehand.

The previous owner may have left the PPE interrupt enabled with RCV_INT
pending. One example is kexec from a kernel with the port up, since the
driver has no .shutdown. In that case:

hip04_mac_interrupt()
  napi_schedule_prep(&priv->napi)   <- zeroed state, returns true
  __napi_schedule(&priv->napi)
    net_rx_action()
      __napi_poll()                 <- napi->poll is NULL

Whether the PPE state survives that handoff depends on the hardware.

[ ... ]

> @@ -1021,7 +1021,6 @@ static int hip04_mac_probe(struct platform_device *pdev)
>  	hip04_free_ring(ndev, d);
>  init_fail:
>  	of_node_put(priv->phy_node);
> -	free_netdev(ndev);
>  	return ret;
>  }
>  

[Severity: High]
This is a pre-existing issue, but does this error path leave the PHY
attached to a netdev that is about to be freed?

Suppose of_phy_connect() succeeds earlier in hip04_mac_probe(), and then
hip04_alloc_ring() or register_netdev() fails. The alloc_fail and
init_fail labels never call phy_disconnect(priv->phy).

phy_attach_direct() set phydev->attached_dev = ndev, took device and
module references, and resumed the PHY. Once devres frees ndev (or
free_netdev() did, before this patch), attached_dev is left dangling.

On a later system suspend:

mdio_bus_phy_suspend()
  mdio_bus_phy_may_suspend()
    netdev->ethtool->wol_enabled   <- netdev has been freed

The MDIO bus and PHY driver module references also leak. A later bind
of this MAC would then fail in phy_attach_direct() with -EBUSY, because
the PHY is still attached.

> @@ -1038,7 +1037,6 @@ static void hip04_remove(struct platform_device *pdev)
>  	unregister_netdev(ndev);

[Severity: Critical]
This fixes the order of the managed IRQ release and the netdev free. Can
a late interrupt still reach freed memory in hip04_remove() through the
rings, though? The start of the function is unchanged:

hip04_remove() {
	if (priv->phy)
		phy_disconnect(priv->phy);

	hip04_free_ring(ndev, d);
	unregister_netdev(ndev);
	...
}

When the interface is up, hip04_free_ring() frees every rx_buf[] frag,
every pending tx_skb[] and the coherent tx_desc ring. It does not clear
those pointers or unmap rx_phys[].

At that point the MAC, PPE_INTEN, NAPI and the IRQ are all still live.
They are only quiesced by ndo_stop inside unregister_netdev().

In that window, an RX interrupt or the tx_coalesce_timer can still do:

hip04_mac_interrupt()
  napi_schedule_prep(&priv->napi)
  __napi_schedule()
    hip04_rx_poll()
      hip04_tx_reclaim()     <- reads the freed tx_desc
      build_skb(buf, ...)    <- buf is a freed rx_buf[] frag

The hardware can also keep DMAing into the freed RX pages.

After that, unregister_netdev()->dev_close()->hip04_mac_stop() calls
hip04_tx_reclaim(ndev, true). Does this double free the skb for every
TX slot that was not yet reclaimed?

hip04_tx_reclaim() {
	...
		desc = &priv->tx_desc[tx_tail];
		if (desc->send_addr != 0) {
			if (force)
				desc->send_addr = 0;
	...
		bytes_compl += priv->tx_skb[tx_tail]->len;
		dev_kfree_skb(priv->tx_skb[tx_tail]);
	...
}

By then tx_desc has already been released by dma_free_coherent(). The
tx_skb[] entries have already been freed by dev_kfree_skb_any() in
hip04_free_ring(), but they were left non-NULL.

hip04_mac_stop() also calls dma_unmap_single(..., DMA_FROM_DEVICE) on
all the freed RX buffers. On non-coherent ARM, that invalidates cache
lines of pages that may already have been reused.

hip04_mac_stop() also calls phy_stop() on a PHY that phy_disconnect()
already halted, because priv->phy is not cleared. That triggers the
"called from state HALTED" warning.

This is tagged for stable as a fix for late interrupts reaching freed
memory in hip04_remove(). Would it make sense to call unregister_netdev()
first, so ndo_stop quiesces the hardware and NAPI, and only then do
phy_disconnect(), hip04_free_ring() and the rest?

>  	of_node_put(priv->phy_node);
>  	cancel_work_sync(&priv->tx_timeout_task);
> -	free_netdev(ndev);
>  }
>  

[Severity: High]
This is a pre-existing issue, but does the cancel_work_sync() here come
too late to protect against hip04_tx_timeout_task()?

hip04_tx_timeout_task() {
	...
	hip04_mac_stop(priv->ndev);
	hip04_mac_open(priv->ndev);
}

The work runs the ndo_stop and ndo_open logic without rtnl_lock() and
without a netif_running() check. Nothing in ndo_stop cancels it.

Suppose a TX watchdog timeout queued the work, and the work reaches
napi_disable() after ndo_stop has already disabled NAPI. That ndo_stop
can come from unregister_netdev() or from a plain ifdown.
napi_disable() takes netdev_lock(), and napi_disable_locked() then
waits forever:

napi_disable_locked() {
	...
		while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
			usleep_range(20, 200);
			val = READ_ONCE(n->state);
		}
	...
}

Those bits stay set on a disabled NAPI until napi_enable() is called.
The kworker then holds netdev->lock indefinitely. Either
unregister_netdevice_many_notify() blocks on netdev_lock(dev) with RTNL
held, or hip04_remove() blocks in cancel_work_sync().

The same work can also run between hip04_free_ring() and
unregister_netdev(). In that case hip04_mac_open() calls
dma_map_single() on the freed rx_buf[] entries.

Could the work be cancelled from ndo_stop, or serialized with
rtnl_lock() and a netif_running() check?

[Severity: Medium]
This is a pre-existing issue, but should priv->tx_coalesce_timer be
cancelled on this path as well?

The timer is embedded in the netdev private area. It is armed from
hip04_mac_start_xmit() and from the done: tail of hip04_rx_poll(), which
can run after napi_complete_done():

hip04_rx_poll() {
	...
	napi_complete_done(napi, rx);
done:
	/* start a new timer if necessary */
	if (rx < budget && tx_remaining)
		hip04_start_tx_timer(priv);
	...
}

Neither hip04_mac_stop() nor hip04_remove() calls hrtimer_cancel() on
it.

If the timer expires after devres frees the netdev, tx_done() does
container_of() on priv and napi_schedule_prep(&priv->napi). Both of
those touch freed memory.

The timer expires after 100-200us, and the netdev is freed well after
that. So this only happens if the timer is delayed past the whole
teardown.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927144741.1320558-1-yaojiale02%40163.com

[PATCH v2 4/7] net: hisi_femac: manage the netdev lifetime with devres

[Jiale Yao]

hisi_femac_drv_remove() frees the netdev while the shared managed IRQ
remains registered until devres cleanup. Its handler uses the netdev as
private data, so an interrupt in this window can dereference freed
memory.

Manage the netdev allocation with devres so the later IRQ resource is
released before the netdev.

Fixes: 542ae60af24f ("net: hisilicon: Add Fast Ethernet MAC driver")
Cc: stable@vger.kernel.org
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 drivers/net/ethernet/hisilicon/hisi_femac.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hisi_femac.c b/drivers/net/ethernet/hisilicon/hisi_femac.c
index d244a40df430..d369824fa4e8 100644
--- a/drivers/net/ethernet/hisilicon/hisi_femac.c
+++ b/drivers/net/ethernet/hisilicon/hisi_femac.c
@@ -774,7 +774,7 @@ static int hisi_femac_drv_probe(struct platform_device *pdev)
 	struct phy_device *phy;
 	int ret;
 
-	ndev = alloc_etherdev(sizeof(*priv));
+	ndev = devm_alloc_etherdev(dev, sizeof(*priv));
 	if (!ndev)
 		return -ENOMEM;
 
@@ -788,26 +788,26 @@ static int hisi_femac_drv_probe(struct platform_device *pdev)
 	priv->port_base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(priv->port_base)) {
 		ret = PTR_ERR(priv->port_base);
-		goto out_free_netdev;
+		goto out_return;
 	}
 
 	priv->glb_base = devm_platform_ioremap_resource(pdev, 1);
 	if (IS_ERR(priv->glb_base)) {
 		ret = PTR_ERR(priv->glb_base);
-		goto out_free_netdev;
+		goto out_return;
 	}
 
 	priv->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		dev_err(dev, "failed to get clk\n");
 		ret = -ENODEV;
-		goto out_free_netdev;
+		goto out_return;
 	}
 
 	ret = clk_prepare_enable(priv->clk);
 	if (ret) {
 		dev_err(dev, "failed to enable clk %d\n", ret);
-		goto out_free_netdev;
+		goto out_return;
 	}
 
 	priv->mac_rst = devm_reset_control_get(dev, "mac");
@@ -887,9 +887,7 @@ static int hisi_femac_drv_probe(struct platform_device *pdev)
 	phy_disconnect(phy);
 out_disable_clk:
 	clk_disable_unprepare(priv->clk);
-out_free_netdev:
-	free_netdev(ndev);
-
+out_return:
 	return ret;
 }
 
@@ -903,7 +901,6 @@ static void hisi_femac_drv_remove(struct platform_device *pdev)
 
 	phy_disconnect(ndev->phydev);
 	clk_disable_unprepare(priv->clk);
-	free_netdev(ndev);
 }
 
 #ifdef CONFIG_PM
-- 
2.34.1

[PATCH v2 5/7] net: hix5hd2: manage the netdev lifetime with devres

[Jiale Yao]

hix5hd2_dev_remove() manually frees the netdev before devres releases
the IRQ. The interrupt handler receives that netdev as its data pointer
and can access it during this teardown window.

Allocate the netdev through devres so its later registered IRQ is
released first.

Fixes: 57c5bc9ad7d7 ("net: hisilicon: add hix5hd2 mac driver")
Cc: stable@vger.kernel.org
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 drivers/net/ethernet/hisilicon/hix5hd2_gmac.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c b/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c
index 02282dc86faf..ffcb281230eb 100644
--- a/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c
+++ b/drivers/net/ethernet/hisilicon/hix5hd2_gmac.c
@@ -1100,7 +1100,7 @@ static int hix5hd2_dev_probe(struct platform_device *pdev)
 	struct mii_bus *bus;
 	int ret;
 
-	ndev = alloc_etherdev(sizeof(struct hix5hd2_priv));
+	ndev = devm_alloc_etherdev(dev, sizeof(struct hix5hd2_priv));
 	if (!ndev)
 		return -ENOMEM;
 
@@ -1115,26 +1115,26 @@ static int hix5hd2_dev_probe(struct platform_device *pdev)
 	priv->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(priv->base)) {
 		ret = PTR_ERR(priv->base);
-		goto out_free_netdev;
+		goto out_return;
 	}
 
 	priv->ctrl_base = devm_platform_ioremap_resource(pdev, 1);
 	if (IS_ERR(priv->ctrl_base)) {
 		ret = PTR_ERR(priv->ctrl_base);
-		goto out_free_netdev;
+		goto out_return;
 	}
 
 	priv->mac_core_clk = devm_clk_get(&pdev->dev, "mac_core");
 	if (IS_ERR(priv->mac_core_clk)) {
 		netdev_err(ndev, "failed to get mac core clk\n");
 		ret = -ENODEV;
-		goto out_free_netdev;
+		goto out_return;
 	}
 
 	ret = clk_prepare_enable(priv->mac_core_clk);
 	if (ret < 0) {
 		netdev_err(ndev, "failed to enable mac core clk %d\n", ret);
-		goto out_free_netdev;
+		goto out_return;
 	}
 
 	priv->mac_ifc_clk = devm_clk_get(&pdev->dev, "mac_ifc");
@@ -1271,9 +1271,7 @@ static int hix5hd2_dev_probe(struct platform_device *pdev)
 	clk_disable_unprepare(priv->mac_ifc_clk);
 out_disable_mac_core_clk:
 	clk_disable_unprepare(priv->mac_core_clk);
-out_free_netdev:
-	free_netdev(ndev);
-
+out_return:
 	return ret;
 }
 
@@ -1291,7 +1289,6 @@ static void hix5hd2_dev_remove(struct platform_device *pdev)
 	hix5hd2_destroy_hw_desc_queue(priv);
 	of_node_put(priv->phy_node);
 	cancel_work_sync(&priv->tx_timeout_task);
-	free_netdev(ndev);
 }
 
 static const struct of_device_id hix5hd2_of_match[] = {
-- 
2.34.1

[PATCH v2 6/7] net: ravb: fix resource teardown ordering

[Jiale Yao]

ravb_remove() frees the netdev before devres releases the managed IRQs.
The handlers use the netdev as their data pointer, so an interrupt during
that window can access freed memory. Probe error paths have the same
ordering problem.

The remove callback also returns when runtime resume fails. That leaves
the netdev registered while the driver core still releases its managed
resources. A running interface already holds a runtime PM reference, so
the extra get cannot invoke a failing resume. A resume failure therefore
occurs while the interface is down and ndo_stop() will not be called.

Place the IRQ resources in a dedicated devres group and release it before
freeing the netdev. Continue unregistering and freeing software resources
when runtime resume fails, but skip the unmatched runtime PM put.

Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
Fixes: 48f894ab07c4 ("net: ravb: Add runtime PM support")
Cc: stable@vger.kernel.org
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index ea1c7e536791..a25f5ac7062f 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2963,28 +2963,35 @@ static int ravb_probe(struct platform_device *pdev)
 		priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
 	}
 
+	if (!devres_open_group(&pdev->dev, priv, GFP_KERNEL)) {
+		error = -ENOMEM;
+		goto out_reset_assert;
+	}
+
 	error = ravb_setup_irqs(priv);
 	if (error)
-		goto out_reset_assert;
+		goto out_release_irqs;
+
+	devres_close_group(&pdev->dev, priv);
 
 	priv->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		error = PTR_ERR(priv->clk);
-		goto out_reset_assert;
+		goto out_release_irqs;
 	}
 
 	if (info->gptp_ref_clk) {
 		priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp");
 		if (IS_ERR(priv->gptp_clk)) {
 			error = PTR_ERR(priv->gptp_clk);
-			goto out_reset_assert;
+			goto out_release_irqs;
 		}
 	}
 
 	priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
 	if (IS_ERR(priv->refclk)) {
 		error = PTR_ERR(priv->refclk);
-		goto out_reset_assert;
+		goto out_release_irqs;
 	}
 	clk_prepare(priv->refclk);
 
@@ -3124,6 +3131,8 @@ static int ravb_probe(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	clk_unprepare(priv->refclk);
+out_release_irqs:
+	devres_release_group(&pdev->dev, priv);
 out_reset_assert:
 	reset_control_assert(rstc);
 out_free_netdev:
@@ -3141,9 +3150,10 @@ static void ravb_remove(struct platform_device *pdev)
 
 	error = pm_runtime_resume_and_get(dev);
 	if (error < 0)
-		return;
+		dev_warn(dev, "failed to resume device: %d\n", error);
 
 	unregister_netdev(ndev);
+	devres_release_group(dev, priv);
 	if (info->nc_queues)
 		netif_napi_del(&priv->napi[RAVB_NC]);
 	netif_napi_del(&priv->napi[RAVB_BE]);
@@ -3153,7 +3163,8 @@ static void ravb_remove(struct platform_device *pdev)
 	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
 			  priv->desc_bat_dma);
 
-	pm_runtime_put_sync_suspend(&pdev->dev);
+	if (error >= 0)
+		pm_runtime_put_sync_suspend(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(dev);
 	clk_unprepare(priv->refclk);
-- 
2.34.1

Re: [PATCH v2 6/7] net: ravb: fix resource teardown ordering

[Niklas Söderlund]

Hi Jiale,

On 2026-09-27 22:47:39 +0800, Jiale Yao wrote:
> ravb_remove() frees the netdev before devres releases the managed IRQs.
> The handlers use the netdev as their data pointer, so an interrupt during
> that window can access freed memory. Probe error paths have the same
> ordering problem.
> 
> The remove callback also returns when runtime resume fails. That leaves
> the netdev registered while the driver core still releases its managed
> resources. A running interface already holds a runtime PM reference, so
> the extra get cannot invoke a failing resume. A resume failure therefore
> occurs while the interface is down and ndo_stop() will not be called.
> 
> Place the IRQ resources in a dedicated devres group and release it before
> freeing the netdev. Continue unregistering and freeing software resources
> when runtime resume fails, but skip the unmatched runtime PM put.

Would it not make more sens to rework the driver to allocate the ndev 
using devm too instead of adding a complex devres group? AFIK 
s/alloc_etherdev_mqs/devm_alloc_etherdev_mqs/ would allocate the ndev 
with devm too?

> 
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> Fixes: 48f894ab07c4 ("net: ravb: Add runtime PM support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jiale Yao <yaojiale02@163.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ea1c7e536791..a25f5ac7062f 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2963,28 +2963,35 @@ static int ravb_probe(struct platform_device *pdev)
>  		priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
>  	}
>  
> +	if (!devres_open_group(&pdev->dev, priv, GFP_KERNEL)) {
> +		error = -ENOMEM;
> +		goto out_reset_assert;
> +	}
> +
>  	error = ravb_setup_irqs(priv);
>  	if (error)
> -		goto out_reset_assert;
> +		goto out_release_irqs;
> +
> +	devres_close_group(&pdev->dev, priv);
>  
>  	priv->clk = devm_clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(priv->clk)) {
>  		error = PTR_ERR(priv->clk);
> -		goto out_reset_assert;
> +		goto out_release_irqs;
>  	}
>  
>  	if (info->gptp_ref_clk) {
>  		priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp");
>  		if (IS_ERR(priv->gptp_clk)) {
>  			error = PTR_ERR(priv->gptp_clk);
> -			goto out_reset_assert;
> +			goto out_release_irqs;
>  		}
>  	}
>  
>  	priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
>  	if (IS_ERR(priv->refclk)) {
>  		error = PTR_ERR(priv->refclk);
> -		goto out_reset_assert;
> +		goto out_release_irqs;
>  	}
>  	clk_prepare(priv->refclk);
>  
> @@ -3124,6 +3131,8 @@ static int ravb_probe(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	clk_unprepare(priv->refclk);
> +out_release_irqs:
> +	devres_release_group(&pdev->dev, priv);
>  out_reset_assert:
>  	reset_control_assert(rstc);
>  out_free_netdev:
> @@ -3141,9 +3150,10 @@ static void ravb_remove(struct platform_device *pdev)
>  
>  	error = pm_runtime_resume_and_get(dev);
>  	if (error < 0)
> -		return;
> +		dev_warn(dev, "failed to resume device: %d\n", error);
>  
>  	unregister_netdev(ndev);
> +	devres_release_group(dev, priv);
>  	if (info->nc_queues)
>  		netif_napi_del(&priv->napi[RAVB_NC]);
>  	netif_napi_del(&priv->napi[RAVB_BE]);
> @@ -3153,7 +3163,8 @@ static void ravb_remove(struct platform_device *pdev)
>  	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
>  			  priv->desc_bat_dma);
>  
> -	pm_runtime_put_sync_suspend(&pdev->dev);
> +	if (error >= 0)
> +		pm_runtime_put_sync_suspend(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(dev);
>  	clk_unprepare(priv->refclk);
> -- 
> 2.34.1
> 

-- 
Kind Regards,
Niklas Söderlund

Re:Re: [PATCH v2 6/7] net: ravb: fix resource teardown ordering

[jiale yao]

Hi,
At 2026-09-28 00:01:34, "Niklas Söderlund" <niklas.soderlund@ragnatech.se> wrote:
>Hi Jiale,
>
>On 2026-09-27 22:47:39 +0800, Jiale Yao wrote:
>> ravb_remove() frees the netdev before devres releases the managed IRQs.
>> The handlers use the netdev as their data pointer, so an interrupt during
>> that window can access freed memory. Probe error paths have the same
>> ordering problem.
>> 
>> The remove callback also returns when runtime resume fails. That leaves
>> the netdev registered while the driver core still releases its managed
>> resources. A running interface already holds a runtime PM reference, so
>> the extra get cannot invoke a failing resume. A resume failure therefore
>> occurs while the interface is down and ndo_stop() will not be called.
>> 
>> Place the IRQ resources in a dedicated devres group and release it before
>> freeing the netdev. Continue unregistering and freeing software resources
>> when runtime resume fails, but skip the unmatched runtime PM put.
>
>Would it not make more sens to rework the driver to allocate the ndev 
>using devm too instead of adding a complex devres group? AFIK 
>s/alloc_etherdev_mqs/devm_alloc_etherdev_mqs/ would allocate the ndev 
>with devm too?

You are right, I made things complicated. I will rework the ravb patch and
drop the devres group.

>
>> 
>> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
>> Fixes: 48f894ab07c4 ("net: ravb: Add runtime PM support")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jiale Yao <yaojiale02@163.com>
>> ---
>>  drivers/net/ethernet/renesas/ravb_main.c | 23 +++++++++++++++++------
>>  1 file changed, 17 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index ea1c7e536791..a25f5ac7062f 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2963,28 +2963,35 @@ static int ravb_probe(struct platform_device *pdev)
>>  		priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
>>  	}
>>  
>> +	if (!devres_open_group(&pdev->dev, priv, GFP_KERNEL)) {
>> +		error = -ENOMEM;
>> +		goto out_reset_assert;
>> +	}
>> +
>>  	error = ravb_setup_irqs(priv);
>>  	if (error)
>> -		goto out_reset_assert;
>> +		goto out_release_irqs;
>> +
>> +	devres_close_group(&pdev->dev, priv);
>>  
>>  	priv->clk = devm_clk_get(&pdev->dev, NULL);
>>  	if (IS_ERR(priv->clk)) {
>>  		error = PTR_ERR(priv->clk);
>> -		goto out_reset_assert;
>> +		goto out_release_irqs;
>>  	}
>>  
>>  	if (info->gptp_ref_clk) {
>>  		priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp");
>>  		if (IS_ERR(priv->gptp_clk)) {
>>  			error = PTR_ERR(priv->gptp_clk);
>> -			goto out_reset_assert;
>> +			goto out_release_irqs;
>>  		}
>>  	}
>>  
>>  	priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
>>  	if (IS_ERR(priv->refclk)) {
>>  		error = PTR_ERR(priv->refclk);
>> -		goto out_reset_assert;
>> +		goto out_release_irqs;
>>  	}
>>  	clk_prepare(priv->refclk);
>>  
>> @@ -3124,6 +3131,8 @@ static int ravb_probe(struct platform_device *pdev)
>>  	pm_runtime_disable(&pdev->dev);
>>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>>  	clk_unprepare(priv->refclk);
>> +out_release_irqs:
>> +	devres_release_group(&pdev->dev, priv);
>>  out_reset_assert:
>>  	reset_control_assert(rstc);
>>  out_free_netdev:
>> @@ -3141,9 +3150,10 @@ static void ravb_remove(struct platform_device *pdev)
>>  
>>  	error = pm_runtime_resume_and_get(dev);
>>  	if (error < 0)
>> -		return;
>> +		dev_warn(dev, "failed to resume device: %d\n", error);
>>  
>>  	unregister_netdev(ndev);
>> +	devres_release_group(dev, priv);
>>  	if (info->nc_queues)
>>  		netif_napi_del(&priv->napi[RAVB_NC]);
>>  	netif_napi_del(&priv->napi[RAVB_BE]);
>> @@ -3153,7 +3163,8 @@ static void ravb_remove(struct platform_device *pdev)
>>  	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
>>  			  priv->desc_bat_dma);
>>  
>> -	pm_runtime_put_sync_suspend(&pdev->dev);
>> +	if (error >= 0)
>> +		pm_runtime_put_sync_suspend(&pdev->dev);
>>  	pm_runtime_disable(&pdev->dev);
>>  	pm_runtime_dont_use_autosuspend(dev);
>>  	clk_unprepare(priv->refclk);
>> -- 
>> 2.34.1
>> 
>
>-- 
>Kind Regards,
>Niklas Söderlund

Re: [PATCH v2 6/7] net: ravb: fix resource teardown ordering

[Sergey Shtylyov]

Hello!

   I guess you used scripts/get_maintainer.pl -- if so, I suggest that
you add --no-git-fallback next time. Your current To: list is painfully
long and contains some long defunct addresses (like mine)...

On 9/27/26 5:47 PM, Jiale Yao wrote:

> ravb_remove() frees the netdev before devres releases the managed IRQs.
> The handlers use the netdev as their data pointer, so an interrupt during
> that window can access freed memory. Probe error paths have the same
> ordering problem.
> 
> The remove callback also returns when runtime resume fails. That leaves
> the netdev registered while the driver core still releases its managed
> resources. A running interface already holds a runtime PM reference, so
> the extra get cannot invoke a failing resume. A resume failure therefore
> occurs while the interface is down and ndo_stop() will not be called.

   Seems like a separate problem?

> Place the IRQ resources in a dedicated devres group and release it before
> freeing the netdev. Continue unregistering and freeing software resources
> when runtime resume fails, but skip the unmatched runtime PM put.
> 
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> Fixes: 48f894ab07c4 ("net: ravb: Add runtime PM support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jiale Yao <yaojiale02@163.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ea1c7e536791..a25f5ac7062f 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]> @@ -3124,6 +3131,8 @@ static int ravb_probe(struct platform_device *pdev)
>         pm_runtime_disable(&pdev->dev);
>         pm_runtime_dont_use_autosuspend(&pdev->dev);
>         clk_unprepare(priv->refclk);
> +out_release_irqs:

   Somewhat unobvious label name, given the following call...

> +       devres_release_group(&pdev->dev, priv);
>  out_reset_assert:
>         reset_control_assert(rstc);
>  out_free_netdev:
> @@ -3141,9 +3150,10 @@ static void ravb_remove(struct platform_device *pdev)
> 
>         error = pm_runtime_resume_and_get(dev);
>         if (error < 0)
> -               return;
> +               dev_warn(dev, "failed to resume device: %d\n", error);
> 
>         unregister_netdev(ndev);
> +       devres_release_group(dev, priv);
>         if (info->nc_queues)
>                 netif_napi_del(&priv->napi[RAVB_NC]);
>         netif_napi_del(&priv->napi[RAVB_BE]);
> @@ -3153,7 +3163,8 @@ static void ravb_remove(struct platform_device *pdev)
>         dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
>                           priv->desc_bat_dma);
> 
> -       pm_runtime_put_sync_suspend(&pdev->dev);
> +       if (error >= 0)
> +               pm_runtime_put_sync_suspend(&pdev->dev);

   Hm, definitely seems like a material for a separate patch...

[...]

MBR, Sergey

Re:Re: [PATCH v2 6/7] net: ravb: fix resource teardown ordering

[jiale yao]

Hi Sergey,

Thanks for your review.
You're right — I'll split this into two patches:
One patch for the IRQ/netdev ordering issue.
One patch for the runtime PM resume failure handling.
I'll use --no-git-fallback next time, sorry for this.

At 2026-09-30 03:07:52, "Sergey Shtylyov" <s.shtylyov@omp.ru> wrote:
>Hello!
>
>   I guess you used scripts/get_maintainer.pl -- if so, I suggest that
>you add --no-git-fallback next time. Your current To: list is painfully
>long and contains some long defunct addresses (like mine)...
>
>On 9/27/26 5:47 PM, Jiale Yao wrote:
>
>> ravb_remove() frees the netdev before devres releases the managed IRQs.
>> The handlers use the netdev as their data pointer, so an interrupt during
>> that window can access freed memory. Probe error paths have the same
>> ordering problem.
>> 
>> The remove callback also returns when runtime resume fails. That leaves
>> the netdev registered while the driver core still releases its managed
>> resources. A running interface already holds a runtime PM reference, so
>> the extra get cannot invoke a failing resume. A resume failure therefore
>> occurs while the interface is down and ndo_stop() will not be called.
>
>   Seems like a separate problem?
>
>> Place the IRQ resources in a dedicated devres group and release it before
>> freeing the netdev. Continue unregistering and freeing software resources
>> when runtime resume fails, but skip the unmatched runtime PM put.
>> 
>> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
>> Fixes: 48f894ab07c4 ("net: ravb: Add runtime PM support")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jiale Yao <yaojiale02@163.com>
>> ---
>>  drivers/net/ethernet/renesas/ravb_main.c | 23 +++++++++++++++++------
>>  1 file changed, 17 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index ea1c7e536791..a25f5ac7062f 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>[...]> @@ -3124,6 +3131,8 @@ static int ravb_probe(struct platform_device *pdev)
>>         pm_runtime_disable(&pdev->dev);
>>         pm_runtime_dont_use_autosuspend(&pdev->dev);
>>         clk_unprepare(priv->refclk);
>> +out_release_irqs:
>
>   Somewhat unobvious label name, given the following call...
>
>> +       devres_release_group(&pdev->dev, priv);
>>  out_reset_assert:
>>         reset_control_assert(rstc);
>>  out_free_netdev:
>> @@ -3141,9 +3150,10 @@ static void ravb_remove(struct platform_device *pdev)
>> 
>>         error = pm_runtime_resume_and_get(dev);
>>         if (error < 0)
>> -               return;
>> +               dev_warn(dev, "failed to resume device: %d\n", error);
>> 
>>         unregister_netdev(ndev);
>> +       devres_release_group(dev, priv);
>>         if (info->nc_queues)
>>                 netif_napi_del(&priv->napi[RAVB_NC]);
>>         netif_napi_del(&priv->napi[RAVB_BE]);
>> @@ -3153,7 +3163,8 @@ static void ravb_remove(struct platform_device *pdev)
>>         dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
>>                           priv->desc_bat_dma);
>> 
>> -       pm_runtime_put_sync_suspend(&pdev->dev);
>> +       if (error >= 0)
>> +               pm_runtime_put_sync_suspend(&pdev->dev);
>
>   Hm, definitely seems like a material for a separate patch...
>
>[...]
>
>MBR, Sergey
Jiale

Re: [PATCH v2 6/7] net: ravb: fix resource teardown ordering

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch rests on the commit-message claim that "A running
  interface already holds a runtime PM reference ... A resume failure…
- [Medium] The commit says it fixes the probe error paths, but in the
  early ones the IRQ handler can still make an invalid access.
- [Medium] When pm_runtime_resume_and_get() fails in ravb_remove(), the
  patched code now carries on to ravb_mdio_release() (line 3161) instead…
- [Low] The first Fixes: tag is wrong.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but ravb_close()
  returns early when ravb_set_opmode(ndev, CCC_OPC_RESET) fails (lines…
- [Medium] This problem wasn't introduced by this patch, but if
  ravb_set_opmode(ndev, CCC_OPC_CONFIG) fails in ravb_probe() (lines…
- [Medium] This problem wasn't introduced by this patch, but
  ravb_interrupt() (and the other ravb handlers) call…
- [Low] This problem wasn't introduced by this patch, but the
  ravb_probe() error path calls pm_runtime_put(&pdev->dev) (async, line…

--

net: ravb: fix resource teardown ordering

This patch puts the ravb IRQs in a dedicated devres group. The group is
released before free_netdev() in ravb_remove() and in the ravb_probe()
error paths. ravb_remove() also keeps tearing down when
pm_runtime_resume_and_get() fails, and skips only the matching runtime
PM put.

> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")

[Severity: Low]
Is c156633f1353 the right commit for the first Fixes: tag?

In c156633f1353 the open path took the IRQ with request_irq(), and the
close path released it with free_irq(ndev->irq, ndev). So
unregister_netdev() had already freed it before free_netdev() ran.

The window comes from the devm-managed IRQ that uses ndev as dev_id, in
ravb_setup_irq():

    error = devm_request_irq(dev, irq_num, handler, flags, devname, ndev);

git blame points at 32f012b8c01c ("net: ravb: Move getting/requesting
IRQs in the probe() method"). Should the tag be this instead?

Fixes: 32f012b8c01c ("net: ravb: Move getting/requesting IRQs in the probe() method")

The current tag selects stable trees back to v4.2. Those trees have no
ravb_setup_irqs() and free the IRQs in ndo_stop. The second tag,
48f894ab07c4, looks right for the early return in ravb_remove().

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ea1c7e536791e..a25f5ac7062fc 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2963,28 +2963,35 @@ static int ravb_probe(struct platform_device *pdev)
>  		priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
>  	}
>  
> +	if (!devres_open_group(&pdev->dev, priv, GFP_KERNEL)) {
> +		error = -ENOMEM;
> +		goto out_reset_assert;
> +	}
> +
>  	error = ravb_setup_irqs(priv);
>  	if (error)
> -		goto out_reset_assert;
> +		goto out_release_irqs;
> +
> +	devres_close_group(&pdev->dev, priv);
>  
>  	priv->clk = devm_clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(priv->clk)) {
>  		error = PTR_ERR(priv->clk);
> -		goto out_reset_assert;
> +		goto out_release_irqs;
>  	}

[Severity: Medium]
Can the IRQ handler still make an invalid access when this path reaches
out_release_irqs?

The IRQs are live once devres_close_group() returns. ravb_probe() maps
priv->addr and initialises priv->lock only later:

    priv->addr = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
    ...
    spin_lock_init(&priv->lock);

These failures reach devres_release_group() while priv->addr is still
NULL:

- the clock lookup failures here, including -EPROBE_DEFER
- the pm_runtime_resume_and_get() failure

The ioremap failure also reaches it, with an ERR_PTR in priv->addr.

On single-IRQ variants, ravb_setup_irq() requests the line with
IRQF_SHARED. Under CONFIG_DEBUG_SHIRQ, __free_irq() calls the handler of
a shared action:

kernel/irq/manage.c:__free_irq() {
    ...
	if (action->flags & IRQF_SHARED) {
		local_irq_save(flags);
		action->handler(irq, dev_id);
		local_irq_restore(flags);
	}
    ...
}

A real interrupt from another device on the same line would do the
same.

ravb_interrupt() only checks pm_runtime_active(). That returns true when
runtime PM is not enabled yet, and also after out_rpm_disable has
disabled it:

ravb_interrupt() {
    ...
	pm_runtime_get_noresume(dev);

	if (unlikely(!pm_runtime_active(dev)))
		goto out_rpm_put;

	spin_lock(&priv->lock);
	/* Get interrupt status */
	iss = ravb_read(ndev, ISS);
    ...
}

The handler then takes an uninitialised spinlock and does ioread32()
through a NULL or ERR_PTR base. Before this patch the same call hit a
freed ndev.

Would it be better to request the IRQs after the MMIO mapping and
spin_lock_init()?

[ ... ]

> @@ -3124,6 +3131,8 @@ static int ravb_probe(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);

[Severity: Low]
This isn't a bug introduced by this patch, but the out_rpm_put label just
above calls the async put and then disables runtime PM right away:

out_rpm_put:
	pm_runtime_put(&pdev->dev);
out_rpm_disable:
	pm_runtime_disable(&pdev->dev);

Can __pm_runtime_barrier() in pm_runtime_disable() cancel the queued
idle request? If so, ravb_runtime_suspend() and the genpd callbacks
never run before probe fails.

ravb_remove() already uses pm_runtime_put_sync_suspend() here. Should the
probe error path use a synchronous put too?

[Severity: Medium]
This is a pre-existing issue, but does ravb_probe() leak priv->desc_bat
when ravb_set_opmode(ndev, CCC_OPC_CONFIG) fails?

	error = ravb_set_opmode(ndev, CCC_OPC_CONFIG);
	if (error)
		goto out_rpm_put;

In the error path, the only dma_free_coherent() of desc_bat is under
out_reset_mode, which comes before out_rpm_put:

out_reset_mode:
	ravb_set_opmode(ndev, CCC_OPC_RESET);
	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
			  priv->desc_bat_dma);
out_rpm_put:

The allocation is not devres-managed and is not in the new IRQ group.

>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	clk_unprepare(priv->refclk);
> +out_release_irqs:
> +	devres_release_group(&pdev->dev, priv);
>  out_reset_assert:
>  	reset_control_assert(rstc);
>  out_free_netdev:
> @@ -3141,9 +3150,10 @@ static void ravb_remove(struct platform_device *pdev)
>  
>  	error = pm_runtime_resume_and_get(dev);
>  	if (error < 0)
> -		return;
> +		dev_warn(dev, "failed to resume device: %d\n", error);
>  
>  	unregister_netdev(ndev);

[Severity: High]
Can unregister_netdev() call ravb_close() a second time when the resume
above fails?

The commit message says:

  A running interface already holds a runtime PM reference, so the extra
  get cannot invoke a failing resume. A resume failure therefore occurs
  while the interface is down and ndo_stop() will not be called.

A failed system resume seems to break that assumption. With WoL
disabled, ravb_suspend() closes the interface directly and leaves
IFF_UP set:

ravb_suspend()
    netif_device_detach(ndev);
    ravb_close(ndev);
        napi_disable(&priv->napi[RAVB_BE]);
        pm_runtime_put_autosuspend(dev);
    pm_runtime_force_suspend(&priv->pdev->dev);
    reset_control_assert(priv->rstc);

If ravb_resume() then fails in reset_control_deassert(), it returns
before pm_runtime_force_resume() and ravb_open():

	ret = reset_control_deassert(priv->rstc);
	if (ret)
		return ret;

That leaves runtime PM disabled with RPM_SUSPENDED status. The
interface is still IFF_UP but closed, NAPI is disabled, and no PM
reference is held. A pm_runtime_force_resume() failure leaves a similar
state.

On a later unbind, pm_runtime_resume_and_get() here returns -EACCES
every time. Before this patch ravb_remove() returned at that point. It
now continues into unregister_netdev().

__dev_close_many() calls ndo_stop for any IFF_UP device, even after
netif_device_detach().

The second ravb_close() then does the following on an IP that is
clock-gated and held in reset:

- writes RIC0/RIC2/TIC/ECSIPR
- runs ravb_ptp_stop() and ravb_stop_dma()

It then calls napi_disable() on a NAPI instance that was never
re-enabled, and napi_disable_locked() loops forever:

	while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
		usleep_range(20, 200);
		val = READ_ONCE(n->state);
	}

Wouldn't that hang removal with RTNL and the netdev instance lock held?

Past that point, ravb_ring_free() would call page_pool_destroy() again
on the stale priv->rx_pool[q]. pm_runtime_put_autosuspend() would also
underflow the usage count.

Also, with runtime PM disabled, pm_runtime_active() returns true. The
shared-IRQ handlers are then no longer kept off the unpowered hardware
until devres_release_group() runs.

A similar double close could already happen when the resume here
succeeds after a failed system resume. The failing-resume path is new
with this patch. The if (error >= 0) check covers only ravb_remove()'s
own put, not these other states.

> +	devres_release_group(dev, priv);

[Severity: Medium]
This isn't a bug introduced by this patch, but is the runtime PM check in
the ravb IRQ handlers safe against a concurrent runtime suspend?

ravb_interrupt() and the other ravb handlers do this without holding
dev->power.lock:

	pm_runtime_get_noresume(dev);

	if (unlikely(!pm_runtime_active(dev)))
		goto out_rpm_put;

rpm_suspend() checks usage_count and sets RPM_SUSPENDING within one
dev->power.lock critical section. The handler's increment and status
read can both fall in the window between those two steps.

The handler then reads ISS and other registers while
ravb_runtime_suspend() calls clk_disable(priv->refclk) and genpd gates
the clocks.

ravb_runtime_suspend() also has no synchronize_irq(), even though the
main IRQ is IRQF_SHARED.

Would using pm_runtime_get_if_active() in the handlers and adding
synchronize_irq() to ravb_runtime_suspend() close this?

>  	if (info->nc_queues)
>  		netif_napi_del(&priv->napi[RAVB_NC]);
>  	netif_napi_del(&priv->napi[RAVB_BE]);

[Severity: Medium]
When pm_runtime_resume_and_get() has failed, what happens to the MDIO
accesses in the ravb_mdio_release() call that follows?

ravb_mdio_release() calls mdiobus_unregister(), and PHY teardown issues
MDIO transactions:

ravb_mdio_release()
  mdiobus_unregister()
    phy_remove()
      phy_leds_unregister()
        led_classdev_unregister()
          led_set_brightness(led_cdev, LED_OFF)
      phydev->drv->remove(phydev)

On ravb, MDIO is bit-banged through the AVB PIR register:

ravb_mdio_ctrl()
	ravb_modify(priv->ndev, PIR, mask, set ? mask : 0);

ravb_get_mdio_data()
	return (ravb_read(priv->ndev, PIR) & PIR_MDI) != 0;

Because the resume failed, the module clock/refclk is still gated. After
a failed system resume the reset may also still be asserted.

In that state, writes to PIR would be lost. Reads would return garbage
that mdio-bitbang treats as valid. On SoCs that fault on access to a
stopped module, this could raise a bus error.

The commit message only argues that ndo_stop() will not be called.
Should this path skip or otherwise handle the register accesses made by
MDIO teardown?

> @@ -3153,7 +3163,8 @@ static void ravb_remove(struct platform_device *pdev)
>  	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
>  			  priv->desc_bat_dma);
>  
> -	pm_runtime_put_sync_suspend(&pdev->dev);
> +	if (error >= 0)
> +		pm_runtime_put_sync_suspend(&pdev->dev);

[Severity: Medium]
This is a pre-existing issue, but ravb_close() returns early when the
switch to reset opmode fails:

	error = ravb_set_opmode(ndev, CCC_OPC_RESET);
	if (error)
		return error;

	pm_runtime_put_autosuspend(dev);

Does that leak the runtime PM reference taken by
pm_runtime_resume_and_get() in ravb_open()?

The core ignores the return value of ndo_stop. The interface goes down
while the device stays RPM_ACTIVE.

The get/put pair here in ravb_remove() only balances its own reference.
The device would then never be runtime-suspended before
pm_runtime_disable().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927144741.1320558-1-yaojiale02%40163.com

[PATCH v2 7/7] net: sxgbe: manage IRQ data lifetimes with devres

[Jiale Yao]

sxgbe_drv_remove() frees the netdev and hardware operations while
managed IRQs remain registered until the remove callback returns. The
handlers dereference these objects, so an interrupt in that window can
access freed memory.

Allocate both objects with devres. They are acquired before the IRQs and
are consequently released only after the IRQ resources have been
removed.

Fixes: 1edb9ca69e8a ("net: sxgbe: add basic framework for Samsung 10Gb ethernet driver")
Cc: stable@vger.kernel.org
Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 .../net/ethernet/samsung/sxgbe/sxgbe_main.c   | 26 +++++++------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
index 70cf3619555f..ada851477302 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
@@ -2007,7 +2007,7 @@ static int sxgbe_hw_init(struct sxgbe_priv_data * const priv)
 {
 	u32 ctrl_ids;
 
-	priv->hw = kmalloc_obj(*priv->hw);
+	priv->hw = devm_kmalloc(priv->device, sizeof(*priv->hw), GFP_KERNEL);
 	if(!priv->hw)
 		return -ENOMEM;
 
@@ -2058,7 +2058,7 @@ static int sxgbe_sw_reset(void __iomem *addr)
  * @plat_dat: platform data pointer
  * @addr: iobase memory address
  * Description: this is the main probe function used to
- * call the alloc_etherdev, allocate the priv structure.
+ * allocate the netdev and priv structure.
  */
 struct sxgbe_priv_data *sxgbe_drv_probe(struct device *device,
 					struct sxgbe_plat_data *plat_dat,
@@ -2069,8 +2069,8 @@ struct sxgbe_priv_data *sxgbe_drv_probe(struct device *device,
 	int ret;
 	u8 queue_num;
 
-	ndev = alloc_etherdev_mqs(sizeof(struct sxgbe_priv_data),
-				  SXGBE_TX_QUEUES, SXGBE_RX_QUEUES);
+	ndev = devm_alloc_etherdev_mqs(device, sizeof(struct sxgbe_priv_data),
+				       SXGBE_TX_QUEUES, SXGBE_RX_QUEUES);
 	if (!ndev)
 		return NULL;
 
@@ -2086,7 +2086,7 @@ struct sxgbe_priv_data *sxgbe_drv_probe(struct device *device,
 
 	ret = sxgbe_sw_reset(priv->ioaddr);
 	if (ret)
-		goto error_free_netdev;
+		goto error_return;
 
 	/* Verify driver arguments */
 	sxgbe_verify_args();
@@ -2094,16 +2094,16 @@ struct sxgbe_priv_data *sxgbe_drv_probe(struct device *device,
 	/* Init MAC and get the capabilities */
 	ret = sxgbe_hw_init(priv);
 	if (ret)
-		goto error_free_netdev;
+		goto error_return;
 
 	/* allocate memory resources for Descriptor rings */
 	ret = txring_mem_alloc(priv);
 	if (ret)
-		goto error_free_hw;
+		goto error_return;
 
 	ret = rxring_mem_alloc(priv);
 	if (ret)
-		goto error_free_hw;
+		goto error_return;
 
 	ndev->netdev_ops = &sxgbe_netdev_ops;
 
@@ -2191,11 +2191,7 @@ struct sxgbe_priv_data *sxgbe_drv_probe(struct device *device,
 	clk_put(priv->sxgbe_clk);
 error_napi_del:
 	netif_napi_del(&priv->napi);
-error_free_hw:
-	kfree(priv->hw);
-error_free_netdev:
-	free_netdev(ndev);
-
+error_return:
 	return NULL;
 }
 
@@ -2229,10 +2225,6 @@ void sxgbe_drv_remove(struct net_device *ndev)
 	clk_put(priv->sxgbe_clk);
 
 	netif_napi_del(&priv->napi);
-
-	kfree(priv->hw);
-
-	free_netdev(ndev);
 }
 
 #ifdef CONFIG_PM
-- 
2.34.1

Re: [PATCH v2 7/7] net: sxgbe: manage IRQ data lifetimes with devres

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch closes the remove-time IRQ use-after-free for only two
  objects: the netdev and priv->hw.
- [High] The device is never quiesced before the remove window the commit
  claims to close.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  sxgbe_release() calls timer_delete_sync(&priv->eee_ctrl_timer) (line…
- [Medium] This problem wasn't introduced by this patch, but
  init_tx_ring() allocates tx_skbuff_dma and tx_skbuff with…

--

net: sxgbe: manage IRQ data lifetimes with devres

This commit switches the netdev and priv->hw allocations in
sxgbe_drv_probe() to devres, so they are freed after the managed IRQs.
It removes the matching kfree()/free_netdev() calls from the probe error
path and from sxgbe_drv_remove().

> diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
> index 70cf3619555f9..ada851477302e 100644
> --- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
> +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c

[ ... ]

> @@ -2069,8 +2069,8 @@ struct sxgbe_priv_data *sxgbe_drv_probe(struct device *device,
>  	int ret;
>  	u8 queue_num;
>  
> -	ndev = alloc_etherdev_mqs(sizeof(struct sxgbe_priv_data),
> -				  SXGBE_TX_QUEUES, SXGBE_RX_QUEUES);
> +	ndev = devm_alloc_etherdev_mqs(device, sizeof(struct sxgbe_priv_data),
> +				       SXGBE_TX_QUEUES, SXGBE_RX_QUEUES);

[Severity: High]
This isn't a bug introduced by this patch, but eee_ctrl_timer lives in the
netdev private area and can still be pending when that memory is freed.
After this patch the free happens in devm_free_netdev() instead of
free_netdev().

sxgbe_release() deletes the timer before it disables NAPI:

    if (priv->eee_enabled)
        timer_delete_sync(&priv->eee_ctrl_timer);
    ...
    napi_disable(&priv->napi);

The RX/TX interrupts are still enabled between those two calls, so
sxgbe_poll()->sxgbe_tx_all_clean() can re-arm the timer:

    if ((priv->eee_enabled) && (!priv->tx_path_in_lpi_mode)) {
        sxgbe_enable_eee_mode(priv);
        mod_timer(&priv->eee_ctrl_timer, SXGBE_LPI_TIMER(eee_timer));
    }

The phy_stop()/phy_disconnect() calls in between make the window wider.

sxgbe_eee_ctrl_timer() re-arms itself every time it runs. Nothing in
sxgbe_drv_remove() or devres deletes the timer again.

Can the timer fire on freed memory after unbind? Until unbind, it also keeps
calling sxgbe_enable_eee_mode(), which touches MMIO after
clk_disable_unprepare() in sxgbe_release().

>  	if (!ndev)
>  		return NULL;
>  

[ ... ]

> @@ -2229,10 +2225,6 @@ void sxgbe_drv_remove(struct net_device *ndev)
>  	clk_put(priv->sxgbe_clk);
>  
>  	netif_napi_del(&priv->napi);
> -
> -	kfree(priv->hw);
> -
> -	free_netdev(ndev);
>  }

[Severity: High]
Does this close the window described in the commit message ("an interrupt
in that window can access freed memory")? The netdev and priv->hw are now
freed after the devres IRQ release. However, the IRQs are still only
released at unbind.

sxgbe_open() requests every line with devres:

    ret = devm_request_irq(priv->device, priv->irq, sxgbe_common_interrupt,
                           IRQF_SHARED, dev->name, dev);
    ...
    ret = devm_request_irq(priv->device,
                           (priv->txq[queue_num])->irq_no,
                           sxgbe_tx_interrupt, 0,
                           dev->name, priv->txq[queue_num]);

Nothing in sxgbe_release() calls free_irq(), devm_free_irq() or
synchronize_irq(). So the handlers are still live when sxgbe_drv_remove()
tears down the rings:

sxgbe_drv_remove()
  unregister_netdev()
    sxgbe_release()
      priv->hw->dma->stop_tx()
      free_dma_desc_resources()

stop_tx() raises TPS because TSE is part of SXGBE_DMA_ENA_INT.
sxgbe_tx_dma_int_status() reports TPS as tx_hard_error, so
sxgbe_tx_interrupt() calls sxgbe_restart_tx_queue():

    /* free the skbuffs of the ring */
    tx_free_ring_skbufs(tx_ring);
    ...
    priv->hw->dma->start_tx_queue(priv->ioaddr, queue_num);

No lock separates tx_free_ring_skbufs() from
free_dma_desc_resources()->dma_free_tx_skbufs(). Can both of them call
dev_kfree_skb_any() on the same tx_skbuff[i]?

Now suppose the interrupt lands right after stop_tx(). Can start_tx_queue()
re-enable a channel whose descriptor ring free_dma_desc_resources() frees
next? The device would then DMA to freed coherent memory.

The devm IRQs also survive ifdown. A second ifup runs init_tx_ring() again
and allocates new devm tx_skbuff and tx_skbuff_dma arrays. These are newer
than the TX IRQ devres entries from the first open, so devres frees them
before those IRQs at unbind.

On that second open, devm_request_irq() on the non-shared TX/RX lines would
fail with -EBUSY. Each attempt also adds another shared common/LPI handler.

Would request_irq() in sxgbe_open() and free_irq() in sxgbe_release(),
before the rings are freed, be a more direct fix? It would also cover the
netdev and priv->hw window this patch targets, because unregister_netdev()
would free the IRQs before sxgbe_drv_remove() frees anything.

[Severity: High]
Related to the above, the device interrupt sources are never masked during
teardown. sxgbe_dma_channel_init() arms every channel:

    writel(SXGBE_DMA_ENA_INT,
           ioaddr + SXGBE_DMA_CHA_INT_ENABLE_REG(cha_num));

Neither sxgbe_release() nor sxgbe_drv_remove() clears these enables or the
MAC core interrupt sources. Only the RX ISR and NAPI toggle them, through
disable_dma_irq/enable_dma_irq.

So the stop_tx() calls in sxgbe_drv_remove() and sxgbe_release() trigger
the TPS interrupt themselves. sxgbe_restart_tx_queue() then sets
SXGBE_TX_ENABLE again, while the channel's descriptor base and tail
registers still point at the ring that free_dma_desc_resources() frees.

Could the device then fetch descriptors from, and write status back to,
freed coherent memory?

After sxgbe_release() calls clk_disable_unprepare(priv->sxgbe_clk), the
handlers are still registered and read status registers on the gated
block. sxgbe_common_interrupt() is on IRQF_SHARED lines, so another device
on the same line can invoke it.

This patch keeps netdev, hw and ioaddr alive, so these accesses become real
MMIO instead of freed-pointer dereferences. Is anything stopping them?

Should ndo_stop mask the per-channel and MAC interrupt enables, and free or
synchronize the IRQs, before it stops DMA, gates the clock and frees the
rings?

[Severity: Medium]
This is a pre-existing issue, but init_tx_ring() allocates tx_skbuff_dma
and tx_skbuff with devm_kcalloc() on every ndo_open:

    tx_ring->tx_skbuff_dma = devm_kcalloc(dev, tx_rsize,
                                          sizeof(dma_addr_t), GFP_KERNEL);
    ...
    tx_ring->tx_skbuff = devm_kcalloc(dev, tx_rsize,
                                      sizeof(struct sk_buff *), GFP_KERNEL);

free_tx_ring() only frees the coherent descriptors. Do these arrays pile up
on each ifup until unbind?

The devm IRQs from the first open are never freed, so each later ifup fails
with -EBUSY at the TX IRQ request. Each attempt still leaks a new set of
arrays.

Separately, the irq_of_parse_and_map() mappings from sxgbe_platform_probe()
are only disposed on the probe error path. sxgbe_platform_remove() only
calls sxgbe_drv_remove(ndev). Should it also call irq_dispose_mapping() for
the common, TX, RX and LPI mappings?

>  
>  #ifdef CONFIG_PM

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927144741.1320558-1-yaojiale02%40163.com

Re: [PATCH v2 0/7] net: ethernet: release managed IRQs before freeing netdevs

[Jakub Kicinski]

On Sun, 27 Sep 2026 22:47:33 +0800 Jiale Yao wrote:
> Several Ethernet platform drivers request interrupts with
> devm_request_irq() but allocate and free their netdevs manually.
> Device-managed resources are released only after the driver's remove
> callback returns, so these callbacks free the IRQ data while the interrupt
> handlers can still be invoked. A late or shared interrupt in this window
> can dereference freed memory.

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.