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

From: Kurt Kanzenbach <kurt@linutronix.de>
To: Andrew Lunn <andrew@lunn.ch>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>,
	Alexandre Torgue <alexandre.torgue@foss.st.com>,
	Jose Abreu <joabreu@synopsys.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org,
	Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>,
	Wong Vee Khee <vee.khee.wong@linux.intel.com>
Subject: Re: [PATCH net v1] net: stmmac: Fix queue statistics reading
Date: Sun, 15 Jan 2023 12:25:51 +0100	[thread overview]
Message-ID: <87fsccvy2o.fsf@kurt> (raw)
In-Reply-To: <Y8LFyqcpi6RjcjMo@lunn.ch>

[-- Attachment #1.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel