Re: [PATCH v4 04/16] i3c: renesas: Reconfigure the DATBAS register on re-attach

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Hardware transfers are not aborted on software timeout, permanently desynchronizing the bus state and precipitating severe memory corruption.
- [High] Hardware DAA implementation erroneously assumes free device slots are contiguous, assigning already-in-use addresses to new devices under fragmented slot conditions.
- [High] Ignored completion timeout returns in `renesas_i3c_i2c_xfers` lead to silent operational failures.
- [High] Undefined behavior in dynamic `GENMASK` generation causes false phantom device attachment.
--

commit 80894950fcbc5a0242a4c816117f647e2715af0b
Author: Claudiu Beznea <[email protected]>

i3c: renesas: Reconfigure the DATBAS register on re-attach

This commit reconfigures the DATBAS register during re-attach to handle
cases where a device changes its position in the i3c->addrs[] array. It
ensures the correct Device Address Table Basic Register is used and
updates software caches.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 7d448936b74c1..4b81c32b1fdec 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c

[ ... ]

> @@ -892,10 +892,26 @@ static int renesas_i3c_reattach_i3c_dev(struct i3c_dev_desc *dev,
>  	struct i3c_master_controller *m = i3c_dev_get_master(dev);
>  	struct renesas_i3c *i3c = to_renesas_i3c(m);
>  	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
> +	int pos;
> +
> +	pos = renesas_i3c_get_free_pos(i3c);
> +
> +	if (data->index > pos && pos >= 0) {
> +		renesas_writel(i3c->regs, DATBAS(data->index), 0);
> +		i3c->addrs[data->index] = 0;
> +		i3c->free_pos |= BIT(data->index);
> +
> +		data->index = pos;
> +		i3c->free_pos &= ~BIT(data->index);
> +	}
>  
>  	i3c->addrs[data->index] = dev->info.dyn_addr ? dev->info.dyn_addr :
>  							dev->info.static_addr;
>  
> +	renesas_writel(i3c->regs, DATBAS(data->index),
> +		       DATBAS_DVSTAD(dev->info.static_addr) |
> +		       datbas_dvdyad_with_parity(i3c->addrs[data->index]));
> +
>  	return 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but looking at renesas_i3c_daa()
in this file, is it possible for the hardware to overwrite an active device
if slots are fragmented?

In renesas_i3c_daa():

    cmd->cmd0 = NCMDQP_CMD_ATTR(NCMDQP_ADDR_ASSGN) | NCMDQP_ROC |
                NCMDQP_TID(I3C_COMMAND_ADDRESS_ASSIGNMENT) |
                NCMDQP_CMD(I3C_CCC_ENTDAA) | NCMDQP_DEV_INDEX(ret) |
                NCMDQP_DEV_COUNT(i3c->maxdevs - ret) | NCMDQP_TOC;

The code passes i3c->maxdevs - ret to NCMDQP_DEV_COUNT, assuming all
subsequent slots are free. If slot 1 is free but slot 2 is in use, could the
hardware increment its internal DATBAS index and overwrite the active device
in slot 2 when multiple new devices respond to ENTDAA?

[Severity: Critical]
This is a pre-existing issue, but I have a question about how hardware aborts
are handled on software timeout.

When wait_for_completion_timeout() expires in renesas_i3c_wait_xfer():

    time_left = wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000));
    if (!time_left)
        renesas_i3c_dequeue_xfer(i3c, xfer);

The xfer is dequeued, but the hardware transfer doesn't appear to be aborted.
If a new transfer is enqueued, and the hardware eventually completes the
timed-out transfer, could renesas_i3c_resp_isr() blindly apply the late
response to the new transfer buffer?

    xfer = i3c->xferqueue.cur;
    ...
    if (NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) && !cmd->err)
        bytes_remaining = data_len - cmd->rx_count;

    i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, bytes_remaining);

Does this create a scenario where i3c_readl_fifo() copies data into the new
cmd->rx_buf based on the old data_len, potentially causing a buffer overflow?

[Severity: High]
This isn't a bug introduced by this patch, but does the I2C transfer logic
mask completion timeouts?

In renesas_i3c_i2c_xfers():

    time_left = wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
    if (!time_left)
        xfer_failed = true;

    if (cmd->err)
        break;

If the timeout expires, xfer_failed is set to true but cmd->err remains 0
because the ISR never executed. Since cmd->err is 0, the loop does not break,
which seems like it could cause the driver to continue to the next message
and eventually return 0, masking the timeout.

[Severity: High]
This is a pre-existing issue, but can the dynamic mask generation in
renesas_i3c_daa() result in phantom device attachments?

In renesas_i3c_daa(), when configuring the mask for new devices:

    newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);

If no devices respond to the ENTDAA command, cmd->rx_count is set to
i3c->maxdevs - ret. If ret is 0, this makes the upper bound of GENMASK
evaluate to -1. Would executing GENMASK(-1, 0) evaluate to ~0UL and cause
the driver to iterate over all bits, falsely registering phantom devices
via i3c_master_add_i3c_dev_locked()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c