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

[Kurt Kanzenbach <kurt@linutronix.de>]

[-- 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 --]