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

[Biju Das]

Hi Claudiu,

> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@xxxxxxxxx>
> Sent: Tuesday, February 13, 2024 11:07 AM
> Subject: Re: [PATCH net-next v3 5/6] net: ravb: Do not apply features to
> hardware if the interface is down
> 
> 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.

I find this patch complicated by doing unnecessary intiailzation
And goto statements for non-err cases.. 

Maybe others can suggest how to do it in a better way. 

Cheers,
Biju
> 
> 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
> >