From: Claudiu Beznea <[email protected]>
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 <[email protected]>
Signed-off-by: Claudiu Beznea <[email protected]>
---
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))
+ return &ndev->stats;
+
nstats = &ndev->stats;
stats0 = &priv->stats[RAVB_BE];
--
2.39.2
On 12/14/23 2:45 PM, Claudiu wrote:
> From: Claudiu Beznea <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Claudiu Beznea <[email protected]>
> ---
>
> 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 <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Claudiu Beznea <[email protected]>
> ---
>
> 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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Claudiu Beznea <[email protected]>
>> ---
>>
>> 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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Claudiu Beznea <[email protected]>
>>> ---
>>>
>>> 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