[PATCH net-next 0/3] change some statistics to 64-bit

[PATCH net-next 0/3] change some statistics to 64-bit

[Wei Fang]

The port MAC counters of ENETC are 64-bit registers and the statistics
of ethtool are also u64 type, so add enetc_port_rd64() helper function
to read 64-bit statistics from these registers, and also change the
statistics of ring to unsigned long type to be consistent with the
statistics type in struct net_device_stats.

Wei Fang (3):
  net: enetc: change the statistics of ring to unsigned long type
  net: enetc: separate 64-bit counters from enetc_port_counters
  net: enetc: read 64-bit statistics from port MAC counters

 drivers/net/ethernet/freescale/enetc/enetc.h  | 22 ++---
 .../ethernet/freescale/enetc/enetc_ethtool.c  | 99 +++++++++++--------
 .../net/ethernet/freescale/enetc/enetc_hw.h   |  1 +
 3 files changed, 68 insertions(+), 54 deletions(-)

-- 
2.34.1

[PATCH net-next 1/3] net: enetc: change the statistics of ring to unsigned long type

[Wei Fang]

The statistics of the ring are all unsigned int type, so the statistics
will overflow quickly under heavy traffic. In addition, the statistics
of struct net_device_stats are obtained from struct enetc_ring_stats,
but the statistics of net_device_stats are all unsigned long type.
Considering these two factors, the statistics of enetc_ring_stats are
all changed to unsigned long type.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc.h | 22 ++++++++++----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index 872d2cbd088b..62e8ee4d2f04 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -96,17 +96,17 @@ struct enetc_rx_swbd {
 #define ENETC_TXBDS_MAX_NEEDED(x)	ENETC_TXBDS_NEEDED((x) + 1)
 
 struct enetc_ring_stats {
-	unsigned int packets;
-	unsigned int bytes;
-	unsigned int rx_alloc_errs;
-	unsigned int xdp_drops;
-	unsigned int xdp_tx;
-	unsigned int xdp_tx_drops;
-	unsigned int xdp_redirect;
-	unsigned int xdp_redirect_failures;
-	unsigned int recycles;
-	unsigned int recycle_failures;
-	unsigned int win_drop;
+	unsigned long packets;
+	unsigned long bytes;
+	unsigned long rx_alloc_errs;
+	unsigned long xdp_drops;
+	unsigned long xdp_tx;
+	unsigned long xdp_tx_drops;
+	unsigned long xdp_redirect;
+	unsigned long xdp_redirect_failures;
+	unsigned long recycles;
+	unsigned long recycle_failures;
+	unsigned long win_drop;
 };
 
 struct enetc_xdp_data {
-- 
2.34.1

RE: [PATCH net-next 1/3] net: enetc: change the statistics of ring to unsigned long type

[Claudiu Manoil]

> -----Original Message-----
> From: Wei Fang <wei.fang@nxp.com>
> Sent: Friday, June 20, 2025 1:22 PM
[...]
> Subject: [PATCH net-next 1/3] net: enetc: change the statistics of ring to
> unsigned long type
> 
> The statistics of the ring are all unsigned int type, so the statistics
> will overflow quickly under heavy traffic. In addition, the statistics
> of struct net_device_stats are obtained from struct enetc_ring_stats,
> but the statistics of net_device_stats are all unsigned long type.
> Considering these two factors, the statistics of enetc_ring_stats are
> all changed to unsigned long type.
> 
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---

Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com>

Re: [PATCH net-next 1/3] net: enetc: change the statistics of ring to unsigned long type

[Simon Horman]

On Fri, Jun 20, 2025 at 06:21:38PM +0800, Wei Fang wrote:
> The statistics of the ring are all unsigned int type, so the statistics
> will overflow quickly under heavy traffic. In addition, the statistics
> of struct net_device_stats are obtained from struct enetc_ring_stats,
> but the statistics of net_device_stats are all unsigned long type.
> Considering these two factors, the statistics of enetc_ring_stats are
> all changed to unsigned long type.
> 
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>  drivers/net/ethernet/freescale/enetc/enetc.h | 22 ++++++++++----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
> index 872d2cbd088b..62e8ee4d2f04 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.h
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.h
> @@ -96,17 +96,17 @@ struct enetc_rx_swbd {
>  #define ENETC_TXBDS_MAX_NEEDED(x)	ENETC_TXBDS_NEEDED((x) + 1)
>  
>  struct enetc_ring_stats {
> -	unsigned int packets;
> -	unsigned int bytes;
> -	unsigned int rx_alloc_errs;
> -	unsigned int xdp_drops;
> -	unsigned int xdp_tx;
> -	unsigned int xdp_tx_drops;
> -	unsigned int xdp_redirect;
> -	unsigned int xdp_redirect_failures;
> -	unsigned int recycles;
> -	unsigned int recycle_failures;
> -	unsigned int win_drop;
> +	unsigned long packets;
> +	unsigned long bytes;
> +	unsigned long rx_alloc_errs;
> +	unsigned long xdp_drops;
> +	unsigned long xdp_tx;
> +	unsigned long xdp_tx_drops;
> +	unsigned long xdp_redirect;
> +	unsigned long xdp_redirect_failures;
> +	unsigned long recycles;
> +	unsigned long recycle_failures;
> +	unsigned long win_drop;
>  };

Hi Wei fang,

If the desire is for an unsigned 64 bit integer, then
I think either u64 or unsigned long long would be good choices.

unsigned long may be 64bit or 32bit depending on the platform.

RE: [PATCH net-next 1/3] net: enetc: change the statistics of ring to unsigned long type

[Wei Fang]

> >  struct enetc_ring_stats {
> > -	unsigned int packets;
> > -	unsigned int bytes;
> > -	unsigned int rx_alloc_errs;
> > -	unsigned int xdp_drops;
> > -	unsigned int xdp_tx;
> > -	unsigned int xdp_tx_drops;
> > -	unsigned int xdp_redirect;
> > -	unsigned int xdp_redirect_failures;
> > -	unsigned int recycles;
> > -	unsigned int recycle_failures;
> > -	unsigned int win_drop;
> > +	unsigned long packets;
> > +	unsigned long bytes;
> > +	unsigned long rx_alloc_errs;
> > +	unsigned long xdp_drops;
> > +	unsigned long xdp_tx;
> > +	unsigned long xdp_tx_drops;
> > +	unsigned long xdp_redirect;
> > +	unsigned long xdp_redirect_failures;
> > +	unsigned long recycles;
> > +	unsigned long recycle_failures;
> > +	unsigned long win_drop;
> >  };
> 
> Hi Wei fang,
> 
> If the desire is for an unsigned 64 bit integer, then I think either u64 or unsigned
> long long would be good choices.
> 
> unsigned long may be 64bit or 32bit depending on the platform.

The use of unsigned long is to keep it consistent with the statistical
value type in struct net_device_stats. Because some statistics in
net_device_stats come from enetc_ring_stats.

#define NET_DEV_STAT(FIELD)			\
	union {					\
		unsigned long FIELD;		\
		atomic_long_t __##FIELD;	\
	}

Re: [PATCH net-next 1/3] net: enetc: change the statistics of ring to unsigned long type

[Simon Horman]

On Mon, Jun 23, 2025 at 01:49:59AM +0000, Wei Fang wrote:
> > >  struct enetc_ring_stats {
> > > -	unsigned int packets;
> > > -	unsigned int bytes;
> > > -	unsigned int rx_alloc_errs;
> > > -	unsigned int xdp_drops;
> > > -	unsigned int xdp_tx;
> > > -	unsigned int xdp_tx_drops;
> > > -	unsigned int xdp_redirect;
> > > -	unsigned int xdp_redirect_failures;
> > > -	unsigned int recycles;
> > > -	unsigned int recycle_failures;
> > > -	unsigned int win_drop;
> > > +	unsigned long packets;
> > > +	unsigned long bytes;
> > > +	unsigned long rx_alloc_errs;
> > > +	unsigned long xdp_drops;
> > > +	unsigned long xdp_tx;
> > > +	unsigned long xdp_tx_drops;
> > > +	unsigned long xdp_redirect;
> > > +	unsigned long xdp_redirect_failures;
> > > +	unsigned long recycles;
> > > +	unsigned long recycle_failures;
> > > +	unsigned long win_drop;
> > >  };
> > 
> > Hi Wei fang,
> > 
> > If the desire is for an unsigned 64 bit integer, then I think either u64 or unsigned
> > long long would be good choices.
> > 
> > unsigned long may be 64bit or 32bit depending on the platform.
> 
> The use of unsigned long is to keep it consistent with the statistical
> value type in struct net_device_stats. Because some statistics in
> net_device_stats come from enetc_ring_stats.
> 
> #define NET_DEV_STAT(FIELD)			\
> 	union {					\
> 		unsigned long FIELD;		\
> 		atomic_long_t __##FIELD;	\
> 	}

Thanks, I understand. But in this case I think the patch description could
be reworded - unsigned int and unsigned long are the same thing on some
systems, and on such systems there is no overflow advantage of one over the
other.

RE: [PATCH net-next 1/3] net: enetc: change the statistics of ring to unsigned long type

[Wei Fang]

> On Mon, Jun 23, 2025 at 01:49:59AM +0000, Wei Fang wrote:
> > > >  struct enetc_ring_stats {
> > > > -	unsigned int packets;
> > > > -	unsigned int bytes;
> > > > -	unsigned int rx_alloc_errs;
> > > > -	unsigned int xdp_drops;
> > > > -	unsigned int xdp_tx;
> > > > -	unsigned int xdp_tx_drops;
> > > > -	unsigned int xdp_redirect;
> > > > -	unsigned int xdp_redirect_failures;
> > > > -	unsigned int recycles;
> > > > -	unsigned int recycle_failures;
> > > > -	unsigned int win_drop;
> > > > +	unsigned long packets;
> > > > +	unsigned long bytes;
> > > > +	unsigned long rx_alloc_errs;
> > > > +	unsigned long xdp_drops;
> > > > +	unsigned long xdp_tx;
> > > > +	unsigned long xdp_tx_drops;
> > > > +	unsigned long xdp_redirect;
> > > > +	unsigned long xdp_redirect_failures;
> > > > +	unsigned long recycles;
> > > > +	unsigned long recycle_failures;
> > > > +	unsigned long win_drop;
> > > >  };
> > >
> > > Hi Wei fang,
> > >
> > > If the desire is for an unsigned 64 bit integer, then I think either u64 or
> unsigned
> > > long long would be good choices.
> > >
> > > unsigned long may be 64bit or 32bit depending on the platform.
> >
> > The use of unsigned long is to keep it consistent with the statistical
> > value type in struct net_device_stats. Because some statistics in
> > net_device_stats come from enetc_ring_stats.
> >
> > #define NET_DEV_STAT(FIELD)			\
> > 	union {					\
> > 		unsigned long FIELD;		\
> > 		atomic_long_t __##FIELD;	\
> > 	}
> 
> Thanks, I understand. But in this case I think the patch description could
> be reworded - unsigned int and unsigned long are the same thing on some
> systems, and on such systems there is no overflow advantage of one over the
> other.

Okay, I will improve the commit message. Actually both LS1028A and i.MX95
are arm64 architecture, so for these platforms using ENETC IP, unsigned long
is 64-bit.

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

[Wei Fang]

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@nxp.com>
---
 .../net/ethernet/freescale/enetc/enetc_ethtool.c  | 15 ++++++++++++++-
 drivers/net/ethernet/freescale/enetc/enetc_hw.h   |  1 +
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
index 2e5cef646741..2c9aa94c8e3d 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
@@ -142,7 +142,7 @@ static const struct {
 static const struct {
 	int reg;
 	char name[ETH_GSTRING_LEN] __nonstring;
-} enetc_port_counters[] = {
+} enetc_pm_counters[] = {
 	{ ENETC_PM_REOCT(0),	"MAC rx ethernet octets" },
 	{ ENETC_PM_RALN(0),	"MAC rx alignment errors" },
 	{ ENETC_PM_RXPF(0),	"MAC rx valid pause frames" },
@@ -194,6 +194,12 @@ static const struct {
 	{ ENETC_PM_TSCOL(0),	"MAC tx single collisions" },
 	{ ENETC_PM_TLCOL(0),	"MAC tx late collisions" },
 	{ ENETC_PM_TECOL(0),	"MAC tx excessive collisions" },
+};
+
+static const struct {
+	int reg;
+	char name[ETH_GSTRING_LEN] __nonstring;
+} enetc_port_counters[] = {
 	{ ENETC_UFDMF,		"SI MAC nomatch u-cast discards" },
 	{ ENETC_MFDMF,		"SI MAC nomatch m-cast discards" },
 	{ ENETC_PBFDSIR,	"SI MAC nomatch b-cast discards" },
@@ -240,6 +246,7 @@ static int enetc_get_sset_count(struct net_device *ndev, int sset)
 		return len;
 
 	len += ARRAY_SIZE(enetc_port_counters);
+	len += ARRAY_SIZE(enetc_pm_counters);
 
 	return len;
 }
@@ -266,6 +273,9 @@ static void enetc_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
 		for (i = 0; i < ARRAY_SIZE(enetc_port_counters); i++)
 			ethtool_cpy(&data, enetc_port_counters[i].name);
 
+		for (i = 0; i < ARRAY_SIZE(enetc_pm_counters); i++)
+			ethtool_cpy(&data, enetc_pm_counters[i].name);
+
 		break;
 	}
 }
@@ -302,6 +312,9 @@ static void enetc_get_ethtool_stats(struct net_device *ndev,
 
 	for (i = 0; i < ARRAY_SIZE(enetc_port_counters); i++)
 		data[o++] = enetc_port_rd(hw, enetc_port_counters[i].reg);
+
+	for (i = 0; i < ARRAY_SIZE(enetc_pm_counters); i++)
+		data[o++] = enetc_port_rd64(hw, enetc_pm_counters[i].reg);
 }
 
 static void enetc_pause_stats(struct enetc_hw *hw, int mac,
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
index cb26f185f52f..d4bbb07199c5 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
@@ -536,6 +536,7 @@ static inline u64 _enetc_rd_reg64_wa(void __iomem *reg)
 /* port register accessors - PF only */
 #define enetc_port_rd(hw, off)		enetc_rd_reg((hw)->port + (off))
 #define enetc_port_wr(hw, off, val)	enetc_wr_reg((hw)->port + (off), val)
+#define enetc_port_rd64(hw, off)	_enetc_rd_reg64_wa((hw)->port + (off))
 #define enetc_port_rd_mdio(hw, off)	_enetc_rd_mdio_reg_wa((hw)->port + (off))
 #define enetc_port_wr_mdio(hw, off, val)	_enetc_wr_mdio_reg_wa(\
 							(hw)->port + (off), val)
-- 
2.34.1

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

[Claudiu Manoil]

> -----Original Message-----
> From: Wei Fang <wei.fang@nxp.com>
> Sent: Friday, June 20, 2025 1:22 PM
[...]
> Subject: [PATCH net-next 2/3] net: enetc: separate 64-bit counters from
> enetc_port_counters
> 
> 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@nxp.com>
> ---

Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com>

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

[Simon Horman]

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@nxp.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@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")
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?

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

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

[Wei Fang]

> 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")

> 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.

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

[Simon Horman]

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.

[PATCH net-next 3/3] net: enetc: read 64-bit statistics from port MAC counters

[Wei Fang]

The counters of port MAC are all 64-bit registers, and the statistics of
ethtool are u64 type, so replace enetc_port_rd() with enetc_port_rd64()
to read 64-bit statistics.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 .../ethernet/freescale/enetc/enetc_ethtool.c  | 84 +++++++++----------
 1 file changed, 42 insertions(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
index 2c9aa94c8e3d..961e76cd8489 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
@@ -320,8 +320,8 @@ static void enetc_get_ethtool_stats(struct net_device *ndev,
 static void enetc_pause_stats(struct enetc_hw *hw, int mac,
 			      struct ethtool_pause_stats *pause_stats)
 {
-	pause_stats->tx_pause_frames = enetc_port_rd(hw, ENETC_PM_TXPF(mac));
-	pause_stats->rx_pause_frames = enetc_port_rd(hw, ENETC_PM_RXPF(mac));
+	pause_stats->tx_pause_frames = enetc_port_rd64(hw, ENETC_PM_TXPF(mac));
+	pause_stats->rx_pause_frames = enetc_port_rd64(hw, ENETC_PM_RXPF(mac));
 }
 
 static void enetc_get_pause_stats(struct net_device *ndev,
@@ -348,31 +348,31 @@ static void enetc_get_pause_stats(struct net_device *ndev,
 static void enetc_mac_stats(struct enetc_hw *hw, int mac,
 			    struct ethtool_eth_mac_stats *s)
 {
-	s->FramesTransmittedOK = enetc_port_rd(hw, ENETC_PM_TFRM(mac));
-	s->SingleCollisionFrames = enetc_port_rd(hw, ENETC_PM_TSCOL(mac));
-	s->MultipleCollisionFrames = enetc_port_rd(hw, ENETC_PM_TMCOL(mac));
-	s->FramesReceivedOK = enetc_port_rd(hw, ENETC_PM_RFRM(mac));
-	s->FrameCheckSequenceErrors = enetc_port_rd(hw, ENETC_PM_RFCS(mac));
-	s->AlignmentErrors = enetc_port_rd(hw, ENETC_PM_RALN(mac));
-	s->OctetsTransmittedOK = enetc_port_rd(hw, ENETC_PM_TEOCT(mac));
-	s->FramesWithDeferredXmissions = enetc_port_rd(hw, ENETC_PM_TDFR(mac));
-	s->LateCollisions = enetc_port_rd(hw, ENETC_PM_TLCOL(mac));
-	s->FramesAbortedDueToXSColls = enetc_port_rd(hw, ENETC_PM_TECOL(mac));
-	s->FramesLostDueToIntMACXmitError = enetc_port_rd(hw, ENETC_PM_TERR(mac));
-	s->CarrierSenseErrors = enetc_port_rd(hw, ENETC_PM_TCRSE(mac));
-	s->OctetsReceivedOK = enetc_port_rd(hw, ENETC_PM_REOCT(mac));
-	s->FramesLostDueToIntMACRcvError = enetc_port_rd(hw, ENETC_PM_RDRNTP(mac));
-	s->MulticastFramesXmittedOK = enetc_port_rd(hw, ENETC_PM_TMCA(mac));
-	s->BroadcastFramesXmittedOK = enetc_port_rd(hw, ENETC_PM_TBCA(mac));
-	s->MulticastFramesReceivedOK = enetc_port_rd(hw, ENETC_PM_RMCA(mac));
-	s->BroadcastFramesReceivedOK = enetc_port_rd(hw, ENETC_PM_RBCA(mac));
+	s->FramesTransmittedOK = enetc_port_rd64(hw, ENETC_PM_TFRM(mac));
+	s->SingleCollisionFrames = enetc_port_rd64(hw, ENETC_PM_TSCOL(mac));
+	s->MultipleCollisionFrames = enetc_port_rd64(hw, ENETC_PM_TMCOL(mac));
+	s->FramesReceivedOK = enetc_port_rd64(hw, ENETC_PM_RFRM(mac));
+	s->FrameCheckSequenceErrors = enetc_port_rd64(hw, ENETC_PM_RFCS(mac));
+	s->AlignmentErrors = enetc_port_rd64(hw, ENETC_PM_RALN(mac));
+	s->OctetsTransmittedOK = enetc_port_rd64(hw, ENETC_PM_TEOCT(mac));
+	s->FramesWithDeferredXmissions = enetc_port_rd64(hw, ENETC_PM_TDFR(mac));
+	s->LateCollisions = enetc_port_rd64(hw, ENETC_PM_TLCOL(mac));
+	s->FramesAbortedDueToXSColls = enetc_port_rd64(hw, ENETC_PM_TECOL(mac));
+	s->FramesLostDueToIntMACXmitError = enetc_port_rd64(hw, ENETC_PM_TERR(mac));
+	s->CarrierSenseErrors = enetc_port_rd64(hw, ENETC_PM_TCRSE(mac));
+	s->OctetsReceivedOK = enetc_port_rd64(hw, ENETC_PM_REOCT(mac));
+	s->FramesLostDueToIntMACRcvError = enetc_port_rd64(hw, ENETC_PM_RDRNTP(mac));
+	s->MulticastFramesXmittedOK = enetc_port_rd64(hw, ENETC_PM_TMCA(mac));
+	s->BroadcastFramesXmittedOK = enetc_port_rd64(hw, ENETC_PM_TBCA(mac));
+	s->MulticastFramesReceivedOK = enetc_port_rd64(hw, ENETC_PM_RMCA(mac));
+	s->BroadcastFramesReceivedOK = enetc_port_rd64(hw, ENETC_PM_RBCA(mac));
 }
 
 static void enetc_ctrl_stats(struct enetc_hw *hw, int mac,
 			     struct ethtool_eth_ctrl_stats *s)
 {
-	s->MACControlFramesTransmitted = enetc_port_rd(hw, ENETC_PM_TCNP(mac));
-	s->MACControlFramesReceived = enetc_port_rd(hw, ENETC_PM_RCNP(mac));
+	s->MACControlFramesTransmitted = enetc_port_rd64(hw, ENETC_PM_TCNP(mac));
+	s->MACControlFramesReceived = enetc_port_rd64(hw, ENETC_PM_RCNP(mac));
 }
 
 static const struct ethtool_rmon_hist_range enetc_rmon_ranges[] = {
@@ -389,26 +389,26 @@ static const struct ethtool_rmon_hist_range enetc_rmon_ranges[] = {
 static void enetc_rmon_stats(struct enetc_hw *hw, int mac,
 			     struct ethtool_rmon_stats *s)
 {
-	s->undersize_pkts = enetc_port_rd(hw, ENETC_PM_RUND(mac));
-	s->oversize_pkts = enetc_port_rd(hw, ENETC_PM_ROVR(mac));
-	s->fragments = enetc_port_rd(hw, ENETC_PM_RFRG(mac));
-	s->jabbers = enetc_port_rd(hw, ENETC_PM_RJBR(mac));
-
-	s->hist[0] = enetc_port_rd(hw, ENETC_PM_R64(mac));
-	s->hist[1] = enetc_port_rd(hw, ENETC_PM_R127(mac));
-	s->hist[2] = enetc_port_rd(hw, ENETC_PM_R255(mac));
-	s->hist[3] = enetc_port_rd(hw, ENETC_PM_R511(mac));
-	s->hist[4] = enetc_port_rd(hw, ENETC_PM_R1023(mac));
-	s->hist[5] = enetc_port_rd(hw, ENETC_PM_R1522(mac));
-	s->hist[6] = enetc_port_rd(hw, ENETC_PM_R1523X(mac));
-
-	s->hist_tx[0] = enetc_port_rd(hw, ENETC_PM_T64(mac));
-	s->hist_tx[1] = enetc_port_rd(hw, ENETC_PM_T127(mac));
-	s->hist_tx[2] = enetc_port_rd(hw, ENETC_PM_T255(mac));
-	s->hist_tx[3] = enetc_port_rd(hw, ENETC_PM_T511(mac));
-	s->hist_tx[4] = enetc_port_rd(hw, ENETC_PM_T1023(mac));
-	s->hist_tx[5] = enetc_port_rd(hw, ENETC_PM_T1522(mac));
-	s->hist_tx[6] = enetc_port_rd(hw, ENETC_PM_T1523X(mac));
+	s->undersize_pkts = enetc_port_rd64(hw, ENETC_PM_RUND(mac));
+	s->oversize_pkts = enetc_port_rd64(hw, ENETC_PM_ROVR(mac));
+	s->fragments = enetc_port_rd64(hw, ENETC_PM_RFRG(mac));
+	s->jabbers = enetc_port_rd64(hw, ENETC_PM_RJBR(mac));
+
+	s->hist[0] = enetc_port_rd64(hw, ENETC_PM_R64(mac));
+	s->hist[1] = enetc_port_rd64(hw, ENETC_PM_R127(mac));
+	s->hist[2] = enetc_port_rd64(hw, ENETC_PM_R255(mac));
+	s->hist[3] = enetc_port_rd64(hw, ENETC_PM_R511(mac));
+	s->hist[4] = enetc_port_rd64(hw, ENETC_PM_R1023(mac));
+	s->hist[5] = enetc_port_rd64(hw, ENETC_PM_R1522(mac));
+	s->hist[6] = enetc_port_rd64(hw, ENETC_PM_R1523X(mac));
+
+	s->hist_tx[0] = enetc_port_rd64(hw, ENETC_PM_T64(mac));
+	s->hist_tx[1] = enetc_port_rd64(hw, ENETC_PM_T127(mac));
+	s->hist_tx[2] = enetc_port_rd64(hw, ENETC_PM_T255(mac));
+	s->hist_tx[3] = enetc_port_rd64(hw, ENETC_PM_T511(mac));
+	s->hist_tx[4] = enetc_port_rd64(hw, ENETC_PM_T1023(mac));
+	s->hist_tx[5] = enetc_port_rd64(hw, ENETC_PM_T1522(mac));
+	s->hist_tx[6] = enetc_port_rd64(hw, ENETC_PM_T1523X(mac));
 }
 
 static void enetc_get_eth_mac_stats(struct net_device *ndev,
-- 
2.34.1

RE: [PATCH net-next 3/3] net: enetc: read 64-bit statistics from port MAC counters

[Claudiu Manoil]

> -----Original Message-----
> From: Wei Fang <wei.fang@nxp.com>
> Sent: Friday, June 20, 2025 1:22 PM
[...]
> Subject: [PATCH net-next 3/3] net: enetc: read 64-bit statistics from port MAC
> counters
> 
> The counters of port MAC are all 64-bit registers, and the statistics of ethtool
> are u64 type, so replace enetc_port_rd() with enetc_port_rd64() to read 64-
> bit statistics.
> 
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---

Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com>