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@xxxxxxxxxxxx> 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@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