Re: [PATCH 6/8] i3c: master: Defer new-device registration out of DAA caller context

[Adrian Hunter]

On 22/05/2026 21:59, Frank Li wrote:
> On Fri, May 22, 2026 at 07:52:17AM +0300, Adrian Hunter wrote:
>> On 21/05/2026 21:32, Frank Li wrote:
>>> On Fri, May 15, 2026 at 07:42:20PM +0300, Adrian Hunter wrote:
>>>> On 13/05/2026 22:03, Frank Li wrote:
>>>>> On Wed, May 13, 2026 at 08:45:55AM +0300, Adrian Hunter wrote:
>>>>>> On 12/05/2026 19:39, Frank Li wrote:
>>>>>>> On Tue, May 12, 2026 at 03:17:30PM +0300, Adrian Hunter wrote:
>>>>>>>> Master drivers may invoke i3c_master_do_daa_ext() during resume to
>>>>>>>> re-run Dynamic Address Assignment.  As well as assigning addresses to
>>>>>>>> any newly arrived devices, this restores the dynamic address of devices
>>>>>>>> that lost it across system suspend, so it has to run as part of the
>>>>>>>> controller's resume path.
>>>>>>>>
>>>>>>>> A side effect of i3c_master_do_daa_ext() today is that it also
>>>>>>>> registers any newly discovered I3C devices with the driver model
>>>>>>>> inline, via i3c_master_register_new_i3c_devs().  Doing that from the
>>>>>>>> resume path is problematic: a hot-join-capable device may join the bus
>>>>>>>> during this same DAA, and registering it immediately would push driver
>>>>>>>> model work (probing, sysfs, etc.) into the controller's resume context,
>>>>>>>> where the rest of the system is not yet fully resumed and the
>>>>>>>> controller driver is still partway through its own resume sequence.
>>>>>>>>
>>>>>>>> Decouple discovery from registration: add a reg_work work item to
>>>>>>>> struct i3c_master_controller and have i3c_master_do_daa_ext() queue it
>>>>>>>> on master->wq (the freezable workqueue) instead of calling
>>>>>>>> i3c_master_register_new_i3c_devs() directly.  The worker performs the
>>>>>>>> registration only when the controller is not shutting_down, and is
>>>>>>>> cancelled alongside hj_work in i3c_master_shutdown().  Because wq is
>>>>>>>> freezable, any newly observed devices end up being registered after
>>>>>>>> the system has finished resuming.
>>>>>>>>
>>>>>>>> i3c_master_register() also routes its initial post-bus-init registration
>>>>>>>> through reg_work, using flush_work() to keep probe-time behavior
>>>>>>>> synchronous.  This keeps a single registration code path and ensures the
>>>>>>>> worker is the only writer of desc->dev.
>>>>>>>
>>>>>>> why not direct use hj_work?
>>>>>>
>>>>>> i3c_master_register_new_i3c_devs() use of desc->dev is racy, so
>>>>>> i3c_master_register_new_i3c_devs() must not be allowed to race
>>>>>> with itself.  Having it only ever run via reg_work achieves that.
>>>>>
>>>>> Sorry, I have not understand these, Can provide some detail?
>>>>
>>>> >From i3c_master_register_new_i3c_devs():
>>>>
>>>> 	i3c_bus_for_each_i3cdev(&master->bus, desc) {
>>>> 		if (desc->dev || !desc->info.dyn_addr || desc == master->this)
>>>> 			continue;
>>>>
>>>> 		desc->dev = kzalloc_obj(*desc->dev);
>>>> 		...
>>>> 		ret = device_register(&desc->dev->dev);
>>>>
>>>> This is done under the shared i3c_bus_normaluse_lock(), so there can
>>>
>>> i3c_bus_normaluse_lock() may is wrong, suppose it should be
>>> i3c_bus_maintenance_lock(), register new devices change i3c bus's
>>> hierarchical structure.
>>
>> If device_register() probes the device and the probe tries to
>> access the device, won't it deadlock if i3c_bus_maintenance_lock()
>> is held.
> 
> Okay,
> 
> Reviewed-by: Frank Li <Frank.Li@xxxxxxx>

Thanks!

If you have time, there is still patch 2 of the patch set to review ;-)

	https://lore.kernel.org/linux-i3c/20260518115520.98335-3-adrian.hunter@xxxxxxxxx

> 
>>
>>>
>>> Frank
>>>
>>>> be 2 or more instances of i3c_master_register_new_i3c_devs() running
>>>> at the same time.  They might all see desc->dev is NULL and then all
>>>> of them try to initialize and register a dev for the same I3C device.
>>>>
>>