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

[Lakshay Piplani]

Hi Krzysztof,

Thanks for the review.

> > + * i3c_hub_init() - Initialize hub context
> > + * @hub: Hub instance
> > + * @ops: Vendor callbacks
> > + * @hub_dev: I3C hub device
> > + */
> > +void i3c_hub_init(struct i3c_hub *hub,
> > +               const struct i3c_hub_ops *ops,
> > +               struct i3c_device *hub_dev) {
> > +     struct i3c_master_controller *parent;
> > +     unsigned int depth;
> > +
> > +     hub->ops = ops;
> > +     hub->hub_dev = hub_dev;
> > +     mutex_init(&hub->lock);
> > +
> > +     if (!IS_ENABLED(CONFIG_LOCKDEP))
>
> So without lockdep you do not initialize? Honestly, kerneldoc is useless here
> and explains nothing. Kerneldoc is pretty clear - you initialize hub context thus
> why hub context should be uninitialized without lockdep?
>

The hub fields and mutex are initialized before this check; only the
lockdep setup is conditional. I'll clarify that in kernel-doc.

> > +             return;
> > +
> > +     if (WARN_ON_ONCE(!hub_dev || !hub_dev->desc))
> > +             return;
>
> How is this possible? Why panicking the machine?
>

The caller already supplies a valid hub device and descriptor. I'll
document that requirement and remove the check.

> > +
> > +     parent = i3c_dev_get_master(hub_dev->desc);
> > +     if (WARN_ON_ONCE(!parent))
> > +             return;
>
> Why panicking here (WARN on panic)?
>

The parent controller is also guaranteed at this call site. I'll
remove this check as well.

> > +
> > +     /*
> > +      * The routing mutex has the same hub nesting depth as the virtual
> > +      * controllers this hub exposes, so the parent controller is one level
> > +      * shallower. Keying it once here, rather than per port, avoids
> > +      * reclassifying the single shared routing mutex from a later port that
> > +      * may already have used it.
> > +      */
> > +     depth = i3c_hub_controller_depth(parent) + 1;
> > +     if (WARN_ONCE(depth > I3C_HUB_MAX_LOCK_DEPTH,
> > +                   "i3c-hub: routing lock depth %u exceeds lockdep support\n",
> > +                   depth))
>
>
> > +             depth = I3C_HUB_MAX_LOCK_DEPTH;
> > +
> > +     lockdep_set_class(&hub->lock, &i3c_hub_routing_lock_keys[depth -
> > +1]); } EXPORT_SYMBOL_GPL(i3c_hub_init);
> > +
> > +const struct i3c_master_controller_ops *i3c_hub_master_ops(void) {
> > +     return &i3c_hub_master_ops_data; }
> > +EXPORT_SYMBOL_GPL(i3c_hub_master_ops);
>
> Do you really need a wrapper call over single variable? Can the i3c hub core
> code be NOT present when your I3C hub driver is buillin?
>

P3H2X4X_I3C_HUB selects I3C_HUB, so the core is built whenever this
driver is enabled. I'll export the const ops object directly and
remove the accessor.

Thanks,
Lakshay

NXP Public