[PATCH v3 0/4] i2c: Replace lists of special clients with flagging of such clients

[PATCH v3 0/4] i2c: Replace lists of special clients with flagging of such clients

[Heiner Kallweit]

So far lists are used to track special clients, i.e. auto-detected and
userspace-created clients. The same functionality can be achieved much
simpler by flagging such clients.

v2:
- The i2c_driver.clients list is core-internal, however there's an ALSA
  driver using it. So add patch 1 to address this first.

v3:
- Add missing mutex locks to patches 2 and 3

Heiner Kallweit (4):
  ALSA: ppc: Remove i2c client removal hack
  i2c: Replace list-based mechanism for handling auto-detected clients
  i2c: Replace list-based mechanism for handling userspace-created
    clients
  i2c: core: Remove obsolete members of i2c_adapter and i2c_client

 drivers/i2c/i2c-core-base.c | 115 +++++++++++++-----------------------
 include/linux/i2c.h         |  10 +---
 sound/ppc/keywest.c         |   7 +--
 3 files changed, 43 insertions(+), 89 deletions(-)

-- 
2.47.0

[PATCH v3 1/4] ALSA: ppc: Remove i2c client removal hack

[Heiner Kallweit]

The i2c_driver.clients list is internal to I2C core and is going
to be removed.  No driver should access it. Unregister the
i2c client explicitly before deleting the i2c driver.

Reviewed-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 sound/ppc/keywest.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/sound/ppc/keywest.c b/sound/ppc/keywest.c
index 3d3513d9d..4ce81ac7f 100644
--- a/sound/ppc/keywest.c
+++ b/sound/ppc/keywest.c
@@ -61,12 +61,6 @@ static int keywest_attach_adapter(struct i2c_adapter *adapter)
 		return -ENODEV;
 	}
 	
-	/*
-	 * Let i2c-core delete that device on driver removal.
-	 * This is safe because i2c-core holds the core_lock mutex for us.
-	 */
-	list_add_tail(&keywest_ctx->client->detected,
-		      &to_i2c_driver(keywest_ctx->client->dev.driver)->clients);
 	return 0;
 }
 
@@ -99,6 +93,7 @@ static struct i2c_driver keywest_driver = {
 void snd_pmac_keywest_cleanup(struct pmac_keywest *i2c)
 {
 	if (keywest_ctx && keywest_ctx == i2c) {
+		i2c_unregister_device(keywest_ctx->client);
 		i2c_del_driver(&keywest_driver);
 		keywest_ctx = NULL;
 	}
-- 
2.47.0

[PATCH v3 2/4] i2c: Replace list-based mechanism for handling auto-detected clients

[Heiner Kallweit]

So far a list is used to track auto-detected clients per driver.
The same functionality can be achieved much simpler by flagging
auto-detected clients.

Two notes regarding the usage of driver_for_each_device:
In our case it can't fail, however the function is annotated __must_check.
So a little workaround is needed to avoid a compiler warning.
Then we may remove nodes from the list over which we iterate.
This is safe, see the explanation at the beginning of lib/klist.c.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v3:
- protect client removal with core_lock mutex
---
 drivers/i2c/i2c-core-base.c | 52 ++++++++++++-------------------------
 include/linux/i2c.h         |  3 +--
 2 files changed, 17 insertions(+), 38 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 7c810893b..42c62839d 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1696,23 +1696,6 @@ int i2c_add_numbered_adapter(struct i2c_adapter *adap)
 }
 EXPORT_SYMBOL_GPL(i2c_add_numbered_adapter);
 
-static void i2c_do_del_adapter(struct i2c_driver *driver,
-			      struct i2c_adapter *adapter)
-{
-	struct i2c_client *client, *_n;
-
-	/* Remove the devices we created ourselves as the result of hardware
-	 * probing (using a driver's detect method) */
-	list_for_each_entry_safe(client, _n, &driver->clients, detected) {
-		if (client->adapter == adapter) {
-			dev_dbg(&adapter->dev, "Removing %s at 0x%x\n",
-				client->name, client->addr);
-			list_del(&client->detected);
-			i2c_unregister_device(client);
-		}
-	}
-}
-
 static int __unregister_client(struct device *dev, void *dummy)
 {
 	struct i2c_client *client = i2c_verify_client(dev);
@@ -1728,12 +1711,6 @@ static int __unregister_dummy(struct device *dev, void *dummy)
 	return 0;
 }
 
-static int __process_removed_adapter(struct device_driver *d, void *data)
-{
-	i2c_do_del_adapter(to_i2c_driver(d), data);
-	return 0;
-}
-
 /**
  * i2c_del_adapter - unregister I2C adapter
  * @adap: the adapter being unregistered
@@ -1757,11 +1734,6 @@ void i2c_del_adapter(struct i2c_adapter *adap)
 	}
 
 	i2c_acpi_remove_space_handler(adap);
-	/* Tell drivers about this removal */
-	mutex_lock(&core_lock);
-	bus_for_each_drv(&i2c_bus_type, NULL, adap,
-			       __process_removed_adapter);
-	mutex_unlock(&core_lock);
 
 	/* Remove devices instantiated from sysfs */
 	mutex_lock_nested(&adap->userspace_clients_lock,
@@ -1780,8 +1752,10 @@ void i2c_del_adapter(struct i2c_adapter *adap)
 	 * we can't remove the dummy devices during the first pass: they
 	 * could have been instantiated by real devices wishing to clean
 	 * them up properly, so we give them a chance to do that first. */
+	mutex_lock(&core_lock);
 	device_for_each_child(&adap->dev, NULL, __unregister_client);
 	device_for_each_child(&adap->dev, NULL, __unregister_dummy);
+	mutex_unlock(&core_lock);
 
 	/* device name is gone after device_unregister */
 	dev_dbg(&adap->dev, "adapter [%s] unregistered\n", adap->name);
@@ -2001,7 +1975,6 @@ int i2c_register_driver(struct module *owner, struct i2c_driver *driver)
 	/* add the driver to the list of i2c drivers in the driver core */
 	driver->driver.owner = owner;
 	driver->driver.bus = &i2c_bus_type;
-	INIT_LIST_HEAD(&driver->clients);
 
 	/* When registration returns, the driver core
 	 * will have called probe() for all matching-but-unbound devices.
@@ -2019,10 +1992,13 @@ int i2c_register_driver(struct module *owner, struct i2c_driver *driver)
 }
 EXPORT_SYMBOL(i2c_register_driver);
 
-static int __process_removed_driver(struct device *dev, void *data)
+static int __i2c_unregister_detected_client(struct device *dev, void *argp)
 {
-	if (dev->type == &i2c_adapter_type)
-		i2c_do_del_adapter(data, to_i2c_adapter(dev));
+	struct i2c_client *client = i2c_verify_client(dev);
+
+	if (client && client->flags & I2C_CLIENT_AUTO)
+		i2c_unregister_device(client);
+
 	return 0;
 }
 
@@ -2033,7 +2009,12 @@ static int __process_removed_driver(struct device *dev, void *data)
  */
 void i2c_del_driver(struct i2c_driver *driver)
 {
-	i2c_for_each_dev(driver, __process_removed_driver);
+	mutex_lock(&core_lock);
+	/* Satisfy __must_check, function can't fail */
+	if (driver_for_each_device(&driver->driver, NULL, NULL,
+				   __i2c_unregister_detected_client)) {
+	}
+	mutex_unlock(&core_lock);
 
 	driver_unregister(&driver->driver);
 	pr_debug("driver [%s] unregistered\n", driver->driver.name);
@@ -2460,6 +2441,7 @@ static int i2c_detect_address(struct i2c_client *temp_client,
 	/* Finally call the custom detection function */
 	memset(&info, 0, sizeof(struct i2c_board_info));
 	info.addr = addr;
+	info.flags = I2C_CLIENT_AUTO;
 	err = driver->detect(temp_client, &info);
 	if (err) {
 		/* -ENODEV is returned if the detection fails. We catch it
@@ -2486,9 +2468,7 @@ static int i2c_detect_address(struct i2c_client *temp_client,
 		dev_dbg(&adapter->dev, "Creating %s at 0x%02x\n",
 			info.type, info.addr);
 		client = i2c_new_client_device(adapter, &info);
-		if (!IS_ERR(client))
-			list_add_tail(&client->detected, &driver->clients);
-		else
+		if (IS_ERR(client))
 			dev_err(&adapter->dev, "Failed creating %s at 0x%02x\n",
 				info.type, info.addr);
 	}
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 388ce71a2..c4c8e841a 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -244,7 +244,6 @@ enum i2c_driver_flags {
  * @id_table: List of I2C devices supported by this driver
  * @detect: Callback for device detection
  * @address_list: The I2C addresses to probe (for detect)
- * @clients: List of detected clients we created (for i2c-core use only)
  * @flags: A bitmask of flags defined in &enum i2c_driver_flags
  *
  * The driver.owner field should be set to the module owner of this driver.
@@ -299,7 +298,6 @@ struct i2c_driver {
 	/* Device detection callback for automatic device creation */
 	int (*detect)(struct i2c_client *client, struct i2c_board_info *info);
 	const unsigned short *address_list;
-	struct list_head clients;
 
 	u32 flags;
 };
@@ -334,6 +332,7 @@ struct i2c_client {
 #define I2C_CLIENT_SLAVE	0x20	/* we are the slave */
 #define I2C_CLIENT_HOST_NOTIFY	0x40	/* We want to use I2C host notify */
 #define I2C_CLIENT_WAKE		0x80	/* for board_info; true iff can wake */
+#define I2C_CLIENT_AUTO		0x100	/* for board_info; auto-detected */
 #define I2C_CLIENT_SCCB		0x9000	/* Use Omnivision SCCB protocol */
 					/* Must match I2C_M_STOP|IGNORE_NAK */
 
-- 
2.47.0

[PATCH v3 3/4] i2c: Replace list-based mechanism for handling userspace-created clients

[Heiner Kallweit]

Similar to the list of auto-detected clients, we can also replace the
list of userspace-created clients with flagging such client devices.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v3:
- protect client unregistration with core_lock mutex
---
 drivers/i2c/i2c-core-base.c | 61 +++++++++++++++----------------------
 include/linux/i2c.h         |  1 +
 2 files changed, 25 insertions(+), 37 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 42c62839d..15397cfaa 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1293,14 +1293,12 @@ new_device_store(struct device *dev, struct device_attribute *attr,
 		info.flags |= I2C_CLIENT_SLAVE;
 	}
 
+	info.flags |= I2C_CLIENT_USER;
+
 	client = i2c_new_client_device(adap, &info);
 	if (IS_ERR(client))
 		return PTR_ERR(client);
 
-	/* Keep track of the added device */
-	mutex_lock(&adap->userspace_clients_lock);
-	list_add_tail(&client->detected, &adap->userspace_clients);
-	mutex_unlock(&adap->userspace_clients_lock);
 	dev_info(dev, "%s: Instantiated device %s at 0x%02hx\n", "new_device",
 		 info.type, info.addr);
 
@@ -1308,6 +1306,15 @@ new_device_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_WO(new_device);
 
+static int __i2c_find_user_addr(struct device *dev, void *addrp)
+{
+	struct i2c_client *client = i2c_verify_client(dev);
+	unsigned short addr = *(unsigned short *)addrp;
+
+	return client && client->flags & I2C_CLIENT_USER &&
+	       i2c_encode_flags_to_addr(client) == addr;
+}
+
 /*
  * And of course let the users delete the devices they instantiated, if
  * they got it wrong. This interface can only be used to delete devices
@@ -1322,7 +1329,8 @@ delete_device_store(struct device *dev, struct device_attribute *attr,
 		    const char *buf, size_t count)
 {
 	struct i2c_adapter *adap = to_i2c_adapter(dev);
-	struct i2c_client *client, *next;
+	struct i2c_client *client;
+	struct device *child_dev;
 	unsigned short addr;
 	char end;
 	int res;
@@ -1338,28 +1346,20 @@ delete_device_store(struct device *dev, struct device_attribute *attr,
 		return -EINVAL;
 	}
 
+	mutex_lock (&core_lock);
 	/* Make sure the device was added through sysfs */
-	res = -ENOENT;
-	mutex_lock_nested(&adap->userspace_clients_lock,
-			  i2c_adapter_depth(adap));
-	list_for_each_entry_safe(client, next, &adap->userspace_clients,
-				 detected) {
-		if (i2c_encode_flags_to_addr(client) == addr) {
-			dev_info(dev, "%s: Deleting device %s at 0x%02hx\n",
-				 "delete_device", client->name, client->addr);
-
-			list_del(&client->detected);
-			i2c_unregister_device(client);
-			res = count;
-			break;
-		}
+	child_dev = device_find_child(&adap->dev, &addr, __i2c_find_user_addr);
+	if (!child_dev) {
+		mutex_unlock (&core_lock);
+		dev_err(dev, "Can't find userspace-created device at %#x\n", addr);
+		return -ENOENT;
 	}
-	mutex_unlock(&adap->userspace_clients_lock);
+	client = i2c_verify_client(child_dev);
+	i2c_unregister_device(client);
+	put_device(child_dev);
+	mutex_unlock (&core_lock);
 
-	if (res < 0)
-		dev_err(dev, "%s: Can't find device in list\n",
-			"delete_device");
-	return res;
+	return count;
 }
 static DEVICE_ATTR_IGNORE_LOCKDEP(delete_device, S_IWUSR, NULL,
 				  delete_device_store);
@@ -1722,7 +1722,6 @@ static int __unregister_dummy(struct device *dev, void *dummy)
 void i2c_del_adapter(struct i2c_adapter *adap)
 {
 	struct i2c_adapter *found;
-	struct i2c_client *client, *next;
 
 	/* First make sure that this adapter was ever added */
 	mutex_lock(&core_lock);
@@ -1735,18 +1734,6 @@ void i2c_del_adapter(struct i2c_adapter *adap)
 
 	i2c_acpi_remove_space_handler(adap);
 
-	/* Remove devices instantiated from sysfs */
-	mutex_lock_nested(&adap->userspace_clients_lock,
-			  i2c_adapter_depth(adap));
-	list_for_each_entry_safe(client, next, &adap->userspace_clients,
-				 detected) {
-		dev_dbg(&adap->dev, "Removing %s at 0x%x\n", client->name,
-			client->addr);
-		list_del(&client->detected);
-		i2c_unregister_device(client);
-	}
-	mutex_unlock(&adap->userspace_clients_lock);
-
 	/* Detach any active clients. This can't fail, thus we do not
 	 * check the returned value. This is a two-pass process, because
 	 * we can't remove the dummy devices during the first pass: they
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index c4c8e841a..376136b18 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -333,6 +333,7 @@ struct i2c_client {
 #define I2C_CLIENT_HOST_NOTIFY	0x40	/* We want to use I2C host notify */
 #define I2C_CLIENT_WAKE		0x80	/* for board_info; true iff can wake */
 #define I2C_CLIENT_AUTO		0x100	/* for board_info; auto-detected */
+#define I2C_CLIENT_USER		0x200	/* for board_info; userspace-created */
 #define I2C_CLIENT_SCCB		0x9000	/* Use Omnivision SCCB protocol */
 					/* Must match I2C_M_STOP|IGNORE_NAK */
 
-- 
2.47.0

[PATCH v3 4/4] i2c: core: Remove obsolete members of i2c_adapter and i2c_client

[Heiner Kallweit]

After the lists of auto-detected and userspace-created clients have been
removed, we can remove now unused struct members.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/i2c-core-base.c | 2 --
 include/linux/i2c.h         | 6 ------
 2 files changed, 8 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 15397cfaa..32929ca96 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1530,8 +1530,6 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	adap->locked_flags = 0;
 	rt_mutex_init(&adap->bus_lock);
 	rt_mutex_init(&adap->mux_lock);
-	mutex_init(&adap->userspace_clients_lock);
-	INIT_LIST_HEAD(&adap->userspace_clients);
 
 	/* Set default timeout to 1 second if not already set */
 	if (adap->timeout == 0)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 376136b18..42de42807 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -313,8 +313,6 @@ struct i2c_driver {
  * @dev: Driver model device node for the slave.
  * @init_irq: IRQ that was set at initialization
  * @irq: indicates the IRQ generated by this device (if any)
- * @detected: member of an i2c_driver.clients list or i2c-core's
- *	userspace_devices list
  * @slave_cb: Callback when I2C slave mode of an adapter is used. The adapter
  *	calls it to pass on slave events to the slave driver.
  * @devres_group_id: id of the devres group that will be created for resources
@@ -345,7 +343,6 @@ struct i2c_client {
 	struct device dev;		/* the device structure		*/
 	int init_irq;			/* irq set at initialization	*/
 	int irq;			/* irq issued by device		*/
-	struct list_head detected;
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	i2c_slave_cb_t slave_cb;	/* callback for slave mode	*/
 #endif
@@ -751,9 +748,6 @@ struct i2c_adapter {
 	char name[48];
 	struct completion dev_released;
 
-	struct mutex userspace_clients_lock;
-	struct list_head userspace_clients;
-
 	struct i2c_bus_recovery_info *bus_recovery_info;
 	const struct i2c_adapter_quirks *quirks;
 
-- 
2.47.0

Re: [PATCH v3 0/4] i2c: Replace lists of special clients with flagging of such clients

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 462 bytes --]

Hi Heiner,

On Fri, Nov 01, 2024 at 11:05:53PM +0100, Heiner Kallweit wrote:

> So far lists are used to track special clients, i.e. auto-detected and
> userspace-created clients. The same functionality can be achieved much
> simpler by flagging such clients.

Sorry for not having the time to review it in time for 6.13. As we found
with the last version, this really needs in-depth thinking. We will try
again for 6.14.

Thanks and happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/4] ALSA: ppc: Remove i2c client removal hack

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

On Fri, Nov 01, 2024 at 11:07:14PM +0100, Heiner Kallweit wrote:
> The i2c_driver.clients list is internal to I2C core and is going
> to be removed.  No driver should access it. Unregister the
> i2c client explicitly before deleting the i2c driver.
> 
> Reviewed-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/4] i2c: Replace list-based mechanism for handling auto-detected clients

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 831 bytes --]

On Fri, Nov 01, 2024 at 11:09:51PM +0100, Heiner Kallweit wrote:
> So far a list is used to track auto-detected clients per driver.
> The same functionality can be achieved much simpler by flagging
> auto-detected clients.
> 
> Two notes regarding the usage of driver_for_each_device:
> In our case it can't fail, however the function is annotated __must_check.
> So a little workaround is needed to avoid a compiler warning.
> Then we may remove nodes from the list over which we iterate.
> This is safe, see the explanation at the beginning of lib/klist.c.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks! One minor edit:

> +#define I2C_CLIENT_AUTO		0x100	/* for board_info; auto-detected */

This is not for 'board_info'. I changed it to "client was
auto-detected".


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 3/4] i2c: Replace list-based mechanism for handling userspace-created clients

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1316 bytes --]

On Fri, Nov 01, 2024 at 11:11:39PM +0100, Heiner Kallweit wrote:
> Similar to the list of auto-detected clients, we can also replace the
> list of userspace-created clients with flagging such client devices.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks! Some edits:

> +	mutex_lock (&core_lock);

Superfluous space before parens. Fixed it and the other two occasions.

> +	child_dev = device_find_child(&adap->dev, &addr, __i2c_find_user_addr);
> +	if (!child_dev) {
> +		mutex_unlock (&core_lock);
> +		dev_err(dev, "Can't find userspace-created device at %#x\n", addr);
> +		return -ENOENT;
>  	}
> -	mutex_unlock(&adap->userspace_clients_lock);
> +	client = i2c_verify_client(child_dev);
> +	i2c_unregister_device(client);
> +	put_device(child_dev);
> +	mutex_unlock (&core_lock);

With locks, I really prefer to have a single exit point. Reordered the
code to:

	if (child_dev) {
		unreg
		put
	} else {
		dev_err
		count = -ENOENT
	}

and got rid of the 'client' variable while here which was only used
once.

> +#define I2C_CLIENT_USER		0x200	/* for board_info; userspace-created */

Like in the previous patch, I fixed the flag desc a little.

You might want to check my for-mergewindow branch to double check my
edits.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 4/4] i2c: core: Remove obsolete members of i2c_adapter and i2c_client

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 314 bytes --]

On Fri, Nov 01, 2024 at 11:12:29PM +0100, Heiner Kallweit wrote:
> After the lists of auto-detected and userspace-created clients have been
> removed, we can remove now unused struct members.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next by folding into patch 3, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 0/4] i2c: Replace lists of special clients with flagging of such clients

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]


> Sorry for not having the time to review it in time for 6.13. As we found
> with the last version, this really needs in-depth thinking. We will try
> again for 6.14.

So, I reviewed the code two more times and added hacks to test
auto-detected devices. I think we can let it out now into -next for
further testing.

Thanks for your patience and the continued work here! I really like how
this simplifies the I2C core. Let's hope we did not overlook something.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/4] i2c: Replace list-based mechanism for handling auto-detected clients

[Herve Codina]

Hi,

Got a deadlock issue with this patch in v6.14-rc1.

On Fri, 1 Nov 2024 23:09:51 +0100
Heiner Kallweit <hkallweit1@gmail.com> wrote:

> So far a list is used to track auto-detected clients per driver.
> The same functionality can be achieved much simpler by flagging
> auto-detected clients.
> 
> Two notes regarding the usage of driver_for_each_device:
> In our case it can't fail, however the function is annotated __must_check.
> So a little workaround is needed to avoid a compiler warning.
> Then we may remove nodes from the list over which we iterate.
> This is safe, see the explanation at the beginning of lib/klist.c.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v3:
> - protect client removal with core_lock mutex
> ---
>  drivers/i2c/i2c-core-base.c | 52 ++++++++++++-------------------------
>  include/linux/i2c.h         |  3 +--
>  2 files changed, 17 insertions(+), 38 deletions(-)
> 
...

> @@ -1780,8 +1752,10 @@ void i2c_del_adapter(struct i2c_adapter *adap)
>  	 * we can't remove the dummy devices during the first pass: they
>  	 * could have been instantiated by real devices wishing to clean
>  	 * them up properly, so we give them a chance to do that first. */
> +	mutex_lock(&core_lock);
>  	device_for_each_child(&adap->dev, NULL, __unregister_client);
>  	device_for_each_child(&adap->dev, NULL, __unregister_dummy);
> +	mutex_unlock(&core_lock);
>  

Calling __unregister_client() with core_lock mutex held leads to a deadlock
in my case:

    # echo 30a40000.i2c > /sys/bus/platform/drivers/imx-i2c/unbind
    [  242.928264] 
    [  242.929779] ============================================
    [  242.935092] WARNING: possible recursive locking detected
    [  242.940406] 6.14.0-rc1+ #22 Not tainted
    [  242.944245] --------------------------------------------
    [  242.949556] sh/299 is trying to acquire lock:
    [  242.953915] ffff8000818b82e0 (core_lock){+.+.}-{4:4}, at: i2c_del_adapter+0x44/0x1b0
    [  242.961689] 
    [  242.961689] but task is already holding lock:
    [  242.967524] ffff8000818b82e0 (core_lock){+.+.}-{4:4}, at: i2c_del_adapter+0xa0/0x1b0
    [  242.975285] 
    [  242.975285] other info that might help us debug this:
    [  242.981814]  Possible unsafe locking scenario:
    [  242.981814] 
    [  242.987732]        CPU0
    [  242.990179]        ----
    [  242.992625]   lock(core_lock);
    [  242.995686]   lock(core_lock);
    [  242.998748] 
    [  242.998748]  *** DEADLOCK ***
    [  242.998748] 
    [  243.004666]  May be due to missing lock nesting notation
    [  243.004666] 
    [  243.011455] 5 locks held by sh/299:
    [  243.014946]  #0: ffff000079a533f0 (sb_writers#6){.+.+}-{0:0}, at: vfs_write+0x1c4/0x398
    [  243.022976]  #1: ffff000005c29088 (&of->mutex#2){+.+.}-{4:4}, at: kernfs_fop_write_iter+0xf8/0x1c8
    [  243.031962]  #2: ffff000000c240f8 (&dev->mutex){....}-{4:4}, at: device_release_driver_internal+0x48/0x250
    [  243.041645]  #3: ffff8000818b82e0 (core_lock){+.+.}-{4:4}, at: i2c_del_adapter+0xa0/0x1b0
    [  243.049845]  #4: ffff000079f24908 (&dev->mutex){....}-{4:4}, at: device_release_driver_internal+0x48/0x250
    [  243.059522] 
    [  243.059522] stack backtrace:
    [  243.063883] CPU: 2 UID: 0 PID: 299 Comm: sh Not tainted 6.14.0-rc1+ #22
    [  243.070502] Hardware name: GE HealthCare Supernova Patient Hub v1 (DT)
    [  243.077032] Call trace:
    [  243.079481]  show_stack+0x20/0x38 (C)
    [  243.083152]  dump_stack_lvl+0x90/0xd0
    [  243.086819]  dump_stack+0x18/0x28
    [  243.090140]  print_deadlock_bug+0x260/0x350
    [  243.094332]  __lock_acquire+0x113c/0x2180
    [  243.098346]  lock_acquire+0x1c4/0x350
    [  243.102015]  __mutex_lock+0x9c/0x500
    [  243.105599]  mutex_lock_nested+0x2c/0x40
    [  243.109528]  i2c_del_adapter+0x44/0x1b0
    [  243.113371]  i2c_mux_del_adapters+0xa0/0x100
    [  243.117649]  pca954x_cleanup+0x98/0xd0
    [  243.121406]  pca954x_remove+0x38/0x50
    [  243.125078]  i2c_device_remove+0x34/0xb8
    [  243.129007]  device_remove+0x54/0x90
    [  243.132590]  device_release_driver_internal+0x1e8/0x250
    [  243.137824]  device_release_driver+0x20/0x38
    [  243.142101]  bus_remove_device+0xd4/0x120
    [  243.146116]  device_del+0x14c/0x410
    [  243.149612]  device_unregister+0x20/0x48
    [  243.153540]  i2c_unregister_device.part.0+0x50/0x88
    [  243.158427]  __unregister_client+0x74/0x80
    [  243.162530]  device_for_each_child+0x68/0xc8
    [  243.166811]  i2c_del_adapter+0xb8/0x1b0
    [  243.170653]  i2c_imx_remove+0x4c/0x190
    [  243.174412]  platform_remove+0x30/0x58
    [  243.178167]  device_remove+0x54/0x90
    [  243.181751]  device_release_driver_internal+0x1e8/0x250
    [  243.186982]  device_driver_detach+0x20/0x38
    [  243.191172]  unbind_store+0xbc/0xc8
    ...

When I unbind the i2c SoC adapter driver, i2c_del_adapter() is indeed called
recursively. The first call is for the 30a40000.i2c SoC adapter and the
second one for an i2c mux connected on the i2c bus.

My device-tree looks like this:
	i2c@30a40000 {
		compatible = "fsl,imx8mp-i2c", "fsl,imx21-i2c";
		...
		i2c-mux@70 {
			compatible = "nxp,pca9543";
			...
			i2c@0 {
				...
				touchscreen@2a {
					compatible = "eeti,exc80h60";
					...
				};
			};
			
			i2c@1 {
				...
			};
		};
	};


Should the core_lock mutex be taken when both __unregister_client() and
__unregister_dummy() are called ?

Best regards,
Hervé Codina

Re: [PATCH v3 2/4] i2c: Replace list-based mechanism for handling auto-detected clients

[Heiner Kallweit]

On 04.02.2025 16:24, Herve Codina wrote:
> Hi,
> 
> Got a deadlock issue with this patch in v6.14-rc1.
> 
> On Fri, 1 Nov 2024 23:09:51 +0100
> Heiner Kallweit <hkallweit1@gmail.com> wrote:
> 
>> So far a list is used to track auto-detected clients per driver.
>> The same functionality can be achieved much simpler by flagging
>> auto-detected clients.
>>
>> Two notes regarding the usage of driver_for_each_device:
>> In our case it can't fail, however the function is annotated __must_check.
>> So a little workaround is needed to avoid a compiler warning.
>> Then we may remove nodes from the list over which we iterate.
>> This is safe, see the explanation at the beginning of lib/klist.c.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> v3:
>> - protect client removal with core_lock mutex
>> ---
>>  drivers/i2c/i2c-core-base.c | 52 ++++++++++++-------------------------
>>  include/linux/i2c.h         |  3 +--
>>  2 files changed, 17 insertions(+), 38 deletions(-)
>>
> ...
> 
>> @@ -1780,8 +1752,10 @@ void i2c_del_adapter(struct i2c_adapter *adap)
>>  	 * we can't remove the dummy devices during the first pass: they
>>  	 * could have been instantiated by real devices wishing to clean
>>  	 * them up properly, so we give them a chance to do that first. */
>> +	mutex_lock(&core_lock);
>>  	device_for_each_child(&adap->dev, NULL, __unregister_client);
>>  	device_for_each_child(&adap->dev, NULL, __unregister_dummy);
>> +	mutex_unlock(&core_lock);
>>  
> 
> Calling __unregister_client() with core_lock mutex held leads to a deadlock
> in my case:
> 
>     # echo 30a40000.i2c > /sys/bus/platform/drivers/imx-i2c/unbind
>     [  242.928264] 
>     [  242.929779] ============================================
>     [  242.935092] WARNING: possible recursive locking detected
>     [  242.940406] 6.14.0-rc1+ #22 Not tainted
>     [  242.944245] --------------------------------------------
>     [  242.949556] sh/299 is trying to acquire lock:
>     [  242.953915] ffff8000818b82e0 (core_lock){+.+.}-{4:4}, at: i2c_del_adapter+0x44/0x1b0
>     [  242.961689] 
>     [  242.961689] but task is already holding lock:
>     [  242.967524] ffff8000818b82e0 (core_lock){+.+.}-{4:4}, at: i2c_del_adapter+0xa0/0x1b0
>     [  242.975285] 
>     [  242.975285] other info that might help us debug this:
>     [  242.981814]  Possible unsafe locking scenario:
>     [  242.981814] 
>     [  242.987732]        CPU0
>     [  242.990179]        ----
>     [  242.992625]   lock(core_lock);
>     [  242.995686]   lock(core_lock);
>     [  242.998748] 
>     [  242.998748]  *** DEADLOCK ***
>     [  242.998748] 
>     [  243.004666]  May be due to missing lock nesting notation
>     [  243.004666] 
>     [  243.011455] 5 locks held by sh/299:
>     [  243.014946]  #0: ffff000079a533f0 (sb_writers#6){.+.+}-{0:0}, at: vfs_write+0x1c4/0x398
>     [  243.022976]  #1: ffff000005c29088 (&of->mutex#2){+.+.}-{4:4}, at: kernfs_fop_write_iter+0xf8/0x1c8
>     [  243.031962]  #2: ffff000000c240f8 (&dev->mutex){....}-{4:4}, at: device_release_driver_internal+0x48/0x250
>     [  243.041645]  #3: ffff8000818b82e0 (core_lock){+.+.}-{4:4}, at: i2c_del_adapter+0xa0/0x1b0
>     [  243.049845]  #4: ffff000079f24908 (&dev->mutex){....}-{4:4}, at: device_release_driver_internal+0x48/0x250
>     [  243.059522] 
>     [  243.059522] stack backtrace:
>     [  243.063883] CPU: 2 UID: 0 PID: 299 Comm: sh Not tainted 6.14.0-rc1+ #22
>     [  243.070502] Hardware name: GE HealthCare Supernova Patient Hub v1 (DT)
>     [  243.077032] Call trace:
>     [  243.079481]  show_stack+0x20/0x38 (C)
>     [  243.083152]  dump_stack_lvl+0x90/0xd0
>     [  243.086819]  dump_stack+0x18/0x28
>     [  243.090140]  print_deadlock_bug+0x260/0x350
>     [  243.094332]  __lock_acquire+0x113c/0x2180
>     [  243.098346]  lock_acquire+0x1c4/0x350
>     [  243.102015]  __mutex_lock+0x9c/0x500
>     [  243.105599]  mutex_lock_nested+0x2c/0x40
>     [  243.109528]  i2c_del_adapter+0x44/0x1b0
>     [  243.113371]  i2c_mux_del_adapters+0xa0/0x100
>     [  243.117649]  pca954x_cleanup+0x98/0xd0
>     [  243.121406]  pca954x_remove+0x38/0x50
>     [  243.125078]  i2c_device_remove+0x34/0xb8
>     [  243.129007]  device_remove+0x54/0x90
>     [  243.132590]  device_release_driver_internal+0x1e8/0x250
>     [  243.137824]  device_release_driver+0x20/0x38
>     [  243.142101]  bus_remove_device+0xd4/0x120
>     [  243.146116]  device_del+0x14c/0x410
>     [  243.149612]  device_unregister+0x20/0x48
>     [  243.153540]  i2c_unregister_device.part.0+0x50/0x88
>     [  243.158427]  __unregister_client+0x74/0x80
>     [  243.162530]  device_for_each_child+0x68/0xc8
>     [  243.166811]  i2c_del_adapter+0xb8/0x1b0
>     [  243.170653]  i2c_imx_remove+0x4c/0x190
>     [  243.174412]  platform_remove+0x30/0x58
>     [  243.178167]  device_remove+0x54/0x90
>     [  243.181751]  device_release_driver_internal+0x1e8/0x250
>     [  243.186982]  device_driver_detach+0x20/0x38
>     [  243.191172]  unbind_store+0xbc/0xc8
>     ...
> 
> When I unbind the i2c SoC adapter driver, i2c_del_adapter() is indeed called
> recursively. The first call is for the 30a40000.i2c SoC adapter and the
> second one for an i2c mux connected on the i2c bus.
> 
> My device-tree looks like this:
> 	i2c@30a40000 {
> 		compatible = "fsl,imx8mp-i2c", "fsl,imx21-i2c";
> 		...
> 		i2c-mux@70 {
> 			compatible = "nxp,pca9543";
> 			...
> 			i2c@0 {
> 				...
> 				touchscreen@2a {
> 					compatible = "eeti,exc80h60";
> 					...
> 				};
> 			};
> 			
> 			i2c@1 {
> 				...
> 			};
> 		};
> 	};
> 
> 
> Should the core_lock mutex be taken when both __unregister_client() and
> __unregister_dummy() are called ?
> 

Thanks for the report! I just submitted a fix, for now as RFC.
If fixes the deadlock and uses a new approach to prevent the race
which caused us to acquire the core lock in few place.
Could you please re-test?

> Best regards,
> Hervé Codina

Heiner