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

[Simon Horman <horms@kernel.org>]

On Mon, Jun 23, 2025 at 03:13:16AM +0000, Wei Fang wrote:
> > 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@kernel.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")
> 
> I think the fixes tag should be:
> Fixes: 16eb4c85c964 ("enetc: Add ethtool statistics")

Yes, thanks. I did notice that too, but somehow I managed
to post the wrong tag. Oops.

> 
> > Signed-off-by: Simon Horman <horms@kernel.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?
> 
> Yes, ioread64() returns a host endian value, below is the definition
> of ioread64() in include/asm-generic/io.h.
> 
> static inline u64 ioread64(const volatile void __iomem *addr)
> {
> 	return readq(addr);
> }
> 
> static inline u64 readq(const volatile void __iomem *addr)
> {
> 	u64 val;
> 
> 	log_read_mmio(64, addr, _THIS_IP_, _RET_IP_);
> 	__io_br();
> 	val = __le64_to_cpu((__le64 __force)__raw_readq(addr));
> 	__io_ar(val);
> 	log_post_read_mmio(val, 64, addr, _THIS_IP_, _RET_IP_);
> 	return val;
> }
> 
> And ioread32() is also defined similarly, so ioread32() also returns a
> host endian value.
> 
> static inline u32 ioread32(const volatile void __iomem *addr)
> {
> 	return readl(addr);
> }
> 
> static inline u32 readl(const volatile void __iomem *addr)
> {
> 	u32 val;
> 
> 	log_read_mmio(32, addr, _THIS_IP_, _RET_IP_);
> 	__io_br();
> 	val = __le32_to_cpu((__le32 __force)__raw_readl(addr));
> 	__io_ar(val);
> 	log_post_read_mmio(val, 32, addr, _THIS_IP_, _RET_IP_);
> 	return val;
> }
> > 
> > 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;
> 
> Yes, this change is enough. BTW, currently, the platforms using ENETC
> are all arm64, so ioread64() is used to read registers. Therefore, it does
> not cause any problems in actual use. However, from the driver's
> perspective, it should indeed be fixed. Thanks very much.

Thanks.

I'll send out an updated patch.