Re: [PATCH v4 07/16] i3c: renesas: Clean DATBAS register on detach

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

New issues:
- [Critical] `renesas_i3c_group_devs_in_slots` assumes the target swap slot is occupied by an I3C device, which crashes if the slot is free. It also fails to update the free slot bitmask.

Pre-existing issues:
- [High] The DATBAS registers and device address trackers for unused slots are left dirty after Dynamic Address Assignment (DAA), which can cause hardware TX/RX logic issues.
--

commit bcdea49e3d9ced6f5b774a208065f0889dbb7c12
Author: Claudiu Beznea <[email protected]>

i3c: renesas: Clean DATBAS register on detach

The controller uses DATBAS registers on TX/RX logic. Clean the DATBAS
register for the detached I3C device to avoid issues.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index a63edddbc9fe0..abe24e563d217 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -1042,6 +1042,8 @@ static void renesas_i3c_detach_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);
>  
> +	renesas_writel(i3c->regs, DATBAS(data->index), 0);
> +
>  	i3c_dev_set_master_data(dev, NULL);
>  	i3c->addrs[data->index].addr = 0;
>  	i3c->addrs[data->index].i3c_dev = NULL;

[Severity: Critical]
Since we are allowing I3C devices to be detached and free their slots here,
will this cause a null pointer dereference in
renesas_i3c_group_devs_in_slots() during resume?

If a slot at i2c_pos was previously detached, i3c_addr.i3c_dev can be
completely free. In this scenario, it seems i3c_dev is null and we
dereference it:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_group_devs_in_slots() {
    ...
        /*
         * Swap the I3C device on i2c_pos slot with the I2C dev
         * on pos slot.
         */
        i3c_addr = i3c->addrs[i2c_pos];
        i3c_dev = i3c_addr.i3c_dev;

        i2c_addr = i3c->addrs[pos];
        i2c_dev = i2c_addr.i2c_dev;

        new_i2c_data = i3c_dev_get_master_data(i3c_dev);
    ...
}

It also looks like the function moves the I2C device to i2c_pos but fails
to update the i3c->free_pos bitmask, causing state corruption where the
controller considers the new I2C slot as free. Does this bitmask need to
be updated when swapping devices?

[Severity: High]
This isn't a bug introduced by this patch, but since we are cleaning the
DATBAS registers on detach to avoid tx/rx logic issues, do we also need to
clean them after Dynamic Address Assignment for unassigned slots?

In renesas_i3c_daa(), all free DATBAS registers are pre-configured before
the ENTDAA command:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_daa() {
    ...
        ret = i3c_master_get_free_addr(m, last_addr + 1);
        if (ret < 0)
            return -ENOSPC;

        i3c->addrs[pos].addr = ret;
        last_addr = ret;

        renesas_writel(i3c->regs, DATBAS(pos),
                       datbas_dvdyad_with_parity(ret));
    }
    ...
}

After the ENTDAA command completes, it uses the remaining count
(cmd->rx_count) to determine how many devices were actually assigned.
However, it appears the driver never clears DATBAS(pos) and
i3c->addrs[pos].addr for the remaining unassigned slots.

Could leaving invalid dynamic addresses in the DATBAS registers cause the
controller's tx/rx logic to misbehave?

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

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