* Re: [PATCH] i3c: fix big-endian FIFO transfers
2025-09-24 15:02 [PATCH] i3c: fix big-endian FIFO transfers Arnd Bergmann
@ 2025-09-24 16:09 ` Frank Li
2025-09-24 16:35 ` Nuno Sá
2025-09-24 17:01 ` Jorge Marques
2 siblings, 0 replies; 5+ messages in thread
From: Frank Li @ 2025-09-24 16:09 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Alexandre Belloni, Wolfram Sang, Jorge Marques, Arnd Bergmann,
Manikanta Guntupalli, linux-i3c, linux-kernel
On Wed, Sep 24, 2025 at 05:02:53PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> Short MMIO transfers that are not a multiple of four bytes in size need
> a special case for the final bytes, however the existing implementation
> is not endian-safe and introduces an incorrect byteswap on big-endian
> kernels.
>
> This usually does not cause problems because most systems are
> little-endian and most transfers are multiple of four bytes long, but
> still needs to be fixed to avoid the extra byteswap.
>
> Change the special case for both i3c_writel_fifo() and i3c_readl_fifo()
> to use non-byteswapping writesl() and readsl() with a single element
> instead of the byteswapping writel()/readl() that are meant for individual
> MMIO registers.
>
> The earlier versions in the dw-i3c and i3c-master-cdns had a correct
> implementation, but the generic version that was recently added broke it.
>
> Fixes: 733b439375b4 ("i3c: master: Add inline i3c_readl_fifo() and i3c_writel_fifo()")
> Cc: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> This was a recent regression, the version in 6.16 still works,
> but 6.17-rc is broken.
> ---
> drivers/i3c/internals.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> index 0d857cc68cc5..0f8a25cb71e7 100644
> --- a/drivers/i3c/internals.h
> +++ b/drivers/i3c/internals.h
> @@ -38,7 +38,7 @@ static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
> u32 tmp = 0;
>
> memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
> - writel(tmp, addr);
> + writesl(addr, &buf, 1);
Can you add comments here. According to function name writesl()\writel(),
it is hard to realize writesl don't swap.
Actually, write to FIFO according to byte sequency in memory, regardless
cpu endian.
Frank
> }
> }
>
> @@ -55,7 +55,7 @@ static inline void i3c_readl_fifo(const void __iomem *addr, void *buf,
> if (nbytes & 3) {
> u32 tmp;
>
> - tmp = readl(addr);
> + readsl(addr, &tmp, 1);
> memcpy(buf + (nbytes & ~3), &tmp, nbytes & 3);
> }
> }
> --
> 2.39.5
>
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] i3c: fix big-endian FIFO transfers
2025-09-24 15:02 [PATCH] i3c: fix big-endian FIFO transfers Arnd Bergmann
2025-09-24 16:09 ` Frank Li
@ 2025-09-24 16:35 ` Nuno Sá
2025-09-24 17:01 ` Jorge Marques
2 siblings, 0 replies; 5+ messages in thread
From: Nuno Sá @ 2025-09-24 16:35 UTC (permalink / raw)
To: Arnd Bergmann, Alexandre Belloni, Wolfram Sang, Jorge Marques,
Frank Li
Cc: Arnd Bergmann, Manikanta Guntupalli, linux-i3c, linux-kernel
On Wed, 2025-09-24 at 17:02 +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> Short MMIO transfers that are not a multiple of four bytes in size need
> a special case for the final bytes, however the existing implementation
> is not endian-safe and introduces an incorrect byteswap on big-endian
> kernels.
>
> This usually does not cause problems because most systems are
> little-endian and most transfers are multiple of four bytes long, but
> still needs to be fixed to avoid the extra byteswap.
>
> Change the special case for both i3c_writel_fifo() and i3c_readl_fifo()
> to use non-byteswapping writesl() and readsl() with a single element
> instead of the byteswapping writel()/readl() that are meant for individual
> MMIO registers.
>
> The earlier versions in the dw-i3c and i3c-master-cdns had a correct
> implementation, but the generic version that was recently added broke it.
>
> Fixes: 733b439375b4 ("i3c: master: Add inline i3c_readl_fifo() and
> i3c_writel_fifo()")
> Cc: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> This was a recent regression, the version in 6.16 still works,
> but 6.17-rc is broken.
> ---
> drivers/i3c/internals.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> index 0d857cc68cc5..0f8a25cb71e7 100644
> --- a/drivers/i3c/internals.h
> +++ b/drivers/i3c/internals.h
> @@ -38,7 +38,7 @@ static inline void i3c_writel_fifo(void __iomem *addr, const
> void *buf,
> u32 tmp = 0;
>
> memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
> - writel(tmp, addr);
> + writesl(addr, &buf, 1);
Didn't you meant writesl(addr, &tmp, 1)?
- Nuno Sá
> }
> }
>
> @@ -55,7 +55,7 @@ static inline void i3c_readl_fifo(const void __iomem *addr,
> void *buf,
> if (nbytes & 3) {
> u32 tmp;
>
> - tmp = readl(addr);
> + readsl(addr, &tmp, 1);
> memcpy(buf + (nbytes & ~3), &tmp, nbytes & 3);
> }
> }
> --
> 2.39.5
>
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] i3c: fix big-endian FIFO transfers
2025-09-24 15:02 [PATCH] i3c: fix big-endian FIFO transfers Arnd Bergmann
2025-09-24 16:09 ` Frank Li
2025-09-24 16:35 ` Nuno Sá
@ 2025-09-24 17:01 ` Jorge Marques
2025-09-24 20:19 ` Arnd Bergmann
2 siblings, 1 reply; 5+ messages in thread
From: Jorge Marques @ 2025-09-24 17:01 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Alexandre Belloni, Wolfram Sang, Jorge Marques, Frank Li,
Arnd Bergmann, Manikanta Guntupalli, linux-i3c, linux-kernel
On Wed, Sep 24, 2025 at 05:02:53PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> Short MMIO transfers that are not a multiple of four bytes in size need
> a special case for the final bytes, however the existing implementation
> is not endian-safe and introduces an incorrect byteswap on big-endian
> kernels.
>
> This usually does not cause problems because most systems are
> little-endian and most transfers are multiple of four bytes long, but
> still needs to be fixed to avoid the extra byteswap.
>
> Change the special case for both i3c_writel_fifo() and i3c_readl_fifo()
> to use non-byteswapping writesl() and readsl() with a single element
> instead of the byteswapping writel()/readl() that are meant for individual
> MMIO registers.
>
> The earlier versions in the dw-i3c and i3c-master-cdns had a correct
> implementation, but the generic version that was recently added broke it.
>
> Fixes: 733b439375b4 ("i3c: master: Add inline i3c_readl_fifo() and i3c_writel_fifo()")
> Cc: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> This was a recent regression, the version in 6.16 still works,
> but 6.17-rc is broken.
> ---
> drivers/i3c/internals.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> index 0d857cc68cc5..0f8a25cb71e7 100644
> --- a/drivers/i3c/internals.h
> +++ b/drivers/i3c/internals.h
> @@ -38,7 +38,7 @@ static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
> u32 tmp = 0;
>
> memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
> - writel(tmp, addr);
> + writesl(addr, &buf, 1);
Hi Arnd, thanks for catching this,
Indeed, writel and readl are byte-swapping and the patch introduced a
bug. At include/asm-generic/io.h:
__raw_writel((u32 __force)__cpu_to_le32(value), addr);
...
val = __le32_to_cpu((__le32 __force)__raw_readl(addr));
While the writesl/readsl use the __raw_* methods without the byte
swamping.
Can you fix, as Nuno pointed, to:
writesl(addr, &tmp, 1);
as in the original drivers, and adding this information to the cover for
further clarification.
Thanks,
Jorge
> }
> }
>
> @@ -55,7 +55,7 @@ static inline void i3c_readl_fifo(const void __iomem *addr, void *buf,
> if (nbytes & 3) {
> u32 tmp;
>
> - tmp = readl(addr);
> + readsl(addr, &tmp, 1);
> memcpy(buf + (nbytes & ~3), &tmp, nbytes & 3);
> }
> }
> --
> 2.39.5
>
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] i3c: fix big-endian FIFO transfers
2025-09-24 17:01 ` Jorge Marques
@ 2025-09-24 20:19 ` Arnd Bergmann
0 siblings, 0 replies; 5+ messages in thread
From: Arnd Bergmann @ 2025-09-24 20:19 UTC (permalink / raw)
To: Jorge Marques, Arnd Bergmann
Cc: Alexandre Belloni, Wolfram Sang, Jorge Marques, Frank Li,
Manikanta Guntupalli, linux-i3c@lists.infradead.org, linux-kernel
On Wed, Sep 24, 2025, at 19:01, Jorge Marques wrote:
> On Wed, Sep 24, 2025 at 05:02:53PM +0200, Arnd Bergmann wrote:
> Can you fix, as Nuno pointed, to:
>
> writesl(addr, &tmp, 1);
>
> as in the original drivers, and adding this information to the cover for
> further clarification.
Sent v2 now with those two changes.
Arnd
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 5+ messages in thread