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

[Biju Das <biju.das.jz@bp.renesas.com>]

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;

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