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@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
[...]> @@ -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