diff mbox series

[v4] net: mvneta: fix calls to page_pool_get_stats

Message ID 4wba22pa6sxknqfxve42xevswz4wfu637p5gyyeq546tmzudzu@4z3kphfrpm64 (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [v4] net: mvneta: fix calls to page_pool_get_stats | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
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: 1352 this patch: 1352
netdev/cc_maintainers fail 1 blamed authors not CCed: andrew@lunn.ch; 3 maintainers not CCed: edumazet@google.com pabeni@redhat.com andrew@lunn.ch
netdev/build_clang success Errors and warnings before: 1378 this patch: 1378
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 Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1380 this patch: 1380
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 55 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

Sven Auhagen Nov. 9, 2023, 5:59 a.m. UTC 
Calling page_pool_get_stats in the mvneta driver without checks
leads to kernel crashes.
First the page pool is only available if the bm is not used.
The page pool is also not allocated when the port is stopped.
It can also be not allocated in case of errors.

The current implementation leads to the following crash calling
ethstats on a port that is down or when calling it at the wrong moment:

ble to handle kernel NULL pointer dereference at virtual address 00000070
[00000070] *pgd=00000000
Internal error: Oops: 5 [#1] SMP ARM
Hardware name: Marvell Armada 380/385 (Device Tree)
PC is at page_pool_get_stats+0x18/0x1cc
LR is at mvneta_ethtool_get_stats+0xa0/0xe0 [mvneta]
pc : [<c0b413cc>]    lr : [<bf0a98d8>]    psr: a0000013
sp : f1439d48  ip : f1439dc0  fp : 0000001d
r10: 00000100  r9 : c4816b80  r8 : f0d75150
r7 : bf0b400c  r6 : c238f000  r5 : 00000000  r4 : f1439d68
r3 : c2091040  r2 : ffffffd8  r1 : f1439d68  r0 : 00000000
Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 066b004a  DAC: 00000051
Register r0 information: NULL pointer
Register r1 information: 2-page vmalloc region starting at 0xf1438000 allocated at kernel_clone+0x9c/0x390
Register r2 information: non-paged memory
Register r3 information: slab kmalloc-2k start c2091000 pointer offset 64 size 2048
Register r4 information: 2-page vmalloc region starting at 0xf1438000 allocated at kernel_clone+0x9c/0x390
Register r5 information: NULL pointer
Register r6 information: slab kmalloc-cg-4k start c238f000 pointer offset 0 size 4096
Register r7 information: 15-page vmalloc region starting at 0xbf0a8000 allocated at load_module+0xa30/0x219c
Register r8 information: 1-page vmalloc region starting at 0xf0d75000 allocated at ethtool_get_stats+0x138/0x208
Register r9 information: slab task_struct start c4816b80 pointer offset 0
Register r10 information: non-paged memory
Register r11 information: non-paged memory
Register r12 information: 2-page vmalloc region starting at 0xf1438000 allocated at kernel_clone+0x9c/0x390
Process snmpd (pid: 733, stack limit = 0x38de3a88)
Stack: (0xf1439d48 to 0xf143a000)
9d40:                   000000c0 00000001 c238f000 bf0b400c f0d75150 c4816b80
9d60: 00000100 bf0a98d8 00000000 00000000 00000000 00000000 00000000 00000000
9d80: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
9da0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
9dc0: 00000dc0 5335509c 00000035 c238f000 bf0b2214 01067f50 f0d75000 c0b9b9c8
9de0: 0000001d 00000035 c2212094 5335509c c4816b80 c238f000 c5ad6e00 01067f50
9e00: c1b0be80 c4816b80 00014813 c0b9d7f0 00000000 00000000 0000001d 0000001d
9e20: 00000000 00001200 00000000 00000000 c216ed90 c73943b8 00000000 00000000
9e40: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
9e60: 00000000 c0ad9034 00000000 00000000 00000000 00000000 00000000 00000000
9e80: 00000000 00000000 00000000 5335509c c1b0be80 f1439ee4 00008946 c1b0be80
9ea0: 01067f50 f1439ee3 00000000 00000046 b6d77ae0 c0b383f0 00008946 becc83e8
9ec0: c1b0be80 00000051 0000000b c68ca480 c7172d00 c0ad8ff0 f1439ee3 cf600e40
9ee0: 01600e40 32687465 00000000 00000000 00000000 01067f50 00000000 00000000
9f00: 00000000 5335509c 00008946 00008946 00000000 c68ca480 becc83e8 c05e2de0
9f20: f1439fb0 c03002f0 00000006 5ac3c35a c4816b80 00000006 b6d77ae0 c030caf0
9f40: c4817350 00000014 f1439e1c 0000000c 00000000 00000051 01000000 00000014
9f60: 00003fec f1439edc 00000001 c0372abc b6d77ae0 c0372abc cf600e40 5335509c
9f80: c21e6800 01015c9c 0000000b 00008946 00000036 c03002f0 c4816b80 00000036
9fa0: b6d77ae0 c03000c0 01015c9c 0000000b 0000000b 00008946 becc83e8 00000000
9fc0: 01015c9c 0000000b 00008946 00000036 00000035 010678a0 b6d797ec b6d77ae0
9fe0: b6dbf738 becc838c b6d186d7 b6baa858 40000030 0000000b 00000000 00000000
 page_pool_get_stats from mvneta_ethtool_get_stats+0xa0/0xe0 [mvneta]
 mvneta_ethtool_get_stats [mvneta] from ethtool_get_stats+0x154/0x208
 ethtool_get_stats from dev_ethtool+0xf48/0x2480
 dev_ethtool from dev_ioctl+0x538/0x63c
 dev_ioctl from sock_ioctl+0x49c/0x53c
 sock_ioctl from sys_ioctl+0x134/0xbd8
 sys_ioctl from ret_fast_syscall+0x0/0x1c
Exception stack(0xf1439fa8 to 0xf1439ff0)
9fa0:                   01015c9c 0000000b 0000000b 00008946 becc83e8 00000000
9fc0: 01015c9c 0000000b 00008946 00000036 00000035 010678a0 b6d797ec b6d77ae0
9fe0: b6dbf738 becc838c b6d186d7 b6baa858
Code: e28dd004 e1a05000 e2514000 0a00006a (e5902070)

This commit adds the proper checks before calling page_pool_get_stats.

Fixes: b3fc79225f05 ("net: mvneta: add support for page_pool_get_stats")
Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>
Reported-by: Paulo Da Silva <Paulo.DaSilva@kyberna.com>
---

Change from v3:
	* Move the page pool check back to mvneta

Change from v2:
	* Fix the fixes tag

Change from v1:
	* Add cover letter
	* Move the page pool check in mvneta to the ethtool stats
	  function

Comments

Lorenzo Bianconi Nov. 9, 2023, 7:48 a.m. UTC | #1 
> Calling page_pool_get_stats in the mvneta driver without checks
> leads to kernel crashes.
> First the page pool is only available if the bm is not used.
> The page pool is also not allocated when the port is stopped.
> It can also be not allocated in case of errors.
> 
> The current implementation leads to the following crash calling
> ethstats on a port that is down or when calling it at the wrong moment:
> 
> ble to handle kernel NULL pointer dereference at virtual address 00000070
> [00000070] *pgd=00000000
> Internal error: Oops: 5 [#1] SMP ARM
> Hardware name: Marvell Armada 380/385 (Device Tree)
> PC is at page_pool_get_stats+0x18/0x1cc
> LR is at mvneta_ethtool_get_stats+0xa0/0xe0 [mvneta]
> pc : [<c0b413cc>]    lr : [<bf0a98d8>]    psr: a0000013
> sp : f1439d48  ip : f1439dc0  fp : 0000001d
> r10: 00000100  r9 : c4816b80  r8 : f0d75150
> r7 : bf0b400c  r6 : c238f000  r5 : 00000000  r4 : f1439d68
> r3 : c2091040  r2 : ffffffd8  r1 : f1439d68  r0 : 00000000
> Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> Control: 10c5387d  Table: 066b004a  DAC: 00000051
> Register r0 information: NULL pointer
> Register r1 information: 2-page vmalloc region starting at 0xf1438000 allocated at kernel_clone+0x9c/0x390
> Register r2 information: non-paged memory
> Register r3 information: slab kmalloc-2k start c2091000 pointer offset 64 size 2048
> Register r4 information: 2-page vmalloc region starting at 0xf1438000 allocated at kernel_clone+0x9c/0x390
> Register r5 information: NULL pointer
> Register r6 information: slab kmalloc-cg-4k start c238f000 pointer offset 0 size 4096
> Register r7 information: 15-page vmalloc region starting at 0xbf0a8000 allocated at load_module+0xa30/0x219c
> Register r8 information: 1-page vmalloc region starting at 0xf0d75000 allocated at ethtool_get_stats+0x138/0x208
> Register r9 information: slab task_struct start c4816b80 pointer offset 0
> Register r10 information: non-paged memory
> Register r11 information: non-paged memory
> Register r12 information: 2-page vmalloc region starting at 0xf1438000 allocated at kernel_clone+0x9c/0x390
> Process snmpd (pid: 733, stack limit = 0x38de3a88)
> Stack: (0xf1439d48 to 0xf143a000)
> 9d40:                   000000c0 00000001 c238f000 bf0b400c f0d75150 c4816b80
> 9d60: 00000100 bf0a98d8 00000000 00000000 00000000 00000000 00000000 00000000
> 9d80: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 9da0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 9dc0: 00000dc0 5335509c 00000035 c238f000 bf0b2214 01067f50 f0d75000 c0b9b9c8
> 9de0: 0000001d 00000035 c2212094 5335509c c4816b80 c238f000 c5ad6e00 01067f50
> 9e00: c1b0be80 c4816b80 00014813 c0b9d7f0 00000000 00000000 0000001d 0000001d
> 9e20: 00000000 00001200 00000000 00000000 c216ed90 c73943b8 00000000 00000000
> 9e40: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 9e60: 00000000 c0ad9034 00000000 00000000 00000000 00000000 00000000 00000000
> 9e80: 00000000 00000000 00000000 5335509c c1b0be80 f1439ee4 00008946 c1b0be80
> 9ea0: 01067f50 f1439ee3 00000000 00000046 b6d77ae0 c0b383f0 00008946 becc83e8
> 9ec0: c1b0be80 00000051 0000000b c68ca480 c7172d00 c0ad8ff0 f1439ee3 cf600e40
> 9ee0: 01600e40 32687465 00000000 00000000 00000000 01067f50 00000000 00000000
> 9f00: 00000000 5335509c 00008946 00008946 00000000 c68ca480 becc83e8 c05e2de0
> 9f20: f1439fb0 c03002f0 00000006 5ac3c35a c4816b80 00000006 b6d77ae0 c030caf0
> 9f40: c4817350 00000014 f1439e1c 0000000c 00000000 00000051 01000000 00000014
> 9f60: 00003fec f1439edc 00000001 c0372abc b6d77ae0 c0372abc cf600e40 5335509c
> 9f80: c21e6800 01015c9c 0000000b 00008946 00000036 c03002f0 c4816b80 00000036
> 9fa0: b6d77ae0 c03000c0 01015c9c 0000000b 0000000b 00008946 becc83e8 00000000
> 9fc0: 01015c9c 0000000b 00008946 00000036 00000035 010678a0 b6d797ec b6d77ae0
> 9fe0: b6dbf738 becc838c b6d186d7 b6baa858 40000030 0000000b 00000000 00000000
>  page_pool_get_stats from mvneta_ethtool_get_stats+0xa0/0xe0 [mvneta]
>  mvneta_ethtool_get_stats [mvneta] from ethtool_get_stats+0x154/0x208
>  ethtool_get_stats from dev_ethtool+0xf48/0x2480
>  dev_ethtool from dev_ioctl+0x538/0x63c
>  dev_ioctl from sock_ioctl+0x49c/0x53c
>  sock_ioctl from sys_ioctl+0x134/0xbd8
>  sys_ioctl from ret_fast_syscall+0x0/0x1c
> Exception stack(0xf1439fa8 to 0xf1439ff0)
> 9fa0:                   01015c9c 0000000b 0000000b 00008946 becc83e8 00000000
> 9fc0: 01015c9c 0000000b 00008946 00000036 00000035 010678a0 b6d797ec b6d77ae0
> 9fe0: b6dbf738 becc838c b6d186d7 b6baa858
> Code: e28dd004 e1a05000 e2514000 0a00006a (e5902070)
> 
> This commit adds the proper checks before calling page_pool_get_stats.
> 
> Fixes: b3fc79225f05 ("net: mvneta: add support for page_pool_get_stats")
> Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>
> Reported-by: Paulo Da Silva <Paulo.DaSilva@kyberna.com>
> ---

Hi Sven,

first of all thx for fixing it. Just minor comments inline.

Regards,
Lorenzo

> 
> Change from v3:
> 	* Move the page pool check back to mvneta
> 
> Change from v2:
> 	* Fix the fixes tag
> 
> Change from v1:
> 	* Add cover letter
> 	* Move the page pool check in mvneta to the ethtool stats
> 	  function
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 8b0f12a0e0f2..bbb5d972657a 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -4734,13 +4734,16 @@ static void mvneta_ethtool_get_strings(struct net_device *netdev, u32 sset,
>  {
>  	if (sset == ETH_SS_STATS) {
>  		int i;
> +		struct mvneta_port *pp = netdev_priv(netdev);

nit: reverse christmas tree here (just if you need to repost)

>  
>  		for (i = 0; i < ARRAY_SIZE(mvneta_statistics); i++)
>  			memcpy(data + i * ETH_GSTRING_LEN,
>  			       mvneta_statistics[i].name, ETH_GSTRING_LEN);
>  
> -		data += ETH_GSTRING_LEN * ARRAY_SIZE(mvneta_statistics);
> -		page_pool_ethtool_stats_get_strings(data);
> +		if (!pp->bm_priv) {
> +			data += ETH_GSTRING_LEN * ARRAY_SIZE(mvneta_statistics);
> +			page_pool_ethtool_stats_get_strings(data);
> +		}
>  	}
>  }
>  
> @@ -4858,8 +4861,10 @@ static void mvneta_ethtool_pp_stats(struct mvneta_port *pp, u64 *data)
>  	struct page_pool_stats stats = {};
>  	int i;
>  
> -	for (i = 0; i < rxq_number; i++)
> -		page_pool_get_stats(pp->rxqs[i].page_pool, &stats);
> +	for (i = 0; i < rxq_number; i++) {
> +		if (pp->rxqs[i].page_pool)
> +			page_pool_get_stats(pp->rxqs[i].page_pool, &stats);
> +	}
>  
>  	page_pool_ethtool_stats_get(data, &stats);
>  }
> @@ -4875,14 +4880,21 @@ static void mvneta_ethtool_get_stats(struct net_device *dev,
>  	for (i = 0; i < ARRAY_SIZE(mvneta_statistics); i++)
>  		*data++ = pp->ethtool_stats[i];
>  
> -	mvneta_ethtool_pp_stats(pp, data);
> +	if (!pp->bm_priv && !pp->is_stopped)

do we need to check pp->is_stopped here? (we already check if page_pool
pointer is NULL in mvneta_ethtool_pp_stats).
Moreover in mvneta_ethtool_get_sset_count() and in mvneta_ethtool_get_strings()
we just check pp->bm_priv pointer. Are the stats disaligned in this case?

> +		mvneta_ethtool_pp_stats(pp, data);
>  }
>  
>  static int mvneta_ethtool_get_sset_count(struct net_device *dev, int sset)
>  {
> -	if (sset == ETH_SS_STATS)
> -		return ARRAY_SIZE(mvneta_statistics) +
> -		       page_pool_ethtool_stats_get_count();
> +	if (sset == ETH_SS_STATS) {
> +		int count = ARRAY_SIZE(mvneta_statistics);
> +		struct mvneta_port *pp = netdev_priv(dev);
> +
> +		if (!pp->bm_priv)
> +			count += page_pool_ethtool_stats_get_count();
> +
> +		return count;
> +	}
>  
>  	return -EOPNOTSUPP;
>  }
> -- 
> 2.42.0
>
Sven Auhagen Nov. 9, 2023, 11:06 a.m. UTC | #2 
On Thu, Nov 09, 2023 at 08:48:00AM +0100, Lorenzo Bianconi wrote:
> > Calling page_pool_get_stats in the mvneta driver without checks
> > leads to kernel crashes.
> > First the page pool is only available if the bm is not used.
> > The page pool is also not allocated when the port is stopped.
> > It can also be not allocated in case of errors.
> > 
> > The current implementation leads to the following crash calling
> > ethstats on a port that is down or when calling it at the wrong moment:
> > 
> > ble to handle kernel NULL pointer dereference at virtual address 00000070
> > [00000070] *pgd=00000000
> > Internal error: Oops: 5 [#1] SMP ARM
> > Hardware name: Marvell Armada 380/385 (Device Tree)
> > PC is at page_pool_get_stats+0x18/0x1cc
> > LR is at mvneta_ethtool_get_stats+0xa0/0xe0 [mvneta]
> > pc : [<c0b413cc>]    lr : [<bf0a98d8>]    psr: a0000013
> > sp : f1439d48  ip : f1439dc0  fp : 0000001d
> > r10: 00000100  r9 : c4816b80  r8 : f0d75150
> > r7 : bf0b400c  r6 : c238f000  r5 : 00000000  r4 : f1439d68
> > r3 : c2091040  r2 : ffffffd8  r1 : f1439d68  r0 : 00000000
> > Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> > Control: 10c5387d  Table: 066b004a  DAC: 00000051
> > Register r0 information: NULL pointer
> > Register r1 information: 2-page vmalloc region starting at 0xf1438000 allocated at kernel_clone+0x9c/0x390
> > Register r2 information: non-paged memory
> > Register r3 information: slab kmalloc-2k start c2091000 pointer offset 64 size 2048
> > Register r4 information: 2-page vmalloc region starting at 0xf1438000 allocated at kernel_clone+0x9c/0x390
> > Register r5 information: NULL pointer
> > Register r6 information: slab kmalloc-cg-4k start c238f000 pointer offset 0 size 4096
> > Register r7 information: 15-page vmalloc region starting at 0xbf0a8000 allocated at load_module+0xa30/0x219c
> > Register r8 information: 1-page vmalloc region starting at 0xf0d75000 allocated at ethtool_get_stats+0x138/0x208
> > Register r9 information: slab task_struct start c4816b80 pointer offset 0
> > Register r10 information: non-paged memory
> > Register r11 information: non-paged memory
> > Register r12 information: 2-page vmalloc region starting at 0xf1438000 allocated at kernel_clone+0x9c/0x390
> > Process snmpd (pid: 733, stack limit = 0x38de3a88)
> > Stack: (0xf1439d48 to 0xf143a000)
> > 9d40:                   000000c0 00000001 c238f000 bf0b400c f0d75150 c4816b80
> > 9d60: 00000100 bf0a98d8 00000000 00000000 00000000 00000000 00000000 00000000
> > 9d80: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> > 9da0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> > 9dc0: 00000dc0 5335509c 00000035 c238f000 bf0b2214 01067f50 f0d75000 c0b9b9c8
> > 9de0: 0000001d 00000035 c2212094 5335509c c4816b80 c238f000 c5ad6e00 01067f50
> > 9e00: c1b0be80 c4816b80 00014813 c0b9d7f0 00000000 00000000 0000001d 0000001d
> > 9e20: 00000000 00001200 00000000 00000000 c216ed90 c73943b8 00000000 00000000
> > 9e40: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> > 9e60: 00000000 c0ad9034 00000000 00000000 00000000 00000000 00000000 00000000
> > 9e80: 00000000 00000000 00000000 5335509c c1b0be80 f1439ee4 00008946 c1b0be80
> > 9ea0: 01067f50 f1439ee3 00000000 00000046 b6d77ae0 c0b383f0 00008946 becc83e8
> > 9ec0: c1b0be80 00000051 0000000b c68ca480 c7172d00 c0ad8ff0 f1439ee3 cf600e40
> > 9ee0: 01600e40 32687465 00000000 00000000 00000000 01067f50 00000000 00000000
> > 9f00: 00000000 5335509c 00008946 00008946 00000000 c68ca480 becc83e8 c05e2de0
> > 9f20: f1439fb0 c03002f0 00000006 5ac3c35a c4816b80 00000006 b6d77ae0 c030caf0
> > 9f40: c4817350 00000014 f1439e1c 0000000c 00000000 00000051 01000000 00000014
> > 9f60: 00003fec f1439edc 00000001 c0372abc b6d77ae0 c0372abc cf600e40 5335509c
> > 9f80: c21e6800 01015c9c 0000000b 00008946 00000036 c03002f0 c4816b80 00000036
> > 9fa0: b6d77ae0 c03000c0 01015c9c 0000000b 0000000b 00008946 becc83e8 00000000
> > 9fc0: 01015c9c 0000000b 00008946 00000036 00000035 010678a0 b6d797ec b6d77ae0
> > 9fe0: b6dbf738 becc838c b6d186d7 b6baa858 40000030 0000000b 00000000 00000000
> >  page_pool_get_stats from mvneta_ethtool_get_stats+0xa0/0xe0 [mvneta]
> >  mvneta_ethtool_get_stats [mvneta] from ethtool_get_stats+0x154/0x208
> >  ethtool_get_stats from dev_ethtool+0xf48/0x2480
> >  dev_ethtool from dev_ioctl+0x538/0x63c
> >  dev_ioctl from sock_ioctl+0x49c/0x53c
> >  sock_ioctl from sys_ioctl+0x134/0xbd8
> >  sys_ioctl from ret_fast_syscall+0x0/0x1c
> > Exception stack(0xf1439fa8 to 0xf1439ff0)
> > 9fa0:                   01015c9c 0000000b 0000000b 00008946 becc83e8 00000000
> > 9fc0: 01015c9c 0000000b 00008946 00000036 00000035 010678a0 b6d797ec b6d77ae0
> > 9fe0: b6dbf738 becc838c b6d186d7 b6baa858
> > Code: e28dd004 e1a05000 e2514000 0a00006a (e5902070)
> > 
> > This commit adds the proper checks before calling page_pool_get_stats.
> > 
> > Fixes: b3fc79225f05 ("net: mvneta: add support for page_pool_get_stats")
> > Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>
> > Reported-by: Paulo Da Silva <Paulo.DaSilva@kyberna.com>
> > ---
> 
> Hi Sven,
> 
> first of all thx for fixing it. Just minor comments inline.
> 
> Regards,
> Lorenzo
> 
> > 
> > Change from v3:
> > 	* Move the page pool check back to mvneta
> > 
> > Change from v2:
> > 	* Fix the fixes tag
> > 
> > Change from v1:
> > 	* Add cover letter
> > 	* Move the page pool check in mvneta to the ethtool stats
> > 	  function
> > 
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> > index 8b0f12a0e0f2..bbb5d972657a 100644
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > @@ -4734,13 +4734,16 @@ static void mvneta_ethtool_get_strings(struct net_device *netdev, u32 sset,
> >  {
> >  	if (sset == ETH_SS_STATS) {
> >  		int i;
> > +		struct mvneta_port *pp = netdev_priv(netdev);
> 
> nit: reverse christmas tree here (just if you need to repost)
> 
> >  
> >  		for (i = 0; i < ARRAY_SIZE(mvneta_statistics); i++)
> >  			memcpy(data + i * ETH_GSTRING_LEN,
> >  			       mvneta_statistics[i].name, ETH_GSTRING_LEN);
> >  
> > -		data += ETH_GSTRING_LEN * ARRAY_SIZE(mvneta_statistics);
> > -		page_pool_ethtool_stats_get_strings(data);
> > +		if (!pp->bm_priv) {
> > +			data += ETH_GSTRING_LEN * ARRAY_SIZE(mvneta_statistics);
> > +			page_pool_ethtool_stats_get_strings(data);
> > +		}
> >  	}
> >  }
> >  
> > @@ -4858,8 +4861,10 @@ static void mvneta_ethtool_pp_stats(struct mvneta_port *pp, u64 *data)
> >  	struct page_pool_stats stats = {};
> >  	int i;
> >  
> > -	for (i = 0; i < rxq_number; i++)
> > -		page_pool_get_stats(pp->rxqs[i].page_pool, &stats);
> > +	for (i = 0; i < rxq_number; i++) {
> > +		if (pp->rxqs[i].page_pool)
> > +			page_pool_get_stats(pp->rxqs[i].page_pool, &stats);
> > +	}
> >  
> >  	page_pool_ethtool_stats_get(data, &stats);
> >  }
> > @@ -4875,14 +4880,21 @@ static void mvneta_ethtool_get_stats(struct net_device *dev,
> >  	for (i = 0; i < ARRAY_SIZE(mvneta_statistics); i++)
> >  		*data++ = pp->ethtool_stats[i];
> >  
> > -	mvneta_ethtool_pp_stats(pp, data);
> > +	if (!pp->bm_priv && !pp->is_stopped)
> 
> do we need to check pp->is_stopped here? (we already check if page_pool
> pointer is NULL in mvneta_ethtool_pp_stats).
> Moreover in mvneta_ethtool_get_sset_count() and in mvneta_ethtool_get_strings()
> we just check pp->bm_priv pointer. Are the stats disaligned in this case?

Hi Lorenzo,

so the buffer manager (bm) does not support the page pool.
If this mode is used we can skip any page pool references.

The question is do we end up with a race condition when we skip the is_stopped check
as the variable is set to true just before the page pools are
deallocated on suspend or interface stop calls.

Best
Sven

> 
> > +		mvneta_ethtool_pp_stats(pp, data);
> >  }
> >  
> >  static int mvneta_ethtool_get_sset_count(struct net_device *dev, int sset)
> >  {
> > -	if (sset == ETH_SS_STATS)
> > -		return ARRAY_SIZE(mvneta_statistics) +
> > -		       page_pool_ethtool_stats_get_count();
> > +	if (sset == ETH_SS_STATS) {
> > +		int count = ARRAY_SIZE(mvneta_statistics);
> > +		struct mvneta_port *pp = netdev_priv(dev);
> > +
> > +		if (!pp->bm_priv)
> > +			count += page_pool_ethtool_stats_get_count();
> > +
> > +		return count;
> > +	}
> >  
> >  	return -EOPNOTSUPP;
> >  }
> > -- 
> > 2.42.0
> >
Lorenzo Bianconi Nov. 9, 2023, 11:31 a.m. UTC | #3 
[...]
> > 
> > do we need to check pp->is_stopped here? (we already check if page_pool
> > pointer is NULL in mvneta_ethtool_pp_stats).
> > Moreover in mvneta_ethtool_get_sset_count() and in mvneta_ethtool_get_strings()
> > we just check pp->bm_priv pointer. Are the stats disaligned in this case?
> 
> Hi Lorenzo,
> 
> so the buffer manager (bm) does not support the page pool.
> If this mode is used we can skip any page pool references.
> 
> The question is do we end up with a race condition when we skip the is_stopped check
> as the variable is set to true just before the page pools are
> deallocated on suspend or interface stop calls.

Do we really have a race here? e.g. both ndo_stop and ethtool_get_stats() run under rtnl.
Moreover it seems is_stopped is not set for armada-3700 devices (e.g. my
espressobin). Do we have the issue for this kind of devices?

Regards,
Lorenzo

> 
> Best
> Sven
> 
> > 
> > > +		mvneta_ethtool_pp_stats(pp, data);
> > >  }
> > >  
> > >  static int mvneta_ethtool_get_sset_count(struct net_device *dev, int sset)
> > >  {
> > > -	if (sset == ETH_SS_STATS)
> > > -		return ARRAY_SIZE(mvneta_statistics) +
> > > -		       page_pool_ethtool_stats_get_count();
> > > +	if (sset == ETH_SS_STATS) {
> > > +		int count = ARRAY_SIZE(mvneta_statistics);
> > > +		struct mvneta_port *pp = netdev_priv(dev);
> > > +
> > > +		if (!pp->bm_priv)
> > > +			count += page_pool_ethtool_stats_get_count();
> > > +
> > > +		return count;
> > > +	}
> > >  
> > >  	return -EOPNOTSUPP;
> > >  }
> > > -- 
> > > 2.42.0
> > > 
> 
>
Sven Auhagen Nov. 10, 2023, 4:22 a.m. UTC | #4 
On Thu, Nov 09, 2023 at 12:31:45PM +0100, Lorenzo Bianconi wrote:
> [...]
> > > 
> > > do we need to check pp->is_stopped here? (we already check if page_pool
> > > pointer is NULL in mvneta_ethtool_pp_stats).
> > > Moreover in mvneta_ethtool_get_sset_count() and in mvneta_ethtool_get_strings()
> > > we just check pp->bm_priv pointer. Are the stats disaligned in this case?
> > 
> > Hi Lorenzo,
> > 
> > so the buffer manager (bm) does not support the page pool.
> > If this mode is used we can skip any page pool references.
> > 
> > The question is do we end up with a race condition when we skip the is_stopped check
> > as the variable is set to true just before the page pools are
> > deallocated on suspend or interface stop calls.
> 
> Do we really have a race here? e.g. both ndo_stop and ethtool_get_stats() run under rtnl.
> Moreover it seems is_stopped is not set for armada-3700 devices (e.g. my
> espressobin). Do we have the issue for this kind of devices?
> 
> Regards,
> Lorenzo

We are fine then if ethtool get stats is under rntl.
I am testing on a clearfog base so is_stopped is set on my device.

Let me remove the check and also fix the variable order
and send a new version.

Best
Sven

> 
> > 
> > Best
> > Sven
> > 
> > > 
> > > > +		mvneta_ethtool_pp_stats(pp, data);
> > > >  }
> > > >  
> > > >  static int mvneta_ethtool_get_sset_count(struct net_device *dev, int sset)
> > > >  {
> > > > -	if (sset == ETH_SS_STATS)
> > > > -		return ARRAY_SIZE(mvneta_statistics) +
> > > > -		       page_pool_ethtool_stats_get_count();
> > > > +	if (sset == ETH_SS_STATS) {
> > > > +		int count = ARRAY_SIZE(mvneta_statistics);
> > > > +		struct mvneta_port *pp = netdev_priv(dev);
> > > > +
> > > > +		if (!pp->bm_priv)
> > > > +			count += page_pool_ethtool_stats_get_count();
> > > > +
> > > > +		return count;
> > > > +	}
> > > >  
> > > >  	return -EOPNOTSUPP;
> > > >  }
> > > > -- 
> > > > 2.42.0
> > > > 
> > 
> >
diff mbox series

Patch

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 8b0f12a0e0f2..bbb5d972657a 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -4734,13 +4734,16 @@  static void mvneta_ethtool_get_strings(struct net_device *netdev, u32 sset,
 {
 	if (sset == ETH_SS_STATS) {
 		int i;
+		struct mvneta_port *pp = netdev_priv(netdev);
 
 		for (i = 0; i < ARRAY_SIZE(mvneta_statistics); i++)
 			memcpy(data + i * ETH_GSTRING_LEN,
 			       mvneta_statistics[i].name, ETH_GSTRING_LEN);
 
-		data += ETH_GSTRING_LEN * ARRAY_SIZE(mvneta_statistics);
-		page_pool_ethtool_stats_get_strings(data);
+		if (!pp->bm_priv) {
+			data += ETH_GSTRING_LEN * ARRAY_SIZE(mvneta_statistics);
+			page_pool_ethtool_stats_get_strings(data);
+		}
 	}
 }
 
@@ -4858,8 +4861,10 @@  static void mvneta_ethtool_pp_stats(struct mvneta_port *pp, u64 *data)
 	struct page_pool_stats stats = {};
 	int i;
 
-	for (i = 0; i < rxq_number; i++)
-		page_pool_get_stats(pp->rxqs[i].page_pool, &stats);
+	for (i = 0; i < rxq_number; i++) {
+		if (pp->rxqs[i].page_pool)
+			page_pool_get_stats(pp->rxqs[i].page_pool, &stats);
+	}
 
 	page_pool_ethtool_stats_get(data, &stats);
 }
@@ -4875,14 +4880,21 @@  static void mvneta_ethtool_get_stats(struct net_device *dev,
 	for (i = 0; i < ARRAY_SIZE(mvneta_statistics); i++)
 		*data++ = pp->ethtool_stats[i];
 
-	mvneta_ethtool_pp_stats(pp, data);
+	if (!pp->bm_priv && !pp->is_stopped)
+		mvneta_ethtool_pp_stats(pp, data);
 }
 
 static int mvneta_ethtool_get_sset_count(struct net_device *dev, int sset)
 {
-	if (sset == ETH_SS_STATS)
-		return ARRAY_SIZE(mvneta_statistics) +
-		       page_pool_ethtool_stats_get_count();
+	if (sset == ETH_SS_STATS) {
+		int count = ARRAY_SIZE(mvneta_statistics);
+		struct mvneta_port *pp = netdev_priv(dev);
+
+		if (!pp->bm_priv)
+			count += page_pool_ethtool_stats_get_count();
+
+		return count;
+	}
 
 	return -EOPNOTSUPP;
 }