All of lore.kernel.org
 help / color / mirror / Atom feed
From: sashiko-bot@kernel.org
To: "Shubham Patil" <shubhamsanjay.patil@amd.com>
Cc: linux-i3c@lists.infradead.org, devicetree@vger.kernel.org,
	robh@kernel.org, conor+dt@kernel.org, Frank.Li@kernel.org,
	"Alexandre Belloni" <alexandre.belloni@bootlin.com>
Subject: Re: [PATCH v2 2/3] i3c: dw: Track the transmit and receive buffer depths separately
Date: Thu, 05 Nov 2026 09:01:14 +0000	[thread overview]
Message-ID: <sashiko-outbox-198746@kernel.org> (raw)
In-Reply-To: <20261105085425.3418230-3-shubhamsanjay.patil@amd.com>

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

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

WARNING: multiple messages have this Message-ID (diff)
From: sashiko-bot@kernel.org
To: "Shubham Patil" <shubhamsanjay.patil@amd.com>
Cc: linux-i3c@lists.infradead.org, devicetree@vger.kernel.org,
	robh@kernel.org, conor+dt@kernel.org, Frank.Li@kernel.org,
	"Alexandre Belloni" <alexandre.belloni@bootlin.com>
Subject: Re: [PATCH v2 2/3] i3c: dw: Track the transmit and receive buffer depths separately
Date: Thu, 05 Nov 2026 09:01:14 +0000	[thread overview]
Message-ID: <sashiko-outbox-198746@kernel.org> (raw)
In-Reply-To: <20261105085425.3418230-3-shubhamsanjay.patil@amd.com>

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

  reply	other threads:[~2026-11-05  9:01 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-11-05  8:54 [PATCH v2 0/3] i3c: account for controller transfer limits Shubham Patil
2026-11-05  8:54 ` Shubham Patil
2026-11-05  8:54 ` [PATCH v2 1/3] i3c: Add helpers to get the maximum transfer size for a device Shubham Patil
2026-11-05  8:54   ` Shubham Patil
2026-11-05 22:04   ` Frank Li
2026-11-05 22:04     ` Frank Li
2026-11-05  8:54 ` [PATCH v2 2/3] i3c: dw: Track the transmit and receive buffer depths separately Shubham Patil
2026-11-05  8:54   ` Shubham Patil
2026-11-05  9:01   ` sashiko-bot [this message]
2026-11-05  9:01     ` sashiko-bot
2026-11-05 22:09     ` Frank Li
2026-11-05 22:09       ` Frank Li
2026-11-06  6:33       ` Patil, Shubham Sanjay
2026-11-06  6:33         ` Patil, Shubham Sanjay
2026-11-05  8:54 ` [PATCH v2 3/3] mctp i3c: Clamp transfer lengths to the controller limits Shubham Patil
2026-11-05  8:54   ` Shubham Patil
2026-11-05 22:10   ` Frank Li
2026-11-05 22:10     ` Frank Li

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=sashiko-outbox-198746@kernel.org \
    --to=sashiko-bot@kernel.org \
    --cc=Frank.Li@kernel.org \
    --cc=alexandre.belloni@bootlin.com \
    --cc=conor+dt@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-i3c@lists.infradead.org \
    --cc=robh@kernel.org \
    --cc=sashiko-reviews@lists.linux.dev \
    --cc=shubhamsanjay.patil@amd.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.