RE: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel_fifo()

Guntupalli, Manikanta
2025-09-24T12:22:54Z

[Public]

Hi,

> -----Original Message-----
> From: Arnd Bergmann <arnd@arndb.de>
> Sent: Wednesday, September 24, 2025 3:30 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>; git (AMD-Xilinx)
> <git@amd.com>; Simek, Michal <michal.simek@amd.com>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; Frank Li <Frank.Li@nxp.com>; Rob Herring
> <robh@kernel.org>; krzk+dt@kernel.org; Conor Dooley <conor+dt@kernel.org>;
> Przemysław Gaj <pgaj@cadence.com>; Wolfram Sang <wsa+renesas@sang-
> engineering.com>; tommaso.merciai.xr@bp.renesas.com;
> quic_msavaliy@quicinc.com; S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>;
> Sakari Ailus <sakari.ailus@linux.intel.com>; 'billy_tsai@aspeedtech.com'
> <billy_tsai@aspeedtech.com>; Kees Cook <kees@kernel.org>; Gustavo A. R. Silva
> <gustavoars@kernel.org>; Jarkko Nikula <jarkko.nikula@linux.intel.com>; Jorge
> Marques <jorge.marques@analog.com>; linux-i3c@lists.infradead.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Linux-Arch <linux-
> arch@vger.kernel.org>; linux-hardening@vger.kernel.org
> Cc: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; Goud, Srinivas
> <srinivas.goud@amd.com>; Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>;
> manion05gk@gmail.com
> Subject: Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo()
> and i3c_writel_fifo()
>
> On Wed, Sep 24, 2025, at 11:00, Guntupalli, Manikanta wrote:
> >> > if (nbytes & 3) {
> >> > u32 tmp = 0;
> >> >
> >> > memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
> >> > - writel(tmp, addr);
> >> > +
> >> > + if (endian)
> >> > + writel_be(tmp, addr);
> >> > + else
> >> > + writel(tmp, addr);
> >>
> >> This bit however seems to fix a bug, but does so in a confusing way.
> >> The way the FIFO registers usually deal with excess bytes is to put
> >> them into the first bytes of the FIFO register, so this should just
> >> be a
> >>
> >> writesl(addr, &tmp, 1);
> >>
> >> to write one set of four bytes into the FIFO without endian-swapping.
> >>
> >> Could it be that you are just trying to use a normal i3c adapter with
> >> little-endian registers on a normal big-endian machine but ran into this bug?
> > The intention here is to enforce big-endian ordering for the trailing
> > bytes as well. By using __cpu_to_be32() for full words and writel_be()
> > for the leftover word, the FIFO is always accessed in big-endian
> > format, regardless of the CPU's native endianness. This ensures
> > consistent and correct data ordering from the device's perspective.
>
> No, this makes no sense: The 'else' portion is incorrect in the function, and prevents
> it from working on all big-endian CPUs because 'writel()' only works for little-endian
> 32-bit registers. If you just fix the bug as I described above, this will work correctly on
> any driver calling the current function. At that point, your hack becomes a nop for big-
> endian systems, while calling the function with 'endian = true' on little-endian kernels
> is still wrong.
>
> Please start by fixing the existing bug and test that again.
>
> I know that endianess with FIFO registers is hard to understand, and everyone has
> to spend the time once to actually wrap their head around this. Even if you don't
> believe me, please try the bugfix below (without your added argument) and think
> about how FIFO registers that transfer byte streams are different of fixed-endian 32-
> bit registers. Note that your driver uses little-endian readl() for normal registers, and
> this is portable to both LE and BE kernels.
>
> See also the explanation in 41739ee355ab ("asm-generic: io:
> don't perform swab during {in,out} string functions").
>
> Arnd
>
> 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);
> }
> }
>
> @@ -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);
> }
> }

We have not observed any issue on little-endian systems in our testing so far (as I mentioned earlier in asm-generic/io.h: Add big-endian MMIO accessors).

That said, I understand your point about FIFO semantics being different from fixed-endian registers. To cover both cases, we considered using writesl() for little-endian and introducing a writesl_be() helper for big-endian, as shown below:

static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
int nbytes, enum i3c_fifo_endian endian)
{
if (endian)
writesl_be(addr, buf, nbytes / 4);
else
writesl(addr, buf, nbytes / 4);

if (nbytes & 3) {
u32 tmp = 0;

memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);

if (endian)
writesl_be(addr, &tmp, 1);
else
writesl(addr, &tmp, 1);
}
}

With this approach, both little-endian and big-endian cases works as expected.

Thanks,
Manikanta.