| Message ID | 20240213094110.853155-6-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ravb: Add runtime PM support (part 2) | expand |
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
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 >
Hi Claudiu, > -----Original Message----- > From: claudiu beznea <claudiu.beznea@tuxon.dev> > 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@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. 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 > >
On 13.02.2024 13:07, claudiu beznea wrote: >>> @@ -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. Looking again at it, your version seems better.
On 2/13/24 12:41 PM, Claudiu wrote: > 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> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> [...] MBR, Sergey
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; 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 = {