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
> >
