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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] ACKed In-Band Interrupts (IBIs) are permanently discarded when they race with normal transfers.
- [High] Eager polling of the shared RX FIFO corrupts read transfers with IBI payload data if an IBI wins arbitration.
- [High] Race condition in IBI teardown allows late IBIs to be queued after the generic core's completion check, leading to a Use-After-Free.
- [Medium] The controller-wide IBI enable refcount is decremented even if disabling the target on the bus fails.
--

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

This commit adds support for receiving and dequeueing I3C in-band
interrupts (IBIs). The controller interrupt carries IBI events only, and
the IBI ACK enable and its interrupt mask are controller-wide. It handles
demultiplexing shared response and read FIFOs when IBIs race with normal
transfers.

> diff --git a/drivers/i3c/master/amd-i3c-master.c b/drivers/i3c/master/amd-i3c-master.c
> index ef5ad5abb788c..d47b04326ea46 100644
> --- a/drivers/i3c/master/amd-i3c-master.c
> +++ b/drivers/i3c/master/amd-i3c-master.c
[ ... ]
> @@ -206,23 +269,79 @@ static inline bool xi3c_is_resp_available(struct xi3c_master *master)
[ ... ]
> +static bool xi3c_xfer_resp_available(struct xi3c_master *master)
> +{
> +	u32 resp;
> +	u16 len;
> +
> +	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;
> +		}
> +
> +		len = FIELD_GET(XI3C_RESP_BYTES_MASK, resp);
> +		dev_dbg_ratelimited(master->dev,
> +				    "IBI raced with transfer, dropping %u bytes\n",
> +				    len);
> +		xi3c_master_drain_ibi_fifo(master, len);
> +	}
> +
> +	return false;
> +}

[Severity: High]
Since the hardware has already ACKed the IBI on the bus, doesn't draining
and discarding the IBI response here mean it is permanently lost? 

If an IBI arrives while the driver is executing a normal transfer, the
driver masks the IBI interrupt, but the hardware still ACKs the IBI on the
bus. The target device won't resend the IBI once it is ACKed. Could this
cause essential target events like sensor data ready or hot-join requests to
be dropped entirely?

[ ... ]
> @@ -437,13 +575,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;

[Severity: High]
Is it safe to eagerly poll the shared RX FIFO before validating the
transfer response?

If an IBI wins arbitration concurrently, its payload might enter the shared
RX FIFO first. Because xi3c_master_rd_from_rx_fifo() is called within this
loop before the transfer's own response word arrives, it seems the driver
might mistakenly read the IBI payload bytes directly into the normal read
transfer's buffer. Could this corrupt normal read transfers with IBI
payload data?

[ ... ]
> @@ -1052,9 +1207,296 @@ static void xi3c_master_bus_cleanup(struct i3c_master_controller *m)
[ ... ]
> +static void xi3c_master_free_ibi(struct i3c_dev_desc *dev)
> +{
> +	struct xi3c_i3c_dev_data *data = i3c_dev_get_master_data(dev);
> +	struct i3c_master_controller *m = i3c_dev_get_master(dev);
> +	struct xi3c_master *master = to_xi3c_master(m);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&master->ibi.lock, flags);
> +	master->ibi.slots[data->ibi_slot] = NULL;
> +	spin_unlock_irqrestore(&master->ibi.lock, flags);
> +
> +	i3c_generic_ibi_free_pool(data->ibi_pool);
> +	i3c_dev_set_master_data(dev, NULL);
> +	kfree(data);
> +}
[ ... ]
> +static int xi3c_master_disable_ibi(struct i3c_dev_desc *dev)
> +{
> +	struct i3c_master_controller *m = i3c_dev_get_master(dev);
> +	struct xi3c_master *master = to_xi3c_master(m);
> +	int ret;
> +
> +	ret = i3c_master_disec_locked(m, dev->info.dyn_addr, I3C_CCC_EVENT_SIR);
> +
> +	xi3c_master_ibi_disarm(master);
> +
> +	return ret;
> +}

[Severity: Medium]
If i3c_master_disec_locked() fails (for example, if the target NACKs the
DISEC command), the device is still considered enabled by the core. Does
unconditionally calling xi3c_master_ibi_disarm() here risk dropping the
master->ibi.enabled_count prematurely? This could unintentionally mask the
global IBI interrupt.

[Severity: High]
Does this teardown sequence need a synchronization barrier (like
synchronize_irq()) to ensure the hard IRQ handler has finished processing
events?

If a late IBI fires just before the DISEC command, the IRQ might be pending
on another CPU. Because xi3c_master_disable_ibi() completes without waiting
for the IRQ, the core might see no pending IBIs and proceed to call
xi3c_master_free_ibi(), freeing the data->ibi_pool.

Could the concurrent IRQ handler then execute, copy the payload, and queue
a work item that accesses the now-freed IBI pool?

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