Thread (27 messages) 27 messages, 4 authors, 2025-09-25

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

From: "Arnd Bergmann" <arnd@arndb.de>
Date: 2025-09-24 14:06:00
Also in: linux-arch, linux-hardening, linux-i3c, lkml

On Wed, Sep 24, 2025, at 14:22, Guntupalli, Manikanta wrote:
quoted
@@ -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? 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.

     Arnd
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help