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

On Tue, Aug 04, 2026 at 07:50:35PM +0300, Adrian Hunter wrote:
> 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: [email protected]
> >> Signed-off-by: Adrian Hunter <[email protected]>
> >> ---
> >>
> >>
> >> Changes in V3:
> >>
> >> 	Added Cc: [email protected]
> >>
> >> 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()

Add comments in struct list_head node; in case someone reuse it without
lock at other place

Frank

>
> >
> > 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
> >>
>

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