netdev - Re: [PATCH net-next 2/3] net: enetc: separate 64-bit counters from enetc_port

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250621103623.GB71935@horms.kernel.org>
Date: Sat, 21 Jun 2025 11:36:23 +0100
From: Simon Horman <horms@...nel.org>
To: Wei Fang <wei.fang@....com>
Cc: claudiu.manoil@....com, vladimir.oltean@....com, xiaoning.wang@....com,
	andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, imx@...ts.linux.dev
Subject: Re: [PATCH net-next 2/3] net: enetc: separate 64-bit counters from
 enetc_port_counters

On Fri, Jun 20, 2025 at 06:21:39PM +0800, Wei Fang wrote:
> Some counters in enetc_port_counters are 32-bit registers, and some are
> 64-bit registers. But in the current driver, they are all read through
> enetc_port_rd(), which can only read a 32-bit value. Therefore, separate
> 64-bit counters (enetc_pm_counters) from enetc_port_counters and use
> enetc_port_rd64() to read the 64-bit statistics.
> 
> Signed-off-by: Wei Fang <wei.fang@....com>

This patch looks fine to me, as does the following one.
However, they multiple sparse warnings relating
to endianness handling in the ioread32() version of _enetc_rd_reg64().

I've collected together my thoughts on that in the form of a patch.
And I'd appreciate it if we could resolve this one way or another.

From: Simon Horman <horms@...nel.org>
Subject: [PATCH RFC net] net: enetc: Correct endianness handling in
 _enetc_rd_reg64

enetc_hw.h provides two versions of _enetc_rd_reg64.
One which simply calls ioread64() when available.
And another that composes the 64-bit result from ioread32() calls.

In the second case the code appears to assume that each ioread32()
call returns a little-endian value. The high and the low 32 bit
values are then combined to make a 64-bit value which is then
converted to host byte order.

However, both the bit shift and the logical or used to combine
the two 32-bit values assume that they are operating on host-byte
order entities. This seems broken and I assume that the code
has only been tested on little endian systems.

Correct this by converting the 32-bit little endian values
to host byte order before operating on them.

Also, use little endian types to store these values, to make
the logic clearer and is moreover good practice.

Flagged by Sparse

Fixes: 69c663660b06 ("net: enetc: Correct endianness handling in _enetc_rd_reg64")
Signed-off-by: Simon Horman <horms@...nel.org>
---
I have marked this as RFC as I am unsure that the above is correct.

The version of _enetc_rd_reg64() that is a trivial wrapper around
ioread64() assumes that the call to ioread64() returns a host byte order
value?

If that is the case then is it also the case that the ioread32() calls,
in this version of _enetc_rd_reg64() also return host byte order values.
And if so, it is probably sufficient for this version to keep using u32
as the type for low, high, and tmp.  And simply:

	return high << 32 | low;
---
 drivers/net/ethernet/freescale/enetc/enetc_hw.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
index cb26f185f52f..3f40fcdbc4a7 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
@@ -502,15 +502,15 @@ static inline u64 _enetc_rd_reg64(void __iomem *reg)
 /* using this to read out stats on 32b systems */
 static inline u64 _enetc_rd_reg64(void __iomem *reg)
 {
-	u32 low, high, tmp;
+	__le32 low, high, tmp;

 	do {
-		high = ioread32(reg + 4);
-		low = ioread32(reg);
-		tmp = ioread32(reg + 4);
+		high = (__force __le32)ioread32(reg + 4);
+		low = (__force __le32)ioread32(reg);
+		tmp = (__force __le32)ioread32(reg + 4);
 	} while (high != tmp);

-	return le64_to_cpu((__le64)high << 32 | low);
+	return (u64)le32_to_cpu(high) << 32 | le32_to_cpu(low);
 }
 #endif

-- 
2.47.2