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 <a...@arndb.de>
> Sent: Wednesday, September 24, 2025 7:35 PM
> To: Guntupalli, Manikanta <manikanta.guntupa...@amd.com>; git (AMD-Xilinx)
> <g...@amd.com>; Simek, Michal <michal.si...@amd.com>; Alexandre Belloni
> <alexandre.bell...@bootlin.com>; Frank Li <frank...@nxp.com>; Rob Herring
> <r...@kernel.org>; krzk...@kernel.org; Conor Dooley <conor...@kernel.org>;
> Przemysław Gaj <p...@cadence.com>; Wolfram Sang <wsa+renesas@sang-
> engineering.com>; tommaso.merciai...@bp.renesas.com;
> quic_msava...@quicinc.com; S-k, Shyam-sundar <shyam-sundar....@amd.com>;
> Sakari Ailus <sakari.ai...@linux.intel.com>; 'billy_t...@aspeedtech.com'
> <billy_t...@aspeedtech.com>; Kees Cook <k...@kernel.org>; Gustavo A. R. Silva
> <gustavo...@kernel.org>; Jarkko Nikula <jarkko.nik...@linux.intel.com>; Jorge
> Marques <jorge.marq...@analog.com>; linux-...@lists.infradead.org;
> devicet...@vger.kernel.org; linux-ker...@vger.kernel.org; Linux-Arch <linux-
> a...@vger.kernel.org>; linux-hardening@vger.kernel.org
> Cc: Pandey, Radhey Shyam <radhey.shyam.pan...@amd.com>; Goud, Srinivas
> <srinivas.g...@amd.com>; Datta, Shubhrajyoti <shubhrajyoti.da...@amd.com>;
> manion0...@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 14:22, Guntupalli, Manikanta wrote:
>
> >> @@ -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).
>
> Did you test the little-endian system with the 'endian' flag set to
> I3C_FIFO_BIG_ENDIAN though?
Yes.

Your v7 code will still work on little-endian kernels
> if that flag is set to I3C_FIFO_LITTLE_ENDIAN, and it will also work on 
> big-endian
> kernels if the flag is set to I3C_FIFO_BIG_ENDIAN. But is broken for the 
> other two:
>
> - on little-endian kernels with I3C_FIFO_BIG_ENDIAN, the entire
>   data buffer is byteswapped in 32-bit chunks
>
> - on big-endian kernels with I3C_FIFO_LITTLE_ENDIAN, you run into
>   the existing bug of the swapped tail word.
>
> > 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.
>
> This version should fix the cases where you have a big-endian kernel with 
> either
> I3C_FIFO_BIG_ENDIAN or I3C_FIFO_LITTLE_ENDIAN, as neither combination
> does any byte swaps.
>
> However I'm fairly sure it's still broken for little-endian kernels when a 
> driver asks for
> a I3C_FIFO_BIG_ENDIAN conversion, same as v7.
We tested using the I3C_FIFO_BIG_ENDIAN flag from the driver on little-endian 
kernels, and it works as expected.

Thanks,
Manikanta.