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

[Lakshay Piplani]

Hi Frank,

Thanks for the review.

> > > +/**
> > > + * i3c_hub_do_daa() - Perform DAA via hub port
> > > + * @hub: Hub instance
> > > + * @controller: Virtual controller for a hub port
> > > + *
> > > + * Enables the port connection, performs DAA on the parent
> > > +controller,
> > > + * then disables the connection.
> > > + */
> > > +static int i3c_hub_do_daa(struct i3c_hub *hub,
> > > +                   struct i3c_master_controller *controller) {
> > > + struct i3c_master_controller *parent;
> > > + int ret;
> > > +
> > > + if (!hub || !hub->hub_dev)
> > > +         return -ENODEV;
> > > +
> > > + parent = i3c_dev_get_master(hub->hub_dev->desc);
> > > + if (!parent)
> > > +         return -ENODEV;
> > > +
> > > + /*
> > > +  * Lock order: hub routing mutex before the parent bus lock (taken here
> > > +  * inside i3c_master_do_daa()). The depth-keyed lockdep classes above
> > > +  * keep this nesting acyclic when the parent is itself a hub.
> > > +  */
> > > + mutex_lock(&hub->lock);
> > > + i3c_hub_enable_port(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.
> > > +  */
> > > + ret = i3c_master_do_daa(parent);
> >
> > [Severity: High]
> > Does delegating DAA to the physical parent controller break
> > reachability for downstream devices?
> >
> > When devices respond to ENTDAA, they are instantiated on the parent
> > bus rather than the virtual bus. If drivers subsequently initiate
> > transfers, they use the parent controller's transfer operations
> > directly, bypassing the hub's i3c_hub_master_priv_xfers(). As a
> > result, the hub target port is never enabled during the transfer,
> > leaving these downstream devices permanently unreachable.
>
> where reorder port, is it true?
>


No, P3H2x4x never re-orders or re-switches ports. All I3C ports are connected once at probe and stay connected (always_enable makes enable/disable_port no-ops), so a prior DAA never goes stale.
DAA runs on the parent on purpose: all downstream devices share the parent's single address domain, so addresses stay unique across ports.
DT addresses are reserved for the parent-bus lifetime and aren't freed on a port toggle, so enable/disable only gates reachability, never reallocates. I'll add a comment.
(enable/disable_port are kept as part of the generic hub op set and would be the hook for optional sysfs port control later - out of scope here.)

> >
> > > + i3c_hub_disable_port(controller);
> > > + mutex_unlock(&hub->lock);
> > > +
> > > + return ret;
> > > +}
> > [ ... ]
> > > +static int i3c_hub_send_ccc_cmd(struct i3c_hub *hub,
> > > +                         struct i3c_master_controller *controller,
> > > +                         struct i3c_ccc_cmd *cmd)
> > > +{
> > > + struct i3c_master_controller *parent;
> > > + int ret;
> > > +
> > > + if (!hub || !hub->hub_dev)
> > > +         return -ENODEV;
> > > +
> > > + parent = i3c_dev_get_master(hub->hub_dev->desc);
> > > + if (!parent)
> > > +         return -ENODEV;
> > > +
> > > + mutex_lock(&hub->lock);
> > > + i3c_hub_enable_port(controller);
> > > + ret = i3c_master_send_ccc_cmd(parent, cmd);
> >
> > [Severity: Medium]
> > Can forwarding unfiltered broadcast CCCs to the parent controller
> > break virtual bus isolation?
> >
> > If a driver on a specific virtual bus issues broadcast commands (such
> > as SETMRL, SETMWL, or ENEC), this forwards them to the parent
> > controller which broadcasts them globally. This will unintentionally
> > alter the configuration or event state of all peer devices on the
> > parent bus and other open target ports.
>
> Look like system design limiation, add comments here incase it happen.
>

Agreed - it's a system limitation, I'll note that a broadcast CCC on a virtual bus has parent-bus scope. RSTDAA stays blocked, as forwarding it would reset the hub's own address.

> >
> > > + 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) {
> > > + struct i3c_master_controller *parent = hubc->parent;
> > > + enum i3c_addr_slot_status status;
> > > + u32 assigned_addr;
> > > + u32 reg[3];
> > > + int ret;
> > > +
> > > + if (!parent || !node)
> > > +         return -ENODEV;
> > > +
> > > + for_each_available_child_of_node_scoped(node, child) {
> > > +         /*
> > > +          * Only consider addressable bus nodes: a valid "reg" is
> > > +          * required to describe a device, but its static-address value
> > > +          * does not affect the reservation below.
> > > +          */
> > > +         ret = of_property_read_variable_u32_array(child, "reg", reg, 1,
> 3);
> > > +         if (ret < 0)
> > > +                 continue;
> > > +
> > > +         ret = of_property_read_u32(child, "assigned-address",
> &assigned_addr);
> > > +         if (ret)
> > > +                 continue;
> > > +
> > > +         /* Skip nodes without a usable dynamic address. */
> > > +         if (!assigned_addr || assigned_addr > I3C_MAX_ADDR)
> > > +                 continue;
> > > +
> > > +         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: High]
> > Does this code permanently leak parent bus address slots when the hub
> > driver is removed?
> >
> > The address slots in the parent bus state are set to I3C_ADDR_SLOT_I3C_DEV.
> > However, there appears to be no corresponding teardown mechanism
> > provided to revert these slots to I3C_ADDR_SLOT_FREE when the virtual
> > controllers are destroyed or the hub driver is unbound. This leak can
> > eventually exhaust the address space and cause failures for subsequent
> devices.
>
> I suppose if hub have hotplug later, we have to reserver all static/assigned
> address. Can you add comments here.

Agreed - I'll add a comment. The reserved addresses come from the fixed DT topology and are held for the parent-bus lifetime. If the hub is removed and comes back, it re-reads the same DT and reserves the same addresses again, so nothing is used up over time.

>
> Thank you for great work, fixed the most problem.
>
> Frank

Thanks, Frank. I appreciate your reviews and feedback throughout this series.

Regards
Lakshay

NXP Confidential