[PATCH net] net: enetc: Correct endianness handling in _enetc_rd_reg64

[PATCH net] net: enetc: Correct endianness handling in _enetc_rd_reg64

[Simon Horman]

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. However both the shift and logical or
used to compose the return value would not work correctly on big endian
systems if this were the case. Moreover, this is inconsistent with the
first case where the return value of ioread64() is assumed to be in host
byte order.

It appears that the correct approach is for both versions to treat the
return value of ioread*() functions as being in host byte order. And
this patch corrects the ioread32()-based version to do so.

This is a bug but would only manifest on big endian systems
that make use of the ioread32-based implementation of _enetc_rd_reg64.
While all in-tree users of this driver are little endian and
make use of the ioread64-based implementation of _enetc_rd_reg64.
Thus, no in-tree user of this driver is affected by this bug.

Flagged by Sparse.
Compile tested only.

Cc: Wei Fang <wei.fang@nxp.com>
Fixes: 16eb4c85c964 ("enetc: Add ethtool statistics")
Closes: https://lore.kernel.org/all/AM9PR04MB850500D3FC24FE23DEFCEA158879A@AM9PR04MB8505.eurprd04.prod.outlook.com/
Signed-off-by: Simon Horman <horms@kernel.org>
---
 drivers/net/ethernet/freescale/enetc/enetc_hw.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
index 4098f01479bc..53e8d18c7a34 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
@@ -507,7 +507,7 @@ static inline u64 _enetc_rd_reg64(void __iomem *reg)
 		tmp = ioread32(reg + 4);
 	} while (high != tmp);
 
-	return le64_to_cpu((__le64)high << 32 | low);
+	return (u64)high << 32 | low;
 }
 #endif
 

RE: [PATCH net] net: enetc: Correct endianness handling in _enetc_rd_reg64

[Wei Fang]

> 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. However both the shift and logical or
> used to compose the return value would not work correctly on big endian
> systems if this were the case. Moreover, this is inconsistent with the
> first case where the return value of ioread64() is assumed to be in host
> byte order.
>
> It appears that the correct approach is for both versions to treat the
> return value of ioread*() functions as being in host byte order. And
> this patch corrects the ioread32()-based version to do so.
>
> This is a bug but would only manifest on big endian systems
> that make use of the ioread32-based implementation of _enetc_rd_reg64.
> While all in-tree users of this driver are little endian and
> make use of the ioread64-based implementation of _enetc_rd_reg64.
> Thus, no in-tree user of this driver is affected by this bug.
>
> Flagged by Sparse.
> Compile tested only.
>
> Cc: Wei Fang <wei.fang@nxp.com>
> Fixes: 16eb4c85c964 ("enetc: Add ethtool statistics")
> Closes:
> https://lore.kern/
> el.org%2Fall%2FAM9PR04MB850500D3FC24FE23DEFCEA158879A%40AM9PR0
> 4MB8505.eurprd04.prod.outlook.com%2F&data=05%7C02%7Cwei.fang%40nxp
> .com%7Cefddfbd98e9747bd394d08ddb33d1e72%7C686ea1d3bc2b4c6fa92cd99
> c5c301635%7C0%7C0%7C638863797278832158%7CUnknown%7CTWFpbGZsb3
> d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIj
> oiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=4mt9vl78XAw%2BqX8
> w5zSo7xUA2aajicHGJnn6KpoNbXQ%3D&reserved=0
> Signed-off-by: Simon Horman <horms@kernel.org>
> ---
>  drivers/net/ethernet/freescale/enetc/enetc_hw.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> index 4098f01479bc..53e8d18c7a34 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
> @@ -507,7 +507,7 @@ static inline u64 _enetc_rd_reg64(void __iomem *reg)
>               tmp = ioread32(reg + 4);
>       } while (high != tmp);
>
> -     return le64_to_cpu((__le64)high << 32 | low);
> +     return (u64)high << 32 | low;
>  }
>  #endif
>

Many thanks.

Reviewed-by: Wei Fang <wei.fang@nxp.com>

Re: [PATCH net] net: enetc: Correct endianness handling in _enetc_rd_reg64

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 24 Jun 2025 17:35:12 +0100 you wrote:
> 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. However both the shift and logical or
> used to compose the return value would not work correctly on big endian
> systems if this were the case. Moreover, this is inconsistent with the
> first case where the return value of ioread64() is assumed to be in host
> byte order.
> 
> [...]

Here is the summary with links:
  - [net] net: enetc: Correct endianness handling in _enetc_rd_reg64
    https://git.kernel.org/netdev/net/c/7b515f35a911

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] net: enetc: Correct endianness handling in _enetc_rd_reg64

[Andy Shevchenko]

On Tue, Jun 24, 2025 at 05:35:12PM +0100, Simon Horman wrote:
> 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. However both the shift and logical or
> used to compose the return value would not work correctly on big endian
> systems if this were the case. Moreover, this is inconsistent with the
> first case where the return value of ioread64() is assumed to be in host
> byte order.
> 
> It appears that the correct approach is for both versions to treat the
> return value of ioread*() functions as being in host byte order. And
> this patch corrects the ioread32()-based version to do so.
> 
> This is a bug but would only manifest on big endian systems
> that make use of the ioread32-based implementation of _enetc_rd_reg64.
> While all in-tree users of this driver are little endian and
> make use of the ioread64-based implementation of _enetc_rd_reg64.
> Thus, no in-tree user of this driver is affected by this bug.

...

> @@ -507,7 +507,7 @@ static inline u64 _enetc_rd_reg64(void __iomem *reg)
>  		tmp = ioread32(reg + 4);
>  	} while (high != tmp);
>  
> -	return le64_to_cpu((__le64)high << 32 | low);
> +	return (u64)high << 32 | low;
>  }

Description and the visible context rings a bell like this is probably a
reimplementation of ioread64_lo_hi().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net] net: enetc: Correct endianness handling in _enetc_rd_reg64

[Andy Shevchenko]

On Wed, Sep 03, 2025 at 10:21:03PM +0300, Andy Shevchenko wrote:
> On Tue, Jun 24, 2025 at 05:35:12PM +0100, Simon Horman wrote:
> > 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. However both the shift and logical or
> > used to compose the return value would not work correctly on big endian
> > systems if this were the case. Moreover, this is inconsistent with the
> > first case where the return value of ioread64() is assumed to be in host
> > byte order.
> > 
> > It appears that the correct approach is for both versions to treat the
> > return value of ioread*() functions as being in host byte order. And
> > this patch corrects the ioread32()-based version to do so.
> > 
> > This is a bug but would only manifest on big endian systems
> > that make use of the ioread32-based implementation of _enetc_rd_reg64.
> > While all in-tree users of this driver are little endian and
> > make use of the ioread64-based implementation of _enetc_rd_reg64.
> > Thus, no in-tree user of this driver is affected by this bug.

...

> > @@ -507,7 +507,7 @@ static inline u64 _enetc_rd_reg64(void __iomem *reg)
> >  		tmp = ioread32(reg + 4);
> >  	} while (high != tmp);
> >  
> > -	return le64_to_cpu((__le64)high << 32 | low);
> > +	return (u64)high << 32 | low;
> >  }
> 
> Description and the visible context rings a bell like this is probably a
> reimplementation of ioread64_lo_hi().

And important to add, if the respective (-lo-hi.h) is included, ioread64()
automatically will be ioread64_lo_hi() and hence code can drop all these custom
calls, but again, I haven't looked into it for the details.

-- 
With Best Regards,
Andy Shevchenko