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

On Mon, Aug 17, 2026 at 10:50:58AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [Medium] The commit message claims to add helpers for attaching and detaching I3C devices, but the implementation is completely missing.

Please update commit message

Frank

>
> Pre-existing issues:
> - [High] Accidental removal of ID 0 from `i3c_bus_idr` when `i3c_bus_init()` fails.
> --
>
> --- Patch [1]: [PATCH v15 1/8] i3c: master: Add APIs for I3C hub support ---
> commit 14f349144e98c1c3eedcaa4123a53bf24704569f
> Author: Aman Kumar Pandey <[email protected]>
>
> i3c: master: Add APIs for I3C hub support
>
> This commit adds helpers to prepare for I3C hub support. It exposes
> address slot management and CCC command interfaces, and allows registering
> virtual masters with an explicit firmware node.
>
> > Add helpers for attaching and detaching I3C devices and CCC helpers
> > to check CCC support and send CCC commands, address slot helpers to
> > query and update I3C bus address slot state, registering virtual
>
> [Severity: Medium]
> The commit message claims to add helpers for attaching and detaching I3C
> devices, but the implementation is completely missing from the patch.
>
> The changelog indicates that the attach and detach helpers were dropped in
> v15. Should the commit message be updated to reflect this change?
>
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index f1be38a640ca1..03fb41f0786c5 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
>
> [ ... ]
>
> > @@ -3352,6 +3427,39 @@ int i3c_master_register(struct i3c_master_controller *master,
> >
> >  	return ret;
> >  }
> > +EXPORT_SYMBOL_GPL(i3c_master_register_fwnode);
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does the error handling in
> i3c_master_register_fwnode() corrupt the global IDR state?
>
> If i3c_bus_init() fails at idr_alloc() (e.g., due to duplicate alias in DT),
> it returns an error while leaving i3cbus->id as 0:
>
> drivers/i3c/master.c:i3c_bus_init() {
>     ...
>     ret = idr_alloc(&i3c_bus_idr, i3cbus, start, end, GFP_KERNEL);
>     if (ret < 0)
>         return ret;
>     ...
> }
>
> Taking the goto err_put_dev path then calls put_device(&master->dev), which
> eventually triggers i3c_masterdev_release():
>
> drivers/i3c/master.c:i3c_masterdev_release() {
>     ...
>     i3c_bus_cleanup(bus);
>     ...
> }
>
> This calls idr_remove(&i3c_bus_idr, i3cbus->id). Because i3cbus->id is still 0,
> doesn't this accidentally remove the valid bus 0 from the IDR tree?
>
> This could lead to duplicate sysfs names ('i3c-0') and crashes during
> device_add() for a new bus.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

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