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

[sashiko-bot@kernel.org]

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.

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 <aman.kumarpandey@nxp.com>

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/20260817103844.2142802-1-lakshay.piplani@nxp.com?part=1