| 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
| ||
|
Message-ID:
<DM4PR12MB61093CEE50990ECC403D3FB98C1FA@DM4PR12MB6109.namprd12.prod.outlook.com>
Date: Thu, 25 Sep 2025 08:47:47 +0000
From: "Guntupalli, Manikanta" <manikanta.guntupalli@....com>
To: Nuno Sá <noname.nuno@...il.com>, Jorge Marques
<gastmaier@...il.com>, Arnd Bergmann <arnd@...nel.org>
CC: Alexandre Belloni <alexandre.belloni@...tlin.com>, Jorge Marques
<jorge.marques@...log.com>, Wolfram Sang <wsa+renesas@...g-engineering.com>,
Frank Li <Frank.Li@....com>, Arnd Bergmann <arnd@...db.de>,
"linux-i3c@...ts.infradead.org" <linux-i3c@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "git
(AMD-Xilinx)" <git@....com>, "Simek, Michal" <michal.simek@....com>
Subject: RE: [PATCH] [v2] i3c: fix big-endian FIFO transfers
[Public]
Hi,
> -----Original Message-----
> From: Nuno Sá <noname.nuno@...il.com>
> Sent: Thursday, September 25, 2025 1:22 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli@....com>; Jorge Marques
> <gastmaier@...il.com>; Arnd Bergmann <arnd@...nel.org>
> Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>; Jorge Marques
> <jorge.marques@...log.com>; Wolfram Sang <wsa+renesas@...g-
> engineering.com>; Frank Li <Frank.Li@....com>; Arnd Bergmann
> <arnd@...db.de>; linux-i3c@...ts.infradead.org; linux-kernel@...r.kernel.org; git
> (AMD-Xilinx) <git@....com>; Simek, Michal <michal.simek@....com>
> Subject: Re: [PATCH] [v2] i3c: fix big-endian FIFO transfers
>
> On Thu, 2025-09-25 at 07:37 +0000, Guntupalli, Manikanta wrote:
> > [Public]
> >
> > Hi,
> >
> > > -----Original Message-----
> > > From: Jorge Marques <gastmaier@...il.com>
> > > Sent: Thursday, September 25, 2025 12:47 PM
> > > To: Arnd Bergmann <arnd@...nel.org>
> > > Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>; Jorge Marques
> > > <jorge.marques@...log.com>; Wolfram Sang <wsa+renesas@...g-
> > > engineering.com>; Frank Li <Frank.Li@....com>; Arnd Bergmann
> > > <arnd@...db.de>; Guntupalli, Manikanta
> > > <manikanta.guntupalli@....com>;
> > > linux-
> > > i3c@...ts.infradead.org; linux-kernel@...r.kernel.org
> > > Subject: Re: [PATCH] [v2] i3c: fix big-endian FIFO transfers
> > >
> > > On Wed, Sep 24, 2025 at 10:18:33PM +0200, Arnd Bergmann wrote:
> > > > From: Arnd Bergmann <arnd@...db.de>
> > > >
> > > > Short MMIO transfers that are not a multiple of four bytes in size
> > > > need a special case for the final bytes, however the existing
> > > > implementation is not endian-safe and introduces an incorrect
> > > > byteswap on big-endian kernels.
> > > >
> > > > This usually does not cause problems because most systems are
> > > > little-endian and most transfers are multiple of four bytes long,
> > > > but still needs to be fixed to avoid the extra byteswap.
> > > >
> > > > Change the special case for both i3c_writel_fifo() and
> > > > i3c_readl_fifo() to use non-byteswapping writesl() and readsl()
> > > > with a single element instead of the byteswapping writel()/readl()
> > > > that are meant for individual MMIO registers. As data is copied
> > > > between a FIFO and a memory buffer, the writesl()/readsl() loops
> > > > are typically based on __raw_readl()/ __raw_writel(), resulting in
> > > > the order of bytes in the FIFO to match the order in the buffer,
> > > > regardless of the CPU endianess.
> > > >
> > > > The earlier versions in the dw-i3c and i3c-master-cdns had a
> > > > correct implementation, but the generic version that was recently added broke
> it.
> > > >
> > > > Fixes: 733b439375b4 ("i3c: master: Add inline i3c_readl_fifo() and
> > > > i3c_writel_fifo()")
> > > > Cc: Manikanta Guntupalli <manikanta.guntupalli@....com>
> > > > Signed-off-by: Arnd Bergmann <arnd@...db.de>
> > > > ---
> > > > This was a recent regression, the version in 6.16 still works, but
> > > > 6.17-rc is broken.
> > > >
> > > > v2 changes:
> > > > - add code comments
> > > > - write correct data buffer
> > > > ---
> > > > drivers/i3c/internals.h | 12 ++++++++++--
> > > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> > > > index 0d857cc68cc5..79ceaa5f5afd 100644
> > > > --- a/drivers/i3c/internals.h
> > > > +++ b/drivers/i3c/internals.h
> > > > @@ -38,7 +38,11 @@ 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() instead of writel() to keep FIFO
> > > > + * byteorder on big-endian targets
> > > > + */
> > > > + writesl(addr, &tmp, 1);
> > > > }
> > > > }
> > > >
> > > > @@ -55,7 +59,11 @@ static inline void i3c_readl_fifo(const void
> > > > __iomem *addr,
> > > void *buf,
> > > > if (nbytes & 3) {
> > > > u32 tmp;
> > > >
> > > > - tmp = readl(addr);
> > > > + /*
> > > > + * readsl() instead of readl() to keep FIFO
> > > > + * byteorder on big-endian targets
> > > > + */
> > > > + readsl(addr, &tmp, 1);
> > > > memcpy(buf + (nbytes & ~3), &tmp, nbytes & 3);
> > > > }
> > > > }
> > > Reviewed-by: Jorge Marques <jorge.marques@...log.com>
> > > > --
> > > > 2.39.5
> > > >
> >
> > This patch fixes the sub-word transfer case on big-endian kernels, but
> > it still does not address the scenario of little-endian kernels
> > accessing big- endian FIFOs.
> >
>
> I would argue that's something for callers of these functions to care about.
If each I3C driver has to handle FIFO endianness individually, it introduces unnecessary duplication and overhead across drivers. Centralizing this in the FIFO access helpers keeps the logic consistent, avoids repeated boilerplate, and reduces the chance of subtle bugs.
>
> > With the current version, i3c_writel_fifo() and i3c_readl_fifo() only
> > work when the FIFO has the same endianness as the CPU. On platforms
> > such as the
> > ZCU102 (Zynq UltraScale+ MPSoC, Cortex-A53, little-endian), the I3C
> > FIFOs are big-endian, and this patch alone is not sufficient -
> > transfers fail in that configuration.
> >
> > We have validated this on ZCU102, and the mismatch between LE kernel
> > and BE FIFO is still an issue.
> >
> > On top of this fix, explicit FIFO endianness support is required, as
> > proposed in [PATCH v7 3/4] "i3c: master: Add endianness support for
> > i3c_readl_fifo() and i3c_writel_fifo()". That approach adds an endian
> > argument and uses
> > writesl_be()/readsl_be() where necessary, e.g.:
> >
> > 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);
> > }
> > }
> >
> >
Thanks,
Manikanta.
Powered by blists - more mailing lists