| Message ID | 20231214114600.2451162-19-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S | expand |
On 12/14/23 2:45 PM, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Return the cached statistics in case the interface is down. There should be > no drawback to this, as most of the statistics are updated on the data path > and if runtime PM is enabled and the interface is down, the registers that > are explicitly read on ravb_get_stats() are zero anyway on most of the IP > variants. > > The commit prepares the code for the addition of runtime PM support. > > Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > > Changes in v2: > - none; this patch is new > > drivers/net/ethernet/renesas/ravb_main.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index a2a64c22ec41..1995cf7ff084 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -2110,6 +2110,9 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev) > const struct ravb_hw_info *info = priv->info; > struct net_device_stats *nstats, *stats0, *stats1; > > + if (!netif_running(ndev)) I'm afraid this is racy as __LINK_STATE_START bit gets set by __dev_open() before calling the ndo_open() method... :-( > + return &ndev->stats; > + The sh_eth driver calls sh_eth_get_stats() from sh_eth_dev_exit(); perhaps it's worth doing something similar? MBR, Sergey
On 12/14/23 2:45 PM, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Return the cached statistics in case the interface is down. There should be > no drawback to this, as most of the statistics are updated on the data path > and if runtime PM is enabled and the interface is down, the registers that > are explicitly read on ravb_get_stats() are zero anyway on most of the IP > variants. > > The commit prepares the code for the addition of runtime PM support. > > Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > > Changes in v2: > - none; this patch is new > > drivers/net/ethernet/renesas/ravb_main.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index a2a64c22ec41..1995cf7ff084 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -2110,6 +2110,9 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev) > const struct ravb_hw_info *info = priv->info; > struct net_device_stats *nstats, *stats0, *stats1; > > + if (!netif_running(ndev)) I'm afraid this is racy as __LINK_STATE_START bit gets set by __dev_open() before calling the ndo_open() method... :-( > + return &ndev->stats; > + The sh_eth driver calls sh_eth_get_stats() from sh_eth_dev_exit(); perhaps it's worth doing something similar? MBR, Sergey
On 16.12.2023 22:02, Sergey Shtylyov wrote: > On 12/14/23 2:45 PM, Claudiu wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> Return the cached statistics in case the interface is down. There should be >> no drawback to this, as most of the statistics are updated on the data path >> and if runtime PM is enabled and the interface is down, the registers that >> are explicitly read on ravb_get_stats() are zero anyway on most of the IP >> variants. >> >> The commit prepares the code for the addition of runtime PM support. >> >> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> --- >> >> Changes in v2: >> - none; this patch is new >> >> drivers/net/ethernet/renesas/ravb_main.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >> index a2a64c22ec41..1995cf7ff084 100644 >> --- a/drivers/net/ethernet/renesas/ravb_main.c >> +++ b/drivers/net/ethernet/renesas/ravb_main.c >> @@ -2110,6 +2110,9 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev) >> const struct ravb_hw_info *info = priv->info; >> struct net_device_stats *nstats, *stats0, *stats1; >> >> + if (!netif_running(ndev)) > > I'm afraid this is racy as __LINK_STATE_START bit gets set > by __dev_open() before calling the ndo_open() method... :-( But (at least on my setup), both ndo_get_stats and ndo_open are called with rtnl_mutex locked. > >> + return &ndev->stats; >> + > > The sh_eth driver calls sh_eth_get_stats() from sh_eth_dev_exit(); > perhaps it's worth doing something similar? Indeed, it might help to keep updated those few registers that gets updated only in ndo_get_stats. > > MBR, Sergey
On 12/17/23 4:54 PM, claudiu beznea wrote: [...] >>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> >>> Return the cached statistics in case the interface is down. There should be >>> no drawback to this, as most of the statistics are updated on the data path >>> and if runtime PM is enabled and the interface is down, the registers that >>> are explicitly read on ravb_get_stats() are zero anyway on most of the IP >>> variants. >>> >>> The commit prepares the code for the addition of runtime PM support. >>> >>> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru> >>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> --- >>> >>> Changes in v2: >>> - none; this patch is new >>> >>> drivers/net/ethernet/renesas/ravb_main.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >>> index a2a64c22ec41..1995cf7ff084 100644 >>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c >>> @@ -2110,6 +2110,9 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev) >>> const struct ravb_hw_info *info = priv->info; >>> struct net_device_stats *nstats, *stats0, *stats1; >>> >>> + if (!netif_running(ndev)) >> >> I'm afraid this is racy as __LINK_STATE_START bit gets set >> by __dev_open() before calling the ndo_open() method... :-( > > But (at least on my setup), both ndo_get_stats and ndo_open are called with > rtnl_mutex locked. Unfortunately, it's not always so -- see e.g. netstat_show() in net/core/net-sysfs.c... [...] MBR, Sergey
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index a2a64c22ec41..1995cf7ff084 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -2110,6 +2110,9 @@ static struct net_device_stats *ravb_get_stats(struct net_device *ndev) const struct ravb_hw_info *info = priv->info; struct net_device_stats *nstats, *stats0, *stats1; + if (!netif_running(ndev)) + return &ndev->stats; + nstats = &ndev->stats; stats0 = &priv->stats[RAVB_BE];