[PATCH v2 0/3] i3c: account for controller transfer limits

[PATCH v2 0/3] i3c: account for controller transfer limits

[Shubham Patil]

Targets advertise MRL and MWL, but these values do not account for
smaller controller transmit and receive buffers. This can cause client
drivers such as mctp-i3c to request transfers that the controller must
reject.

Add controller transfer limits to the I3C core, publish the independent
DesignWare transmit and receive buffer depths, and make mctp-i3c use the
effective target/controller limits.

Changes in v2:
- Rework the solution to preserve the target-reported MRL and MWL instead
  of sending SETMRL/SETMWL and rewriting the GETMRL/GETMWL responses.
- Add I3C core fields and helper APIs that combine target and controller
  transfer limits.
- Track the DesignWare transmit and receive buffer depths separately.
- Add an mctp-i3c patch that uses the effective transfer limits.
- Drop the unrelated IBI payload-length clamping from this series.
- Split the original DesignWare-only patch into three patches.

Link to v1:
https://lore.kernel.org/all/20260908102724.3232660-1-shubhamsanjay.patil@amd.com/

Shubham Patil (3):
  i3c: Add helpers to get the maximum transfer size for a device
  i3c: dw: Track the transmit and receive buffer depths separately
  mctp i3c: Clamp transfer lengths to the controller limits

 drivers/i3c/device.c               | 49 ++++++++++++++++++++++++++++++
 drivers/i3c/master/dw-i3c-master.c | 22 ++++++++++----
 drivers/i3c/master/dw-i3c-master.h |  3 +-
 drivers/net/mctp/mctp-i3c.c        |  8 ++---
 include/linux/i3c/device.h         |  3 ++
 include/linux/i3c/master.h         | 13 ++++++--
 6 files changed, 84 insertions(+), 14 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] i3c: Add helpers to get the maximum transfer size for a device

[Shubham Patil]

A target's MRL and MWL, retrieved with GETMRL and GETMWL, describe only
what that target can accept. They say nothing about the controller
driving it, which may impose a smaller limit of its own: the DesignWare
controller, for example, rejects any private SDR transfer that does not
fit its data buffer. A device driver sizing a transfer from MRL alone
can therefore build one the controller has to refuse with -EOPNOTSUPP.

Add tx_fifo_bytes and rx_fifo_bytes to struct i3c_master_controller, to
be set by a controller driver before i3c_master_register() when it has
such a limit, together with two helpers returning the smaller of the
target and controller values for each direction.

Both inputs use 0 to mean "no limit reported", so the helpers combine
them with min_not_zero() and return 0 only when neither side reports
one, leaving each caller to apply its own default. struct
i3c_device_info keeps the raw target values untouched.

The returned value applies to a single message. A controller is free to
budget its buffer across every message in one i3c_device_do_xfers()
call, so a caller batching several messages in the same direction may
still be rejected.

Signed-off-by: Shubham Patil <shubhamsanjay.patil@amd.com>
---
 drivers/i3c/device.c       | 49 ++++++++++++++++++++++++++++++++++++++
 include/linux/i3c/device.h |  3 +++
 include/linux/i3c/master.h | 13 +++++++---
 3 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
index 4df4682af150..588f66b3af6d 100644
--- a/drivers/i3c/device.c
+++ b/drivers/i3c/device.c
@@ -9,6 +9,7 @@
 #include <linux/bug.h>
 #include <linux/completion.h>
 #include <linux/device.h>
+#include <linux/minmax.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
 
@@ -106,6 +107,54 @@ void i3c_device_get_info(const struct i3c_device *dev,
 }
 EXPORT_SYMBOL_GPL(i3c_device_get_info);
 
+/**
+ * i3c_device_get_max_read_xfer_bytes() - get the maximum readable message size
+ *
+ * @dev: device the transfers would be directed to
+ *
+ * Return: the smaller of @dev's MRL and the controller's receive limit for a
+ * single message, or 0 if neither reports a limit.
+ */
+u16 i3c_device_get_max_read_xfer_bytes(const struct i3c_device *dev)
+{
+	u16 mrl = 0, rx_fifo_bytes = 0;
+
+	i3c_bus_normaluse_lock(dev->bus);
+	if (dev->desc) {
+		mrl = dev->desc->info.max_read_len;
+		rx_fifo_bytes = i3c_dev_get_master(dev->desc)->rx_fifo_bytes;
+	}
+	i3c_bus_normaluse_unlock(dev->bus);
+
+	/* 0 means "no limit reported" on either side. */
+	return min_not_zero(mrl, rx_fifo_bytes);
+}
+EXPORT_SYMBOL_GPL(i3c_device_get_max_read_xfer_bytes);
+
+/**
+ * i3c_device_get_max_write_xfer_bytes() - get the maximum writable message size
+ *
+ * @dev: device the transfers would be directed to
+ *
+ * Return: the smaller of @dev's MWL and the controller's transmit limit for a
+ * single message, or 0 if neither reports a limit.
+ */
+u16 i3c_device_get_max_write_xfer_bytes(const struct i3c_device *dev)
+{
+	u16 mwl = 0, tx_fifo_bytes = 0;
+
+	i3c_bus_normaluse_lock(dev->bus);
+	if (dev->desc) {
+		mwl = dev->desc->info.max_write_len;
+		tx_fifo_bytes = i3c_dev_get_master(dev->desc)->tx_fifo_bytes;
+	}
+	i3c_bus_normaluse_unlock(dev->bus);
+
+	/* 0 means "no limit reported" on either side. */
+	return min_not_zero(mwl, tx_fifo_bytes);
+}
+EXPORT_SYMBOL_GPL(i3c_device_get_max_write_xfer_bytes);
+
 /**
  * i3c_device_disable_ibi() - Disable IBIs coming from a specific device
  * @dev: device on which IBIs should be disabled
diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h
index e30ea2cb13cb..cb3374e4f9fa 100644
--- a/include/linux/i3c/device.h
+++ b/include/linux/i3c/device.h
@@ -331,6 +331,9 @@ int i3c_device_do_setdasa(struct i3c_device *dev);
 
 void i3c_device_get_info(const struct i3c_device *dev, struct i3c_device_info *info);
 
+u16 i3c_device_get_max_read_xfer_bytes(const struct i3c_device *dev);
+u16 i3c_device_get_max_write_xfer_bytes(const struct i3c_device *dev);
+
 struct i3c_ibi_payload {
 	unsigned int len;
 	const void *data;
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index d58070a72a49..3084cc7a21be 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -551,11 +551,16 @@ struct i3c_master_controller_ops {
  *            context (notably the resume path), and is skipped if the
  *            controller is shutting down
  * @dev_nack_retry_count: retry count when slave device nack
+ * @tx_fifo_bytes: maximum number of bytes this controller can transmit in a
+ *		   single message, or 0 if it has no such limit
+ * @rx_fifo_bytes: maximum number of bytes this controller can receive in a
+ *		   single message, or 0 if it has no such limit
  *
  * A &struct i3c_master_controller has to be registered to the I3C subsystem
- * through i3c_master_register(). None of &struct i3c_master_controller fields
- * should be set manually, just pass appropriate values to
- * i3c_master_register().
+ * through i3c_master_register(). Except for @tx_fifo_bytes and
+ * @rx_fifo_bytes, which a controller driver may set before registering, none
+ * of &struct i3c_master_controller fields should be set manually, just pass
+ * appropriate values to i3c_master_register().
  */
 struct i3c_master_controller {
 	struct device dev;
@@ -580,6 +585,8 @@ struct i3c_master_controller {
 	struct work_struct hj_work;
 	struct work_struct reg_work;
 	unsigned int dev_nack_retry_count;
+	u16 tx_fifo_bytes;
+	u16 rx_fifo_bytes;
 };
 
 /**
-- 
2.34.1

[PATCH v2 2/3] i3c: dw: Track the transmit and receive buffer depths separately

[Shubham Patil]

The driver currently uses the transmit buffer depth to validate both
transmit and receive transfers. Since the transmit and receive buffers
can have different capacities, receive transfers must instead be
validated against the receive buffer depth.

QUEUE_SIZE_CAPABILITY reports the configured size of each buffer. Take
the receive depth from there, leave the transmit and command depths on
the status level registers they already use, and split
caps.datafifodepth into caps.txfifodepth and caps.rxfifodepth so that
each direction is checked against its own buffer.

That register was already defined at the right offset, but under a name
that does not appear in the databook and with no user at all. Rename
EXTENDED_CAPABILITY to QUEUE_SIZE_CAPABILITY and add the field
accessor.

Publish both depths in bytes through base.tx_fifo_bytes and
base.rx_fifo_bytes, so that device drivers can size a transfer this
controller will accept.

Signed-off-by: Shubham Patil <shubhamsanjay.patil@amd.com>
---
 drivers/i3c/master/dw-i3c-master.c | 22 ++++++++++++++++------
 drivers/i3c/master/dw-i3c-master.h |  3 ++-
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index d1119f203d1b..dd5462b12951 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -203,7 +203,10 @@
 #define BUS_IDLE_TIMING			0xd8
 #define I3C_VER_ID			0xe0
 #define I3C_VER_TYPE			0xe4
-#define EXTENDED_CAPABILITY		0xe8
+#define QUEUE_SIZE_CAPABILITY		0xe8
+#define QUEUE_SIZE_CAPABILITY_RX_BUF(x)	(((x) & GENMASK(7, 4)) >> 4)
+/* Sizes are encoded, 0 meaning 2 DWORDs and every further step doubling it. */
+#define QUEUE_SIZE_TO_DWORDS(x)		(2U << (x))
 #define SLAVE_CONFIG			0xec
 
 #define DYN_ADDR_LO_MASK GENMASK(4, 0)
@@ -978,8 +981,8 @@ static int dw_i3c_master_i3c_xfers(struct i3c_dev_desc *dev,
 			ntxwords += DIV_ROUND_UP(i3c_xfers[i].len, 4);
 	}
 
-	if (ntxwords > master->caps.datafifodepth ||
-	    nrxwords > master->caps.datafifodepth)
+	if (ntxwords > master->caps.txfifodepth ||
+	    nrxwords > master->caps.rxfifodepth)
 		return -EOPNOTSUPP;
 
 	struct dw_i3c_xfer *xfer __free(kfree) = dw_i3c_master_alloc_xfer(master, i3c_nxfers);
@@ -1136,8 +1139,8 @@ static int dw_i3c_master_i2c_xfers(struct i2c_dev_desc *dev,
 			ntxwords += DIV_ROUND_UP(i2c_xfers[i].len, 4);
 	}
 
-	if (ntxwords > master->caps.datafifodepth ||
-	    nrxwords > master->caps.datafifodepth)
+	if (ntxwords > master->caps.txfifodepth ||
+	    nrxwords > master->caps.rxfifodepth)
 		return -EOPNOTSUPP;
 
 	struct dw_i3c_xfer *xfer __free(kfree) = dw_i3c_master_alloc_xfer(master, i2c_nxfers);
@@ -1738,7 +1741,14 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
 	master->caps.cmdfifodepth = QUEUE_STATUS_LEVEL_CMD(ret);
 
 	ret = readl(master->regs + DATA_BUFFER_STATUS_LEVEL);
-	master->caps.datafifodepth = DATA_BUFFER_STATUS_LEVEL_TX(ret);
+	master->caps.txfifodepth = DATA_BUFFER_STATUS_LEVEL_TX(ret);
+
+	ret = readl(master->regs + QUEUE_SIZE_CAPABILITY);
+	master->caps.rxfifodepth =
+		QUEUE_SIZE_TO_DWORDS(QUEUE_SIZE_CAPABILITY_RX_BUF(ret));
+
+	master->base.tx_fifo_bytes = master->caps.txfifodepth * 4;
+	master->base.rx_fifo_bytes = master->caps.rxfifodepth * 4;
 
 	ret = readl(master->regs + DEVICE_ADDR_TABLE_POINTER);
 	master->datstartaddr = ret;
diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h
index f5b4491b247a..e23554bfc5e4 100644
--- a/drivers/i3c/master/dw-i3c-master.h
+++ b/drivers/i3c/master/dw-i3c-master.h
@@ -14,7 +14,8 @@
 
 struct dw_i3c_master_caps {
 	u8 cmdfifodepth;
-	u8 datafifodepth;
+	u8 txfifodepth;
+	u8 rxfifodepth;
 };
 
 struct dw_i3c_dat_entry {
-- 
2.34.1

Re: [PATCH v2 2/3] i3c: dw: Track the transmit and receive buffer depths separately

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] i3c: dw: u8 truncation of rxfifodepth in dw_i3c_common_probe()
--

commit c93cc6843b57993d053db03cc3f7cee25dbd4371
Author: Shubham Patil <shubhamsanjay.patil@amd.com>

i3c: dw: Track the transmit and receive buffer depths separately

This commit splits caps.datafifodepth into caps.txfifodepth and
caps.rxfifodepth, populates caps.rxfifodepth from the QUEUE_SIZE_CAPABILITY
register, and exposes the FIFO capacities via base.tx_fifo_bytes and
base.rx_fifo_bytes.

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index d1119f203d1b6..dd5462b129514 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -203,7 +203,10 @@
>  #define BUS_IDLE_TIMING			0xd8
>  #define I3C_VER_ID			0xe0
>  #define I3C_VER_TYPE			0xe4
> -#define EXTENDED_CAPABILITY		0xe8
> +#define QUEUE_SIZE_CAPABILITY		0xe8
> +#define QUEUE_SIZE_CAPABILITY_RX_BUF(x)	(((x) & GENMASK(7, 4)) >> 4)
> +/* Sizes are encoded, 0 meaning 2 DWORDs and every further step doubling it. */
> +#define QUEUE_SIZE_TO_DWORDS(x)		(2U << (x))
>  #define SLAVE_CONFIG			0xec
>  
>  #define DYN_ADDR_LO_MASK GENMASK(4, 0)
> @@ -978,8 +981,8 @@ static int dw_i3c_master_i3c_xfers(struct i3c_dev_desc *dev,
>  			ntxwords += DIV_ROUND_UP(i3c_xfers[i].len, 4);
>  	}
>  
> -	if (ntxwords > master->caps.datafifodepth ||
> -	    nrxwords > master->caps.datafifodepth)
> +	if (ntxwords > master->caps.txfifodepth ||
> +	    nrxwords > master->caps.rxfifodepth)
>  		return -EOPNOTSUPP;
>  
>  	struct dw_i3c_xfer *xfer __free(kfree) = dw_i3c_master_alloc_xfer(master, i3c_nxfers);
> @@ -1136,8 +1139,8 @@ static int dw_i3c_master_i2c_xfers(struct i2c_dev_desc *dev,
>  			ntxwords += DIV_ROUND_UP(i2c_xfers[i].len, 4);
>  	}
>  
> -	if (ntxwords > master->caps.datafifodepth ||
> -	    nrxwords > master->caps.datafifodepth)
> +	if (ntxwords > master->caps.txfifodepth ||
> +	    nrxwords > master->caps.rxfifodepth)
>  		return -EOPNOTSUPP;
>  
>  	struct dw_i3c_xfer *xfer __free(kfree) = dw_i3c_master_alloc_xfer(master, i2c_nxfers);
> @@ -1738,7 +1741,14 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
>  	master->caps.cmdfifodepth = QUEUE_STATUS_LEVEL_CMD(ret);
>  
>  	ret = readl(master->regs + DATA_BUFFER_STATUS_LEVEL);
> -	master->caps.datafifodepth = DATA_BUFFER_STATUS_LEVEL_TX(ret);
> +	master->caps.txfifodepth = DATA_BUFFER_STATUS_LEVEL_TX(ret);
> +
> +	ret = readl(master->regs + QUEUE_SIZE_CAPABILITY);
> +	master->caps.rxfifodepth =
> +		QUEUE_SIZE_TO_DWORDS(QUEUE_SIZE_CAPABILITY_RX_BUF(ret));
> +
> +	master->base.tx_fifo_bytes = master->caps.txfifodepth * 4;
> +	master->base.rx_fifo_bytes = master->caps.rxfifodepth * 4;
>  
>  	ret = readl(master->regs + DEVICE_ADDR_TABLE_POINTER);
>  	master->datstartaddr = ret;
> diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h
> index f5b4491b247a0..e23554bfc5e49 100644
> --- a/drivers/i3c/master/dw-i3c-master.h
> +++ b/drivers/i3c/master/dw-i3c-master.h
> @@ -14,7 +14,8 @@
>  
>  struct dw_i3c_master_caps {
>  	u8 cmdfifodepth;
> -	u8 datafifodepth;
> +	u8 txfifodepth;
> +	u8 rxfifodepth;

[Severity: High]
Can master->caps.rxfifodepth overflow u8 and truncate to 0 in
dw_i3c_common_probe()?

In the DesignWare I3C controller, QUEUE_SIZE_CAPABILITY_RX_BUF(ret) can
return up to 0x7 (256 32-bit words, or 1024 bytes). When that happens,
QUEUE_SIZE_TO_DWORDS(7) evaluates to 256 (0x100), which truncates to 0 when
stored into the u8 rxfifodepth field in struct dw_i3c_master_caps.

With master->caps.rxfifodepth set to 0, master->base.rx_fifo_bytes also
becomes 0 in dw_i3c_common_probe(), and the nrxwords >
master->caps.rxfifodepth check in both dw_i3c_master_i3c_xfers() and
dw_i3c_master_i2c_xfers() will reject every non-empty read transfer with
-EOPNOTSUPP.

Should rxfifodepth (and txfifodepth) in struct dw_i3c_master_caps be u16
instead of u8?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105085425.3418230-1-shubhamsanjay.patil@amd.com?part=2

[PATCH v2 3/3] mctp i3c: Clamp transfer lengths to the controller limits

[Shubham Patil]

mctp_i3c_setup() takes the maximum read and write lengths straight from
GETMRL and GETMWL. Those describe the endpoint alone, so on a controller
with a smaller limit every read can be built too large and rejected.
Transmit packets can similarly reach the controller even when they are
larger than its transmit buffer. The receive path also allocates an skb
for data the hardware can never deliver.

Use i3c_device_get_max_read_xfer_bytes() and
i3c_device_get_max_write_xfer_bytes() instead, which return the smaller
of the endpoint and controller limits.

Behaviour is unchanged when neither reports a limit, as the helpers
return 0 exactly as the GETMRL and GETMWL fields do today.

Signed-off-by: Shubham Patil <shubhamsanjay.patil@amd.com>
---
 drivers/net/mctp/mctp-i3c.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/mctp/mctp-i3c.c b/drivers/net/mctp/mctp-i3c.c
index 88d9e36cd4a2..6d08898e1d21 100644
--- a/drivers/net/mctp/mctp-i3c.c
+++ b/drivers/net/mctp/mctp-i3c.c
@@ -81,9 +81,9 @@ struct mctp_i3c_device {
 	bool have_mdb;
 	/* I3C dynamic address */
 	u8 addr;
-	/* Maximum read length */
+	/* Maximum read length, limited by the device MRL and the controller */
 	u16 mrl;
-	/* Maximum write length */
+	/* Maximum write length, limited by the device MWL and the controller */
 	u16 mwl;
 	/* Provisioned ID */
 	u64 pid;
@@ -214,8 +214,8 @@ static int mctp_i3c_setup(struct mctp_i3c_device *mi)
 	i3c_device_get_info(mi->i3c, &info);
 	mi->have_mdb = info.bcr & BIT(2);
 	mi->addr = info.dyn_addr;
-	mi->mwl = info.max_write_len;
-	mi->mrl = info.max_read_len;
+	mi->mwl = i3c_device_get_max_write_xfer_bytes(mi->i3c);
+	mi->mrl = i3c_device_get_max_read_xfer_bytes(mi->i3c);
 	mi->pid = info.pid;
 
 	rc = i3c_device_request_ibi(mi->i3c, &ibi);
-- 
2.34.1