Re: [PATCH v19 6/8] i3c: hub: Add support for the I3C interface in the I3C hub

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] i3c: hub: dynamically discovered devices attach to wrong bus bypassing port
- [Medium] i3c: hub: cascaded hubs fail to propagate address reservations to root bus
--

commit 9fb8ad75cf771fba9c1b6bdfec7ca6cd1cb60041
Author: Lakshay Piplani <lakshay.piplani@nxp.com>

i3c: hub: Add support for the I3C interface in the I3C hub

This patch adds virtual I3C bus support for hubs by creating virtual
master controllers for downstream ports and forwarding bus operations
through the hub device. It also provides an interface used by hub
drivers to enable or disable downstream ports during bus transactions.

> diff --git a/drivers/i3c/hub.c b/drivers/i3c/hub.c
> new file mode 100644
> index 0000000000000..5390287680844
> --- /dev/null
> +++ b/drivers/i3c/hub.c
> @@ -0,0 +1,832 @@
[ ... ]
> +static int i3c_hub_do_daa(struct i3c_hub *hub,
> +			  struct i3c_master_controller *controller)
> +{
[ ... ]
> +	/*
> +	 * Downstream devices reachable through hub target-port routes share the
> +	 * parent controller's I3C address space. The hub gates access to a
> +	 * target-port network, but it does not create an independent dynamic
> +	 * address domain per virtual bus.
> +	 *
> +	 * Run DAA on the parent controller so dynamic addresses remain unique
> +	 * across all downstream devices, even when they are behind different
> +	 * target ports.
> +	 *
> +	 * The current hub model shares the parent controller's dynamic-address
> +	 * domain and does not support independent per-port DAA domains, so a
> +	 * DAA result does not go stale behind a port toggle. Addresses reserved
> +	 * from the Device Tree are held for the lifetime of the parent bus and
> +	 * are never released on a port toggle, so enabling or disabling a port
> +	 * only gates reachability and never causes an address to be handed out
> +	 * twice.
> +	 */
> +	ret = i3c_master_do_daa(parent);

[Severity: High]
When i3c_hub_do_daa() forwards DAA to the parent controller here, won't
the parent discover the downstream device and add it to its own
parent->bus.devs.i3c list instead of the virtual bus?

If a downstream device without a static address connects behind the hub
port, and the physical parent controller owns the device record, subsequent
I/O will use the parent's i3c_xfers() implementation. Does this permanently
bypass the virtual hub controller, meaning the i3c_hub_enable_port() routing
logic is never triggered for these devices and transfers will fail?

> +	i3c_hub_disable_port(controller);
> +	mutex_unlock(&hub->lock);
> +
> +	return ret;
> +}
[ ... ]
> +int i3c_hub_reserve_parent_addrslots_from_dt(struct i3c_hub_controller *hubc,
> +					     struct device_node *node)
> +{
[ ... ]
> +		i3c_bus_maintenance_lock(&parent->bus);
> +		status = i3c_bus_get_addr_slot_status(&parent->bus,
> +						      assigned_addr);
> +		if (status == I3C_ADDR_SLOT_FREE)
> +			i3c_bus_set_addr_slot_status(&parent->bus,
> +						     assigned_addr,
> +						     I3C_ADDR_SLOT_I3C_DEV);

[Severity: Medium]
In a cascaded hub topology, won't this only update the addrslots bitmap
of the immediate parent->bus (which is the upstream hub's virtual bus)?

Since DAA is recursively forwarded to the root physical controller, that
root controller assigns dynamic addresses based on its own addrslots bitmap.
Does this mean the nested address reservations fail to reach the root bus,
potentially leading to dynamic address collisions if the root controller
assigns these reserved addresses to newly discovered devices?

> +		i3c_bus_maintenance_unlock(&parent->bus);
> +	}
> +	return 0;
> +}

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