diff mbox series

[net-next,v3,17/19] net: ravb: Return cached statistics if the interface is down

Message ID 20240105082339.1468817-18-claudiu.beznea.uj@bp.renesas.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S | expand

Checks

Context Check Description
netdev/series_format fail Series longer than 15 patches (and no cover letter)
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1113 this patch: 1113
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang success Errors and warnings before: 1140 this patch: 1140
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1140 this patch: 1140
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 18 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Claudiu Jan. 5, 2024, 8:23 a.m. UTC 
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 cached statistics are updated in ravb_close().

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 v3:
- this was patch 18/21 in v2
- use ndev->flags & IFF_UP instead of netif_running checks

Changes in v2:
- none; this patch is new

 drivers/net/ethernet/renesas/ravb_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Sergey Shtylyov Jan. 8, 2024, 8:22 p.m. UTC | #1 
On 1/5/24 11:23 AM, 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 cached statistics are updated in ravb_close().
> 
> 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>
[...]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 76035afd4054..168b6208db37 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2117,6 +2117,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 (!(ndev->flags & IFF_UP))

   Well, I guess it's OK to read the counters in the reset mode... BUT
won't this race with pm_runtime_put_autosuspend() when its call gets added
to ravb_close()?

> +		return &ndev->stats;
> +
>  	nstats = &ndev->stats;
>  	stats0 = &priv->stats[RAVB_BE];
>  
[...]

MBR, Sergey
Claudiu Jan. 10, 2024, 1:17 p.m. UTC | #2 
On 08.01.2024 22:22, Sergey Shtylyov wrote:
> On 1/5/24 11:23 AM, 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 cached statistics are updated in ravb_close().
>>
>> 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>
> [...]
> 
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 76035afd4054..168b6208db37 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2117,6 +2117,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 (!(ndev->flags & IFF_UP))
> 
>    Well, I guess it's OK to read the counters in the reset mode... BUT
> won't this race with pm_runtime_put_autosuspend() when its call gets added
> to ravb_close()?

I re-checked it and, yes, this is true. A sync runtime suspend would be
better here. But, as of my current investigation, even with this
ravb_get_stats() can still race with ravb_open()/ravb_close() as they are
called though different locking scheme (ravb_open()/ravb_close() is called
with rtnl locked while ravb_get_stats() can be called only with
dev_base_lock rwlock locked for reading).

A mutex in the driver should to help with this.

Thank you,
Claudiu Beznea

> 
>> +		return &ndev->stats;
>> +
>>  	nstats = &ndev->stats;
>>  	stats0 = &priv->stats[RAVB_BE];
>>  
> [...]
> 
> MBR, Sergey
>
Sergey Shtylyov Jan. 14, 2024, 12:22 p.m. UTC | #3 
On 1/10/24 4:17 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 cached statistics are updated in ravb_close().
>>>
>>> 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>
>> [...]
>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 76035afd4054..168b6208db37 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -2117,6 +2117,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 (!(ndev->flags & IFF_UP))
>>
>>    Well, I guess it's OK to read the counters in the reset mode... BUT
>> won't this race with pm_runtime_put_autosuspend() when its call gets added
>> to ravb_close()?
> 
> I re-checked it and, yes, this is true. A sync runtime suspend would be
> better here. But, as of my current investigation, even with this

   No, the sync form of the RPM call won't fix the race...

> ravb_get_stats() can still race with ravb_open()/ravb_close() as they are
> called though different locking scheme (ravb_open()/ravb_close() is called
> with rtnl locked while ravb_get_stats() can be called only with
> dev_base_lock rwlock locked for reading).
> 
> A mutex in the driver should to help with this.

   Why don't you want to mimic what the sh_eth driver does?

> Thank you,
> Claudiu Beznea

[...]

MBR, Sergey
Claudiu Jan. 15, 2024, 6:08 a.m. UTC | #4 
On 14.01.2024 14:22, Sergey Shtylyov wrote:
> On 1/10/24 4:17 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 cached statistics are updated in ravb_close().
>>>>
>>>> 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>
>>> [...]
>>>
>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>>> index 76035afd4054..168b6208db37 100644
>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>> @@ -2117,6 +2117,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 (!(ndev->flags & IFF_UP))
>>>
>>>    Well, I guess it's OK to read the counters in the reset mode... BUT
>>> won't this race with pm_runtime_put_autosuspend() when its call gets added
>>> to ravb_close()?
>>
>> I re-checked it and, yes, this is true. A sync runtime suspend would be
>> better here. But, as of my current investigation, even with this
> 
>    No, the sync form of the RPM call won't fix the race...
> 
>> ravb_get_stats() can still race with ravb_open()/ravb_close() as they are
>> called though different locking scheme (ravb_open()/ravb_close() is called
>> with rtnl locked while ravb_get_stats() can be called only with
>> dev_base_lock rwlock locked for reading).
>>
>> A mutex in the driver should to help with this.
> 
>    Why don't you want to mimic what the sh_eth driver does?

I thought it can be replaced by the already existing IFF_UP flag from
ndev->flags.

Investigating it further while trying to address this concurrency issue
made me realize that it fits to address the issue you mentioned here.

Thank you,
Claudiu Beznea

> 
>> Thank you,
>> Claudiu Beznea
> 
> [...]
> 
> MBR, Sergey
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 76035afd4054..168b6208db37 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2117,6 +2117,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 (!(ndev->flags & IFF_UP))
+		return &ndev->stats;
+
 	nstats = &ndev->stats;
 	stats0 = &priv->stats[RAVB_BE];
 
@@ -2226,6 +2229,9 @@  static int ravb_close(struct net_device *ndev)
 	if (info->nc_queues)
 		ravb_ring_free(ndev, RAVB_NC);
 
+	/* Update statistics. */
+	ravb_get_stats(ndev);
+
 	/* Set reset mode. */
 	return ravb_set_opmode(ndev, CCC_OPC_RESET);
 }