Re: [PATCH v2 2/3] i3c: master: amd: Add support for in-band interrupts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Inconsistent register read order causes the polled IBI handler to read the wrong Target Address (DA).
- [High] Premature publication of the IBI slot causes a NULL pointer dereference in the IRQ handler.
- [High] Eager polling of the shared RX FIFO corrupts read transfers and drops IBI payloads.
--

--- Patch [2]: [PATCH v2 2/3] i3c: master: amd: Add support for in-band interrupts ---
commit 09c533cb3c7b670acbbd25a3bdfd78e983efb676
Author: Shubham Patil <shubhamsanjay.patil@amd.com>

i3c: master: amd: Add support for in-band interrupts

This commit adds support for receiving and dequeueing I3C in-band interrupts.
The IBI operations are assembled at probe time if the capability is present in
the device tree. It also introduces logic to handle IBIs that race with normal
transfers by demultiplexing the response FIFO by TID.

> diff --git a/drivers/i3c/master/amd-i3c-master.c b/drivers/i3c/master/amd-i3c-master.c
> index ef5ad5abb788c..92a9f43f89344 100644
> --- a/drivers/i3c/master/amd-i3c-master.c
> +++ b/drivers/i3c/master/amd-i3c-master.c
[ ... ]
> @@ -206,23 +273,88 @@ static inline bool xi3c_is_resp_available(struct xi3c_master *master)
[ ... ]
> +static bool xi3c_xfer_resp_available(struct xi3c_master *master)
> +{
> +	u32 resp;
> +	u8 da;
> +
> +	if (master->xfer_resp_valid)
> +		return true;
> +
> +	while (xi3c_is_resp_available(master)) {
> +		resp = ioread32(master->membase + XI3C_RESP_STATUS_FIFO_OFFSET);
> +
> +		if (FIELD_GET(XI3C_RESP_TID_MASK, resp) != XI3C_IBI_RESP_TID) {
> +			master->xfer_resp = resp;
> +			master->xfer_resp_valid = true;
> +			return true;
> +		}
> +
> +		da = FIELD_GET(XI3C_ADDR_MASK,
> +			       ioread32(master->membase + XI3C_IBI_TARGET_ADDR_OFFSET));

[Severity: High]
Does popping the response FIFO before reading the Target Address register
cause the wrong address to be read?

In the hardware IRQ handler, the Target Address is read before waiting for
and popping the response FIFO:

xi3c_master_handle_ibi() {
    ibi_reg = ioread32(master->membase + XI3C_IBI_TARGET_ADDR_OFFSET);
    da = FIELD_GET(XI3C_ADDR_MASK, ibi_reg);
    if (xi3c_wait_resp_atomic(master)) { ... }
    resp = ioread32(master->membase + XI3C_RESP_STATUS_FIFO_OFFSET);
}

If the response is popped first here, does it advance the FIFO head and cause
this read to return the address of the next queued IBI (or garbage) instead of
the current one?

> +		dev_dbg_ratelimited(master->dev,
> +				    "IBI from 0x%02x raced with transfer\n", da);
> +		xi3c_master_process_ibi(master, resp, da);
[ ... ]
> @@ -437,13 +605,19 @@ static int xi3c_master_read(struct xi3c_master *master, struct xi3c_cmd *cmd)
[ ... ]
>  	/* Read data from rx fifo */
> -	while (cmd->rx_len > 0 && !xi3c_is_resp_available(master)) {
> +	while (cmd->rx_len > 0 && !xi3c_xfer_resp_available(master)) {
>  		if (time_after(jiffies, timeout)) {
>  			dev_err(master->dev, "XI3C read timeout\n");
>  			return -EIO;
>  		}
>  		xi3c_master_rd_from_rx_fifo(master, cmd);
>  		usleep_range(XI3C_POLL_INTERVAL_US, 2 * XI3C_POLL_INTERVAL_US);
>  	}

[Severity: High]
Does this eager polling of the shared RX FIFO risk consuming an IBI payload
into the transfer buffer before the IBI response is processed?

If an IBI executes right before a read transfer, its payload will enter the RX
FIFO. Since !xi3c_xfer_resp_available() can evaluate to true if the response
word hasn't been pushed to the Response FIFO yet, the loop might call
xi3c_master_rd_from_rx_fifo().

Could this blindly copy the IBI payload into the read transfer's buffer, leaving
an empty RX FIFO for xi3c_master_process_ibi() and dropping the IBI data?

[ ... ]
> @@ -1052,9 +1242,337 @@ static void xi3c_master_bus_cleanup(struct i3c_master_controller *m)
[ ... ]
> +static int xi3c_master_request_ibi(struct i3c_dev_desc *dev,
> +				   const struct i3c_ibi_setup *req)
> +{
[ ... ]
> +	spin_lock_irqsave(&master->ibi.lock, flags);
> +	for (i = 0; i < ARRAY_SIZE(master->ibi.slots); i++) {
> +		if (!master->ibi.slots[i]) {
> +			data->ibi_slot = i;
> +			master->ibi.slots[i] = dev;
> +			break;
> +		}
> +	}
> +	spin_unlock_irqrestore(&master->ibi.lock, flags);
> +
> +	if (data->ibi_slot < 0) {
> +		dev_err(master->dev, "IBI: no free slot for addr 0x%02x\n",
> +			dev->info.dyn_addr);
> +		i3c_generic_ibi_free_pool(data->ibi_pool);
> +		kfree(data);
> +		return -ENOSPC;
> +	}
> +
> +	i3c_dev_set_master_data(dev, data);
> +
> +	return 0;
> +}

[Severity: High]
Can an incoming IBI trigger a NULL pointer dereference if it arrives right
after the slot is published but before the master data is set?

If the controller's IBI ACK is already armed by another device, and the hardware
sends an IBI after master->ibi.slots[i] = dev but before
i3c_dev_set_master_data(dev, data), the IRQ handler (xi3c_master_process_ibi)
could look up the device and call i3c_dev_get_master_data(dev).

Would this return NULL and result in a crash when accessing data->ibi_pool?
Should i3c_dev_set_master_data(dev, data) be called before the slot is made
visible to the IRQ handler?

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