| Message ID | 20230114120437.383514-1-kurt@linutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c296c77efb66994d94d9f706446a115581226550 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v1] net: stmmac: Fix queue statistics reading | expand |
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
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
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
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!
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); } } }
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(-)