Re: [PATCH V3 01/14] i3c: master: Fix recursive locking during device registration

[Adrian Hunter <adrian.hunter@intel.com>]

On 04/08/2026 19:46, Frank Li wrote:
> On Tue, Aug 04, 2026 at 04:37:57PM +0300, Adrian Hunter wrote:
>> i3c_master_register_new_i3c_devs() registers newly discovered devices
>> while holding i3c_bus_normaluse_lock(), a down_read().  device_register()
>> can immediately probe the device, and probe callbacks typically invoke
>> I3C helpers that take i3c_bus_normaluse_lock() again, leading to a
>> recursive acquisition of the same rwsem.  rwsems do not support recursive
>> read locking and can deadlock when a writer is waiting.  See the
>> "Recursive read locks" section of Documentation/locking/lockdep-design.rst.
>>
>> For example, with Intel LPSS I3C, LOCKDEP generates a WARNING like:
>>   # echo intel-lpss-i3c.0 > /sys/bus/platform/drivers/mipi-i3c-hci/unbind
>>   # echo intel-lpss-i3c.0 > /sys/bus/platform/drivers/mipi-i3c-hci/bind
>>   WARNING: possible recursive locking detected
>>   kworker/5:1/94 is trying to acquire lock:
>>   ffff88811c810d78 (&i3cbus->lock){++++}-{4:4}, at: i3c_device_match_id+0x45/0x370
>>   but task is already holding lock:
>>   ffff88811c810d78 (&i3cbus->lock){++++}-{4:4}, at: i3c_master_reg_work_fn+0x21/0x5f0
>>
>> Fix this by separating device creation from device registration.
>> Populate desc->dev under the maintenance lock, collect the devices that
>> still need registration into a local list, then release the lock before
>> calling device_register().  Finally retake the lock and clean up any
>> devices that failed to register.
>>
>> Use the maintenance lock rather than the normal-use lock while adding
>> device objects.  A write-side maintenance lock prevents readers from
>> observing a partially initialized desc->dev during initial device
>> population, or desc->dev disappearing if registration fails.
>>
>> The local list requires a list node, so add a list node member to struct
>> i3c_device.
>>
>> Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>
>>
>> Changes in V3:
>>
>> 	Added Cc: stable@vger.kernel.org
>>
>> Changes in V2:
>>
>> 	New patch
>>
>>
>>  drivers/i3c/master.c       | 45 ++++++++++++++++++++++++++++----------
>>  include/linux/i3c/master.h |  2 ++
>>  2 files changed, 35 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
>> index f485b98805cf..d2fb1a110521 100644
>> --- a/drivers/i3c/master.c
>> +++ b/drivers/i3c/master.c
>> @@ -2069,12 +2069,21 @@ static int i3c_master_early_i3c_dev_add(struct i3c_master_controller *master,
>>  static void
>>  i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
>>  {
>> +	struct i3c_device *i3cdev, *tmp;
>>  	struct i3c_dev_desc *desc;
>> +	LIST_HEAD(i3c_unreg_devs);
>>  	int ret;
>>
>>  	if (!master->init_done)
>>  		return;
>>
>> +	i3c_bus_maintenance_lock(&master->bus);
>> +
>> +	if (master->shutting_down) {
>> +		i3c_bus_maintenance_unlock(&master->bus);
>> +		return;
>> +	}
>> +
>>  	i3c_bus_for_each_i3cdev(&master->bus, desc) {
>>  		if (desc->dev || !desc->info.dyn_addr || desc == master->this)
>>  			continue;
>> @@ -2104,25 +2113,37 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
>>  		if (desc->boardinfo)
>>  			device_set_node(&desc->dev->dev, desc->boardinfo->fwnode);
>>
>> -		ret = device_register(&desc->dev->dev);
>> -		if (ret) {
>> -			dev_err(&master->dev,
>> -				"Failed to add I3C device (err = %d)\n", ret);
>> -			desc->dev->desc = NULL;
>> -			put_device(&desc->dev->dev);
>> -			desc->dev = NULL;
>> -		}
>> +		list_add_tail(&desc->dev->node, &i3c_unreg_devs);
>> +	}
>> +
>> +	i3c_bus_maintenance_unlock(&master->bus);
>> +
>> +	list_for_each_entry_safe(i3cdev, tmp, &i3c_unreg_devs, node) {
>> +		ret = device_register(&i3cdev->dev);
>> +		if (ret)
>> +			dev_err(&master->dev, "Failed to add I3C device (err = %d)\n", ret);
>> +		else
>> +			list_del_init(&i3cdev->node);
> 
> Is it risk del node without acquire lock?

The list head is local i3c_unreg_devs, and i3c_master_register_new_i3c_devs()
is not permitted to race with itself, by being called only from the work
function i3c_master_reg_work_fn()

> 
> Frank
> 
>> +	}
>> +
>> +	i3c_bus_maintenance_lock(&master->bus);
>> +
>> +	list_for_each_entry_safe(i3cdev, tmp, &i3c_unreg_devs, node) {
>> +		list_del(&i3cdev->node);
>> +		desc = i3cdev->desc;
>> +		i3cdev->desc = NULL;
>> +		put_device(&i3cdev->dev);
>> +		desc->dev = NULL;
>>  	}
>> +
>> +	i3c_bus_maintenance_unlock(&master->bus);
>>  }
>>
>>  static void i3c_master_reg_work_fn(struct work_struct *work)
>>  {
>>  	struct i3c_master_controller *master = container_of(work, typeof(*master), reg_work);
>>
>> -	i3c_bus_normaluse_lock(&master->bus);
>> -	if (!master->shutting_down)
>> -		i3c_master_register_new_i3c_devs(master);
>> -	i3c_bus_normaluse_unlock(&master->bus);
>> +	i3c_master_register_new_i3c_devs(master);
>>  }
>>
>>  /**
>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
>> index 2dc139a217bf..2b96c4ea75fb 100644
>> --- a/include/linux/i3c/master.h
>> +++ b/include/linux/i3c/master.h
>> @@ -238,6 +238,7 @@ struct i3c_dev_desc {
>>   *	  every time the I3C device is rediscovered with a different dynamic
>>   *	  address assigned
>>   * @bus: I3C bus this device is attached to
>> + * @node: unregistered device list node
>>   *
>>   * I3C device object exposed to I3C device drivers. The takes care of linking
>>   * this object to the relevant &struct_i3c_dev_desc one.
>> @@ -248,6 +249,7 @@ struct i3c_device {
>>  	struct device dev;
>>  	struct i3c_dev_desc *desc;
>>  	struct i3c_bus *bus;
>> +	struct list_head node;
>>  };
>>
>>  /*
>> --
>> 2.53.0
>>