[PATCH net v1] net: stmmac: Fix queue statistics reading

[PATCH net v1] net: stmmac: Fix queue statistics reading

[Kurt Kanzenbach]

Correct queue statistics reading. All queue statistics are stored as unsigned
long values. The retrieval for ethtool fetches these values as u64. However, on
some systems the size of the counters are 32 bit. That yields wrong queue
statistic counters e.g., on arm32 systems such as the stm32mp157. Fix it by
using the correct data type.

Tested on Olimex STMP157-OLinuXino-LIME2 by simple running linuxptp for a short
period of time:

Non-patched kernel:
|root@st1:~# ethtool -S eth0 | grep q0
|     q0_tx_pkt_n: 3775276254951 # ???
|     q0_tx_irq_n: 879
|     q0_rx_pkt_n: 1194000908909 # ???
|     q0_rx_irq_n: 278

Patched kernel:
|root@st1:~# ethtool -S eth0 | grep q0
|     q0_tx_pkt_n: 2434
|     q0_tx_irq_n: 1274
|     q0_rx_pkt_n: 1604
|     q0_rx_irq_n: 846

Fixes: 68e9c5dee1cf ("net: stmmac: add ethtool per-queue statistic framework")
Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
Cc: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
Cc: Wong Vee Khee <vee.khee.wong@linux.intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index f453b0d09366..35c8dd92d369 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -551,16 +551,16 @@ static void stmmac_get_per_qstats(struct stmmac_priv *priv, u64 *data)
 		p = (char *)priv + offsetof(struct stmmac_priv,
 					    xstats.txq_stats[q].tx_pkt_n);
 		for (stat = 0; stat < STMMAC_TXQ_STATS; stat++) {
-			*data++ = (*(u64 *)p);
-			p += sizeof(u64 *);
+			*data++ = (*(unsigned long *)p);
+			p += sizeof(unsigned long);
 		}
 	}
 	for (q = 0; q < rx_cnt; q++) {
 		p = (char *)priv + offsetof(struct stmmac_priv,
 					    xstats.rxq_stats[q].rx_pkt_n);
 		for (stat = 0; stat < STMMAC_RXQ_STATS; stat++) {
-			*data++ = (*(u64 *)p);
-			p += sizeof(u64 *);
+			*data++ = (*(unsigned long *)p);
+			p += sizeof(unsigned long);
 		}
 	}
 }
-- 
2.30.2

Re: [PATCH net v1] net: stmmac: Fix queue statistics reading

[Andrew Lunn]

On Sat, Jan 14, 2023 at 01:04:37PM +0100, Kurt Kanzenbach wrote:
> Correct queue statistics reading. All queue statistics are stored as unsigned
> long values. The retrieval for ethtool fetches these values as u64. However, on
> some systems the size of the counters are 32 bit.

> @@ -551,16 +551,16 @@ static void stmmac_get_per_qstats(struct stmmac_priv *priv, u64 *data)
>  		p = (char *)priv + offsetof(struct stmmac_priv,
>  					    xstats.txq_stats[q].tx_pkt_n);
>  		for (stat = 0; stat < STMMAC_TXQ_STATS; stat++) {
> -			*data++ = (*(u64 *)p);
> -			p += sizeof(u64 *);
> +			*data++ = (*(unsigned long *)p);
> +			p += sizeof(unsigned long);

As you said in the comment, the register is 32 bits. So maybe u32
would be better than unsigned long? And it would also avoid issues if
this code is every used on a 64 bit machine.

     Andrew

Re: [PATCH net v1] net: stmmac: Fix queue statistics reading

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 1719 bytes --]

Hi Andrew,

On Sat Jan 14 2023, Andrew Lunn wrote:
> On Sat, Jan 14, 2023 at 01:04:37PM +0100, Kurt Kanzenbach wrote:
>> Correct queue statistics reading. All queue statistics are stored as unsigned
>> long values. The retrieval for ethtool fetches these values as u64. However, on
>> some systems the size of the counters are 32 bit.
>
>> @@ -551,16 +551,16 @@ static void stmmac_get_per_qstats(struct stmmac_priv *priv, u64 *data)
>>  		p = (char *)priv + offsetof(struct stmmac_priv,
>>  					    xstats.txq_stats[q].tx_pkt_n);
>>  		for (stat = 0; stat < STMMAC_TXQ_STATS; stat++) {
>> -			*data++ = (*(u64 *)p);
>> -			p += sizeof(u64 *);
>> +			*data++ = (*(unsigned long *)p);
>> +			p += sizeof(unsigned long);
>
> As you said in the comment, the register is 32 bits.

Maybe the commit message is a bit misleading. There are no registers
involved here. The queue statistics are accounted in software. See
stmmac_txq_stats and stmmac_rxq_stats.

> So maybe u32 would be better than unsigned long?

The problem is pkt_n and irq_n are stored as longs. The size of these
are either 4 or 8 byte depending on the architecture this code runs
on. I my opinion we cannot use u32 or u64 for that reason.

BTW, all the other stmmac statistic counters follow a different
pattern. For example:

for (i = 0; i < STMMAC_MMC_STATS_LEN; i++) {
	char *p;
	p = (char *)priv + stmmac_mmc[i].stat_offset;

	data[j++] = (stmmac_mmc[i].sizeof_stat ==
		     sizeof(u64)) ? (*(u64 *)p) :
		     (*(u32 *)p);
}

> And it would also avoid issues if this code is every used on a 64 bit
> machine.

The current code runs fine on 64 bit architectures such as Intel EHL,
TGL, ADL, ... I'm trying to fix it for the 32 bit case.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH net v1] net: stmmac: Fix queue statistics reading

[Andrew Lunn]

On Sun, Jan 15, 2023 at 12:25:51PM +0100, Kurt Kanzenbach wrote:
> Hi Andrew,
> 
> On Sat Jan 14 2023, Andrew Lunn wrote:
> > On Sat, Jan 14, 2023 at 01:04:37PM +0100, Kurt Kanzenbach wrote:
> >> Correct queue statistics reading. All queue statistics are stored as unsigned
> >> long values. The retrieval for ethtool fetches these values as u64. However, on
> >> some systems the size of the counters are 32 bit.
> >
> >> @@ -551,16 +551,16 @@ static void stmmac_get_per_qstats(struct stmmac_priv *priv, u64 *data)
> >>  		p = (char *)priv + offsetof(struct stmmac_priv,
> >>  					    xstats.txq_stats[q].tx_pkt_n);
> >>  		for (stat = 0; stat < STMMAC_TXQ_STATS; stat++) {
> >> -			*data++ = (*(u64 *)p);
> >> -			p += sizeof(u64 *);
> >> +			*data++ = (*(unsigned long *)p);
> >> +			p += sizeof(unsigned long);
> >
> > As you said in the comment, the register is 32 bits.
> 
> Maybe the commit message is a bit misleading. There are no registers
> involved here.

Ah!

In that case, yes, unsigned long. Maybe reword the commit message to
mention the values are being read from struct stmmac_txq_stats and
struct stmmac_rxq_stats which use unsigned long. That would avoid my
confusion of thinking it is being read from a register.

With that change made, you can add my Reviewed-by.

    Andrew

Re: [PATCH net v1] net: stmmac: Fix queue statistics reading

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Sat, 14 Jan 2023 13:04:37 +0100 you wrote:
> Correct queue statistics reading. All queue statistics are stored as unsigned
> long values. The retrieval for ethtool fetches these values as u64. However, on
> some systems the size of the counters are 32 bit. That yields wrong queue
> statistic counters e.g., on arm32 systems such as the stm32mp157. Fix it by
> using the correct data type.
> 
> Tested on Olimex STMP157-OLinuXino-LIME2 by simple running linuxptp for a short
> period of time:
> 
> [...]

Here is the summary with links:
  - [net,v1] net: stmmac: Fix queue statistics reading
    https://git.kernel.org/netdev/net/c/c296c77efb66

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html