Re: [PATCH net 1/2] net: ravb: Fix missing rtnl lock in suspend path

[Sergey Shtylyov <s.shtylyov@omp.ru>]

On 1/22/25 9:33 PM, Sergey Shtylyov wrote:
[...]

>> Fix the suspend path by ensuring the rtnl lock is held where required.
> 
>    Maybe suspend/resume path (the same w/the subject)?
> 
>> Calls to ravb_open, ravb_close and wol operations must be performed under
>> the rtnl lock to prevent conflicts with ongoing ndo operations.
> 
> [...]
> 
>> Reported-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> Closes: https://lore.kernel.org/netdev/4c6419d8-c06b-495c-b987-d66c2e1ff848@tuxon.dev/
>> Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support")
>> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> 
>     FWIW:
> 
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> 
> [...]
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index bc395294a32d..2c6d8e4966c3 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
>> @@ -3245,19 +3250,25 @@ static int ravb_resume(struct device *dev)
>>  	if (!netif_running(ndev))
>>  		return 0;
>>  
>> +	rtnl_lock();
>>  	/* If WoL is enabled restore the interface. */
>>  	if (priv->wol_enabled) {
>>  		ret = ravb_wol_restore(ndev);
>> -		if (ret)
>> +		if (ret)  {
>> +			rtnl_unlock();
>>  			return ret;
>> +		}
>>  	} else {
>>  		ret = pm_runtime_force_resume(dev);
>> -		if (ret)
>> +		if (ret) {
>> +			rtnl_unlock();
>>  			return ret;
> 
>    Hm, are you sure we need to have rtnl_lock around pm_runtime_force_resume() too?
 
   Anyway, the above *if* statements are needlessly duplicated, I think it's time
that we do away with this! :-)

[...]

MBR, Sergey