Re: [PATCH 7/8] i3c: mipi-i3c-hci: Improve AST2700 PIO TX queue utilization

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Calling `of_device_is_compatible()` via the `is_aspeed()` helper inside the high-speed PIO TX data transfer loop causes severe CPU overhead and latency.
- [High] Hardware TOCTOU / Stale write pointer read leading to FIFO overflow.
--

commit 3be95193756af176407d7f4071b09a591d9a4eb3
Author: Billy Tsai <billy_tsai@aspeedtech.com>

i3c: mipi-i3c-hci: Improve AST2700 PIO TX queue utilization

This commit updates the MIPI I3C HCI driver to improve TX FIFO utilization
for the AST2700 controller. It introduces a vendor-specific check to poll
the exact number of free FIFO entries using hardware read/write pointers
instead of relying on a half-empty threshold.

> diff --git a/drivers/i3c/master/mipi-i3c-hci/pio.c b/drivers/i3c/master/mipi-i3c-hci/pio.c
> index 6bd5e43364a1d..3d70257dcc1dc 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/pio.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/pio.c

[ ... ]

> @@ -295,11 +296,23 @@ static bool hci_pio_do_tx(struct i3c_hci *hci, struct hci_pio_data *pio)
>  	p += (xfer->data_len - xfer->data_left) / 4;
>  
>  	while (xfer->data_left >= 4) {
> -		/* bail out if FIFO free space is below set threshold */
> -		if (!(pio_reg_read(INTR_STATUS) & STAT_TX_THLD))
> +		unsigned int avail = pio->tx_thresh_size;
> +
> +		if (is_aspeed(hci)) {

[Severity: High]
Does calling of_device_is_compatible() inside this fastpath byte-transfer
loop cause significant CPU overhead and latency?

Looking at is_aspeed() in vendor_aspeed.h, it parses the Device Tree and
performs string matching:

static inline bool is_aspeed(struct i3c_hci *hci)
{
	return of_device_is_compatible(hci->master.dev.of_node,
					"aspeed,ast2700-i3c-hci");
}

Since this executes repeatedly for every 4 bytes transferred, it might ruin
throughput, especially for non-Aspeed controllers where it gets evaluated
constantly. Would it be better to check compatibility exactly once during
probe and cache the result as a quirk flag?

> +			/*
> +			 * The vendor FIFO pointers give the exact free space,
> +			 * allowing better TX FIFO utilization than the
> +			 * threshold status.
> +			 */
> +			avail = aspeed_i3c_avail_tx_entries(hci);

[Severity: High]
Could this read of the hardware write pointer immediately after posting a
write lead to a FIFO overflow?

Memory-mapped I/O writes over buses like AHB/AXI are posted, and the
hardware controller might need a few clock cycles to commit the data and
update its pointers. 

If aspeed_i3c_avail_tx_entries() reads QUEUE_PTR0 before the hardware
increments the write pointer, won't avail remain artificially high?
If it does, the loop will immediately push another chunk of data,
potentially overflowing the 32-entry hardware FIFO. 

Should the driver locally track and deduct the number of words written from
the available space during the active transfer instead of re-polling the
hardware register?

> +			if (!avail)
> +				return false;
> +		} else if (!(pio_reg_read(INTR_STATUS) & STAT_TX_THLD)) {

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-b4-i3c-hci-ast2700-v1-0-19909e7cbd7e@aspeedtech.com?part=7