| Message ID | 20240209170459.4143861-5-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | net: ravb: Add runtime PM support (part 2) | expand |
Hi Claudiu Beznea, > -----Original Message----- > From: Claudiu <claudiu.beznea@tuxon.dev> > Sent: Friday, February 9, 2024 5:05 PM > Subject: [PATCH net-next v2 4/5] net: ravb: Do not apply RX checksum > settings to hardware if the interface is down > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Do not apply the RX checksum settings 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 > RX checksum settings and apply them in ravb_open() through > ravb_emac_init(). > This has been solved by introducing pm_runtime_active() check. 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 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 > > [2] > > drivers/net/ethernet/renesas/ravb_main.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > b/drivers/net/ethernet/renesas/ravb_main.c > index 7a7f743a1fef..f4be08f0198d 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -2478,8 +2478,14 @@ static int ravb_change_mtu(struct net_device *ndev, > int new_mtu) static void ravb_set_rx_csum(struct net_device *ndev, bool > enable) { > struct ravb_private *priv = netdev_priv(ndev); > + struct device *dev = &priv->pdev->dev; > unsigned long flags; > > + pm_runtime_get_noresume(dev); > + > + if (!pm_runtime_active(dev)) > + goto out_rpm_put; Thanks for the patch, Why can't this be handled in ravb_set_features() to avoid code duplication?? Cheers, Biju > + > spin_lock_irqsave(&priv->lock, flags); > > /* Disable TX and RX */ > @@ -2492,6 +2498,9 @@ static void ravb_set_rx_csum(struct net_device > *ndev, bool enable) > ravb_rcv_snd_enable(ndev); > > spin_unlock_irqrestore(&priv->lock, flags); > + > +out_rpm_put: > + pm_runtime_put_noidle(dev); > } > > static int ravb_endisable_csum_gbeth(struct net_device *ndev, enum > ravb_reg reg, @@ -2515,10 +2524,16 @@ static int > ravb_set_features_gbeth(struct net_device *ndev, { > netdev_features_t changed = ndev->features ^ features; > struct ravb_private *priv = netdev_priv(ndev); > + struct device *dev = &priv->pdev->dev; > unsigned long flags; > int ret = 0; > u32 val; > > + pm_runtime_get_noresume(dev); > + > + if (!pm_runtime_active(dev)) > + goto out_rpm_put; > + > spin_lock_irqsave(&priv->lock, flags); > if (changed & NETIF_F_RXCSUM) { > if (features & NETIF_F_RXCSUM) > @@ -2542,9 +2557,12 @@ static int ravb_set_features_gbeth(struct > net_device *ndev, > goto done; > } > > - ndev->features = features; > done: > spin_unlock_irqrestore(&priv->lock, flags); > +out_rpm_put: > + pm_runtime_put_noidle(dev); > + if (!ret) > + ndev->features = features; > > return ret; > } > -- > 2.39.2 >
On 2/9/24 8:04 PM, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Do not apply the RX checksum settings 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 RX > checksum settings and apply them in ravb_open() through ravb_emac_init(). > This has been solved by introducing pm_runtime_active() check. 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
Hi Sergey, > -----Original Message----- > From: Sergey Shtylyov <s.shtylyov@omp.ru> > Sent: Friday, February 9, 2024 8:27 PM > Subject: Re: [PATCH net-next v2 4/5] net: ravb: Do not apply RX checksum > settings to hardware if the interface is down > > On 2/9/24 8:04 PM, Claudiu wrote: > > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > Do not apply the RX checksum settings 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 RX checksum settings and apply them in ravb_open() > through ravb_emac_init(). > > This has been solved by introducing pm_runtime_active() check. 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> This will do the same job, without code duplication right? static int ravb_set_features(struct net_device *ndev, netdev_features_t features) { struct ravb_private *priv = netdev_priv(ndev); struct device *dev = &priv->pdev->dev; const struct ravb_hw_info *info = priv->info; pm_runtime_get_noresume(dev); if (!pm_runtime_active(dev)) { pm_runtime_put_noidle(dev); ndev->features = features; return 0; } return info->set_feature(ndev, features); } Cheers, Biju
On 2/9/24 11:41 PM, Biju Das wrote: [...] >>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> >>> Do not apply the RX checksum settings 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 RX checksum settings and apply them in ravb_open() >>> through ravb_emac_init(). >>> This has been solved by introducing pm_runtime_active() check. 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> > > This will do the same job, without code duplication right? > > static int ravb_set_features(struct net_device *ndev, > netdev_features_t features) > { > struct ravb_private *priv = netdev_priv(ndev); > struct device *dev = &priv->pdev->dev; > const struct ravb_hw_info *info = priv->info; > > pm_runtime_get_noresume(dev); > if (!pm_runtime_active(dev)) { > pm_runtime_put_noidle(dev); > ndev->features = features; > return 0; > } > > return info->set_feature(ndev, features); We now leak the device reference by not calling pm_runtime_put_noidle() after this statement... The approach seems sane though -- Claudiu, please consider following it. [...] > Cheers, > Biju MBR, Sergey
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 7a7f743a1fef..f4be08f0198d 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -2478,8 +2478,14 @@ static int ravb_change_mtu(struct net_device *ndev, int new_mtu) static void ravb_set_rx_csum(struct net_device *ndev, bool enable) { struct ravb_private *priv = netdev_priv(ndev); + struct device *dev = &priv->pdev->dev; unsigned long flags; + pm_runtime_get_noresume(dev); + + if (!pm_runtime_active(dev)) + goto out_rpm_put; + spin_lock_irqsave(&priv->lock, flags); /* Disable TX and RX */ @@ -2492,6 +2498,9 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable) ravb_rcv_snd_enable(ndev); spin_unlock_irqrestore(&priv->lock, flags); + +out_rpm_put: + pm_runtime_put_noidle(dev); } static int ravb_endisable_csum_gbeth(struct net_device *ndev, enum ravb_reg reg, @@ -2515,10 +2524,16 @@ static int ravb_set_features_gbeth(struct net_device *ndev, { netdev_features_t changed = ndev->features ^ features; struct ravb_private *priv = netdev_priv(ndev); + struct device *dev = &priv->pdev->dev; unsigned long flags; int ret = 0; u32 val; + pm_runtime_get_noresume(dev); + + if (!pm_runtime_active(dev)) + goto out_rpm_put; + spin_lock_irqsave(&priv->lock, flags); if (changed & NETIF_F_RXCSUM) { if (features & NETIF_F_RXCSUM) @@ -2542,9 +2557,12 @@ static int ravb_set_features_gbeth(struct net_device *ndev, goto done; } - ndev->features = features; done: spin_unlock_irqrestore(&priv->lock, flags); +out_rpm_put: + pm_runtime_put_noidle(dev); + if (!ret) + ndev->features = features; return ret; }