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

Guntupalli, Manikanta
2025-09-24T09:01:00Z

[Public]

Hi,

> -----Original Message-----
> From: Arnd Bergmann <arnd@arndb.de>
> Sent: Wednesday, September 24, 2025 12:21 AM
> 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 Tue, Sep 23, 2025, at 17:45, Manikanta Guntupalli wrote:
> > /**
> > * i3c_writel_fifo - Write data buffer to 32bit FIFO
> > * @addr: FIFO Address to write to
> > * @buf: Pointer to the data bytes to write
> > * @nbytes: Number of bytes to write
> > + * @endian: Endianness of FIFO write
> > */
> > static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
> > - int nbytes)
> > + int nbytes, enum i3c_fifo_endian endian)
> > {
> > - writesl(addr, buf, nbytes / 4);
> > + if (endian)
> > + writesl_be(addr, buf, nbytes / 4);
> > + else
> > + writesl(addr, buf, nbytes / 4);
> > +
>
> This seems counter-intuitive: a FIFO doesn't really have an endianness, it is instead
> used to transfer a stream of bytes, so if the device has a fixed endianess, the FIFO
> still needs to be read using a plain writesl().
>
> I see that your writesl_be() has an incorrect definition, which would lead to the
> i3c_writel_fifo() function accidentally still working if both the device and CPU use
> big-endian registers:
>
> static inline void writesl_be(volatile void __iomem *addr,
> const void *buffer,
> unsigned int count)
> {
> if (count) {
> const u32 *buf = buffer;
> do {
> __raw_writel((u32 __force)__cpu_to_be32(*buf), addr);
> buf++;
> } while (--count);
> }
> }
>
> The __cpu_to_be32() call that you add here means that the FIFO data is swapped
> on little-endian CPUs but not swapped on big-endian ones. Compare this to the
> normal writesl() function that never swaps because it writes a byte stream.
The use of __cpu_to_be32() in writesl_be() is intentional. The goal here is to ensure that the FIFO is always accessed in big-endian format, regardless of whether the CPU is little-endian or big-endian. On little-endian CPUs, this introduces the required byte swap before writing; on big-endian CPUs, the data is already in big-endian order, so no additional swap occurs.
This guarantees that the FIFO sees consistent big-endian data, matching the hardware's expectation.

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

Thanks,
Manikanta.