linux-kernel - Re: [PATCH V7 3/4] i3c: master: Add endianness support for i3c_readl_fifo() and i3c_writel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <134c3a96-4023-47ab-8aa9-fd6ab75e5654@app.fastmail.com>
Date: Wed, 24 Sep 2025 16:05:26 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Manikanta Guntupalli" <manikanta.guntupalli@....com>,
 "git (AMD-Xilinx)" <git@....com>, "Michal Simek" <michal.simek@....com>,
 "Alexandre Belloni" <alexandre.belloni@...tlin.com>,
 "Frank Li" <Frank.Li@....com>, "Rob Herring" <robh@...nel.org>,
 "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
 "Conor Dooley" <conor+dt@...nel.org>,
 Przemysław Gaj <pgaj@...ence.com>,
 "Wolfram Sang" <wsa+renesas@...g-engineering.com>,
 "tommaso.merciai.xr@...renesas.com" <tommaso.merciai.xr@...renesas.com>,
 "quic_msavaliy@...cinc.com" <quic_msavaliy@...cinc.com>, "S-k,
 Shyam-sundar" <Shyam-sundar.S-k@....com>,
 "Sakari Ailus" <sakari.ailus@...ux.intel.com>,
 "'billy_tsai@...eedtech.com'" <billy_tsai@...eedtech.com>,
 "Kees Cook" <kees@...nel.org>,
 "Gustavo A. R. Silva" <gustavoars@...nel.org>,
 "Jarkko Nikula" <jarkko.nikula@...ux.intel.com>,
 "Jorge Marques" <jorge.marques@...log.com>,
 "linux-i3c@...ts.infradead.org" <linux-i3c@...ts.infradead.org>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 Linux-Arch <linux-arch@...r.kernel.org>,
 "linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org>
Cc: "Pandey, Radhey Shyam" <radhey.shyam.pandey@....com>,
 "Goud, Srinivas" <srinivas.goud@....com>,
 "Datta, Shubhrajyoti" <shubhrajyoti.datta@....com>,
 "manion05gk@...il.com" <manion05gk@...il.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? 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