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

[Guntupalli, Manikanta]

[Public]

Hi,

> -----Original Message-----
> From: Arnd Bergmann <arnd@xxxxxxxx>
> Sent: Wednesday, September 24, 2025 12:21 AM
> To: Guntupalli, Manikanta <manikanta.guntupalli@xxxxxxx>; git (AMD-Xilinx)
> <git@xxxxxxx>; Simek, Michal <michal.simek@xxxxxxx>; Alexandre Belloni
> <alexandre.belloni@xxxxxxxxxxx>; Frank Li <Frank.Li@xxxxxxx>; Rob Herring
> <robh@xxxxxxxxxx>; krzk+dt@xxxxxxxxxx; Conor Dooley <conor+dt@xxxxxxxxxx>;
> Przemysław Gaj <pgaj@xxxxxxxxxxx>; Wolfram Sang <wsa+renesas@sang-
> engineering.com>; tommaso.merciai.xr@xxxxxxxxxxxxxx;
> quic_msavaliy@xxxxxxxxxxx; S-k, Shyam-sundar <Shyam-sundar.S-k@xxxxxxx>;
> Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>; 'billy_tsai@xxxxxxxxxxxxxx'
> <billy_tsai@xxxxxxxxxxxxxx>; Kees Cook <kees@xxxxxxxxxx>; Gustavo A. R. Silva
> <gustavoars@xxxxxxxxxx>; Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx>; Jorge
> Marques <jorge.marques@xxxxxxxxxx>; linux-i3c@xxxxxxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Linux-Arch <linux-
> arch@xxxxxxxxxxxxxxx>; linux-hardening@xxxxxxxxxxxxxxx
> Cc: Pandey, Radhey Shyam <radhey.shyam.pandey@xxxxxxx>; Goud, Srinivas
> <srinivas.goud@xxxxxxx>; Datta, Shubhrajyoti <shubhrajyoti.datta@xxxxxxx>;
> manion05gk@xxxxxxxxx
> 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.