[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 at arndb.de>
> Sent: Wednesday, September 24, 2025 3:30 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli at amd.com>; git (AMD-Xilinx)
> <git at amd.com>; Simek, Michal <michal.simek at amd.com>; Alexandre Belloni
> <alexandre.belloni at bootlin.com>; Frank Li <Frank.Li at nxp.com>; Rob Herring
> <robh at kernel.org>; krzk+dt at kernel.org; Conor Dooley <conor+dt at kernel.org>;
> Przemysław Gaj <pgaj at cadence.com>; Wolfram Sang <wsa+renesas at sang-
> engineering.com>; tommaso.merciai.xr at bp.renesas.com;
> quic_msavaliy at quicinc.com; S-k, Shyam-sundar <Shyam-sundar.S-k at amd.com>;
> Sakari Ailus <sakari.ailus at linux.intel.com>; 'billy_tsai at aspeedtech.com'
> <billy_tsai at aspeedtech.com>; Kees Cook <kees at kernel.org>; Gustavo A. R. Silva
> <gustavoars at kernel.org>; Jarkko Nikula <jarkko.nikula at linux.intel.com>; Jorge
> Marques <jorge.marques at analog.com>; linux-i3c at lists.infradead.org;
> devicetree at vger.kernel.org; linux-kernel at vger.kernel.org; Linux-Arch <linux-
> arch at vger.kernel.org>; linux-hardening at vger.kernel.org
> Cc: Pandey, Radhey Shyam <radhey.shyam.pandey at amd.com>; Goud, Srinivas
> <srinivas.goud at amd.com>; Datta, Shubhrajyoti <shubhrajyoti.datta at amd.com>;
> manion05gk at 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.