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@xxxxxxxxxxxxxxx
> Signed-off-by: Jiale Yao <yaojiale02@xxxxxxx>
> ---
>  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