The I3C abstraction expects u8 buffers, but some controllers operate with
a 32-bit bus width FIFO and cannot flag valid bytes individually. To avoid
reading or writing outside the buffer bounds, use 32-bit accesses where
possible and apply memcpy for any remaining bytes

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
 drivers/i3c/internals.h | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
index 433f6088b7cec8c77288ee24dbee8b18338aa1eb..6a11437fee47bd1d19354e983d4a561a4356b08d 100644
--- a/drivers/i3c/internals.h
+++ b/drivers/i3c/internals.h
@@ -22,4 +22,41 @@ int i3c_dev_enable_ibi_locked(struct i3c_dev_desc *dev);
 int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
 			       const struct i3c_ibi_setup *req);
 void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev);
+
+/**
+ * i3c_writel_fifo - Write data buffer to 32bit FIFO
+ * @addr: FIFO Address to write to
+ * @buf: Pointer to the data bytes to write
+ * @nbytes: Number of bytes to write
+ */
+static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
+				   int nbytes)
+{
+	writesl(addr, buf, nbytes / 4);
+	if (nbytes & 3) {
+		u32 tmp = 0;
+
+		memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
+		writel(tmp, addr);
+	}
+}
+
+/**
+ * i3c_readl_fifo - Read data buffer from 32bit FIFO
+ * @addr: FIFO Address to read from
+ * @buf: Pointer to the buffer to store read bytes
+ * @nbytes: Number of bytes to read
+ */
+static inline void i3c_readl_fifo(const void __iomem *addr, void *buf,
+				  int nbytes)
+{
+	readsl(addr, buf, nbytes / 4);
+	if (nbytes & 3) {
+		u32 tmp;
+
+		tmp = readl(addr);
+		memcpy(buf + (nbytes & ~3), &tmp, nbytes & 3);
+	}
+}
+
 #endif /* I3C_INTERNAL_H */

-- 
2.49.0

On Tue, Jun 24, 2025 at 11:06:04AM +0200, Jorge Marques wrote:
> The I3C abstraction expects u8 buffers, but some controllers operate with
> a 32-bit bus width FIFO and cannot flag valid bytes individually. To avoid
> reading or writing outside the buffer bounds, use 32-bit accesses where
> possible and apply memcpy for any remaining bytes
> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>

As per [1]:
Suggested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

My concern is rejected, and the rest looks good to me, so:
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

It still works nicely with the Renesas driver:
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

[1] https://lore.kernel.org/r/20250611093934.4208-5-wsa+renesas@sang-engineering.com

On Tue, Jun 24, 2025 at 11:06:04AM +0200, Jorge Marques wrote:
> The I3C abstraction expects u8 buffers, but some controllers operate with
> a 32-bit bus width FIFO and cannot flag valid bytes individually. To avoid
> reading or writing outside the buffer bounds, use 32-bit accesses where
> possible and apply memcpy for any remaining bytes
>
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/i3c/internals.h | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> index 433f6088b7cec8c77288ee24dbee8b18338aa1eb..6a11437fee47bd1d19354e983d4a561a4356b08d 100644
> --- a/drivers/i3c/internals.h
> +++ b/drivers/i3c/internals.h
> @@ -22,4 +22,41 @@ int i3c_dev_enable_ibi_locked(struct i3c_dev_desc *dev);
>  int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
>  			       const struct i3c_ibi_setup *req);
>  void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev);
> +
> +/**
> + * i3c_writel_fifo - Write data buffer to 32bit FIFO
> + * @addr: FIFO Address to write to
> + * @buf: Pointer to the data bytes to write
> + * @nbytes: Number of bytes to write
> + */
> +static inline void i3c_writel_fifo(void __iomem *addr, const void *buf,
> +				   int nbytes)
> +{
> +	writesl(addr, buf, nbytes / 4);
> +	if (nbytes & 3) {
> +		u32 tmp = 0;
> +
> +		memcpy(&tmp, buf + (nbytes & ~3), nbytes & 3);
> +		writel(tmp, addr);
> +	}
> +}
> +
> +/**
> + * i3c_readl_fifo - Read data buffer from 32bit FIFO
> + * @addr: FIFO Address to read from
> + * @buf: Pointer to the buffer to store read bytes
> + * @nbytes: Number of bytes to read
> + */
> +static inline void i3c_readl_fifo(const void __iomem *addr, void *buf,
> +				  int nbytes)
> +{
> +	readsl(addr, buf, nbytes / 4);
> +	if (nbytes & 3) {
> +		u32 tmp;
> +
> +		tmp = readl(addr);
> +		memcpy(buf + (nbytes & ~3), &tmp, nbytes & 3);
> +	}
> +}
> +
>  #endif /* I3C_INTERNAL_H */
>
> --
> 2.49.0
>

On Tue, Jun 24, 2025 at 02:08:45PM -0400, Frank Li wrote:
> On Tue, Jun 24, 2025 at 11:06:04AM +0200, Jorge Marques wrote:
> > The I3C abstraction expects u8 buffers, but some controllers operate with
> > a 32-bit bus width FIFO and cannot flag valid bytes individually. To avoid
> > reading or writing outside the buffer bounds, use 32-bit accesses where
> > possible and apply memcpy for any remaining bytes
> >
> > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> 
> > ---
> >  drivers/i3c/internals.h | 37 +++++++++++++++++++++++++++++++++++++

Is there a reason we don't put this in 'include/linux/i3c/master.h'?
'internals.h' is used for the core only so far, and '#include
<../something.h>' also looks a bit like a layering violation.

On Tue, Jun 24, 2025 at 10:30:42PM +0200, Wolfram Sang wrote:
> On Tue, Jun 24, 2025 at 02:08:45PM -0400, Frank Li wrote:
> > On Tue, Jun 24, 2025 at 11:06:04AM +0200, Jorge Marques wrote:
> > > The I3C abstraction expects u8 buffers, but some controllers operate with
> > > a 32-bit bus width FIFO and cannot flag valid bytes individually. To avoid
> > > reading or writing outside the buffer bounds, use 32-bit accesses where
> > > possible and apply memcpy for any remaining bytes
> > >
> > > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> >
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> >
> > > ---
> > >  drivers/i3c/internals.h | 37 +++++++++++++++++++++++++++++++++++++
>
> Is there a reason we don't put this in 'include/linux/i3c/master.h'?

master.h can be accessed in whole kernel tree. The scope is too big for these
helper functions, which should only limited to i3c drivers.

> 'internals.h' is used for the core only so far, and '#include
> <../something.h>' also looks a bit like a layering violation.

Not yet, you can look driver/pci/controller/*,  which include ../pci.h.

Frank
>

> > Is there a reason we don't put this in 'include/linux/i3c/master.h'?
> 
> master.h can be accessed in whole kernel tree. The scope is too big for these
> helper functions, which should only limited to i3c drivers.

Yet, who is going to include 'linux/i3c/master.h' outside of an I3C
controller driver?

> 
> > 'internals.h' is used for the core only so far, and '#include
> > <../something.h>' also looks a bit like a layering violation.
> 
> Not yet, you can look driver/pci/controller/*,  which include ../pci.h.

I never denied that it already exists in the kernel. But it still looks
fragile to me.

Whatever, it's not worth a big discussion. We can move this code if
there ever comes a reason to do so.