Re: [PATCH net-next v3 5/6] net: ravb: Do not apply features to hardware if the interface is down

[claudiu beznea <claudiu.beznea@tuxon.dev>]

Hi, Biju,

On 13.02.2024 12:13, Biju Das wrote:
> 
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: Claudiu <claudiu.beznea@tuxon.dev>
>> Sent: Tuesday, February 13, 2024 9:41 AM
>> Subject: [PATCH net-next v3 5/6] net: ravb: Do not apply features to
>> hardware if the interface is down
>>
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Do not apply features to hardware if the interface is down. In case
>> runtime PM is enabled, and while the interface is down, the IP will be in
>> reset mode (as for some platforms disabling the clocks will switch the IP
>> to reset mode, which will lead to losing register contents) and applying
>> settings in reset mode is not an option. Instead, cache the features and
>> apply them in ravb_open() through ravb_emac_init().
>>
>> To avoid accessing the hardware while the interface is down
>> pm_runtime_active() check was introduced. Along with it the device runtime
>> PM usage counter has been incremented to avoid disabling the device clocks
>> while the check is in progress (if any).
>>
>> Commit prepares for the addition of runtime PM.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v3:
>> - updated patch title and description
>> - updated patch content due to patch 4/6
>>
>> Changes in v2:
>> - fixed typo in patch description
>> - adjusted ravb_set_features_gbeth(); didn't collect the Sergey's Rb
>>   tag due to this
>>
>> Changes since [2]:
>> - use pm_runtime_get_noresume() and pm_runtime_active() and updated the
>>   commit message to describe that
>> - fixed typos
>> - s/CSUM/checksum in patch title and description
>>
>> Changes in v3 of [2]:
>> - this was patch 20/21 in v2
>> - fixed typos in patch description
>> - removed code from ravb_open()
>> - use ndev->flags & IFF_UP checks instead of netif_running()
>>
>> Changes in v2 of [2]:
>> - none; this patch is new
>>
>>
>>  drivers/net/ethernet/renesas/ravb_main.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>> b/drivers/net/ethernet/renesas/ravb_main.c
>> index b3b91783bb7a..4dd0520dea90 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2566,15 +2566,23 @@ static int ravb_set_features(struct net_device
>> *ndev,  {
>>  	struct ravb_private *priv = netdev_priv(ndev);
>>  	const struct ravb_hw_info *info = priv->info;
>> -	int ret;
>> +	struct device *dev = &priv->pdev->dev;
>> +	int ret = 0;
>> +
>> +	pm_runtime_get_noresume(dev);
>> +
>> +	if (!pm_runtime_active(dev))
>> +		goto out_set_features;
> 
> This can be simplified, which avoids 1 goto statement and
> Unnecessary ret initialization. I am leaving to you and Sergey.
> 
> 	if (!pm_runtime_active(dev))
> 		ret = 0;
> 	else
> 		ret = info->set_feature(ndev, features);
> 
> 	pm_runtime_put_noidle(dev);
> 	if (ret)
> 		goto err;
> 
> 	ndev->features = features;
> 
> err:
> 	return ret;
> 

I find it a bit difficult to follow this way.

Thank you,
Claudiu Beznea

> Cheers,
> Biju
> 
>>
>>  	ret = info->set_feature(ndev, features);
>>  	if (ret)
>> -		return ret;
>> +		goto out_rpm_put;
>>
>> +out_set_features:
>>  	ndev->features = features;
>> -
>> -	return 0;
>> +out_rpm_put:
>> +	pm_runtime_put_noidle(dev);
>> +	return ret;
>>  }
>>
>>  static const struct net_device_ops ravb_netdev_ops = {
>> --
>> 2.39.2
>