Hi, Biju,
On 13.02.2024 12:13, Biju Das wrote:
>
> Hi Claudiu,
>
>> -----Original Message-----
>> From: Claudiu <claudiu.beznea@xxxxxxxxx>
>> 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@xxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxx>
>> ---
>>
>> 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
>
