Re: [PATCH v18 1/8] i3c: master: Add APIs for I3C hub support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Ignored return value of i3c_master_reattach_i3c_dev_locked leads to dynamic address collision.
--

commit d579609388ce2e1b2127e5830b25d39229bf1664
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>

i3c: master: Add APIs for I3C hub support

This commit adds CCC helpers, address slot helpers, and locking functions
to prepare for I3C hub support. It also enables registering virtual masters
with an explicit firmware node.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index afcd7a21a3e66..9ea4726495ff6 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> @@ -422,11 +434,18 @@ static void i3c_bus_set_addr_slot_status_mask(struct i3c_bus *bus, u16 addr,
>  	*ptr |= ((unsigned long)status & mask) << (bitpos % BITS_PER_LONG);
>  }
>  
> -static void i3c_bus_set_addr_slot_status(struct i3c_bus *bus, u16 addr,
> -					 enum i3c_addr_slot_status status)
> +/**
> + * i3c_bus_set_addr_slot_status() - Set I3C bus address slot status
> + * @bus: I3C bus.
> + * @addr: I3C address to update.
> + * @status: Address slot status to set.
> + */
> +void i3c_bus_set_addr_slot_status(struct i3c_bus *bus, u16 addr,
> +				  enum i3c_addr_slot_status status)
>  {
>  	i3c_bus_set_addr_slot_status_mask(bus, addr, status, I3C_ADDR_SLOT_STATUS_MASK);
>  }
> +EXPORT_SYMBOL_GPL(i3c_bus_set_addr_slot_status);

[Severity: High]
This is a pre-existing issue, but while reviewing this patch and how address
slots are managed, I noticed a logic flaw in __i3c_master_add_i3c_dev_locked() 
that could lead to dynamic address collisions.

If a device is expected to have a specific dynamic address, it attempts to
apply it:

	if (!ret) {
		old_dyn_addr = newdev->info.dyn_addr;
		newdev->info.dyn_addr = expected_dyn_addr;
		i3c_master_reattach_i3c_dev_locked(newdev, old_dyn_addr);
	} else {
		dev_err(&master->dev,
			"Failed to assign reserved/old address to device...");

The return value of i3c_master_reattach_i3c_dev_locked() is discarded here. 
If reattachment fails, that function cleans up by releasing the addresses:

	if (master->ops->reattach_i3c_dev) {
		ret = master->ops->reattach_i3c_dev(dev, old_dyn_addr);
		if (ret) {
			i3c_master_put_i3c_addrs(dev);
			return ret;
		}
	}

This means the address is marked as I3C_ADDR_SLOT_FREE in the bus addrslots. 
However, because the failure is ignored in __i3c_master_add_i3c_dev_locked(),
the device remains active in the bus list. A subsequently added device 
(e.g., via hot-join) could then be assigned this exact same address, causing 
an electrical collision on the bus.

Should we be checking the return value of i3c_master_reattach_i3c_dev_locked()
and detaching the device or reverting the address state if it fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903062108.2712296-1-lakshay.piplani@nxp.com?part=1

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c