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

[PATCH v2 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.

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 | 108 +++++++++++-------------------------
 include/linux/i2c.h         |  10 +---
 sound/ppc/keywest.c         |   7 +--
 3 files changed, 36 insertions(+), 89 deletions(-)

-- 
2.46.0

[PATCH v2 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.

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

[PATCH v2 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>
---
 drivers/i2c/i2c-core-base.c | 48 ++++++++++---------------------------
 include/linux/i2c.h         |  3 +--
 2 files changed, 13 insertions(+), 38 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 79292bb33..ea76007ea 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1662,23 +1662,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);
@@ -1694,12 +1677,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
@@ -1723,11 +1700,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,
@@ -1967,7 +1939,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.
@@ -1985,10 +1956,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;
 }
 
@@ -1999,7 +1973,10 @@ 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);
+	/* Satisfy __must_check, function can't fail */
+	if (driver_for_each_device(&driver->driver, NULL, NULL,
+				   __i2c_unregister_detected_client)) {
+	}
 
 	driver_unregister(&driver->driver);
 	pr_debug("driver [%s] unregistered\n", driver->driver.name);
@@ -2426,6 +2403,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
@@ -2452,9 +2430,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 377def497..910a9b259 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.46.0

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

[Heiner Kallweit]

Similarly 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>
---
 drivers/i2c/i2c-core-base.c | 58 ++++++++++++++-----------------------
 include/linux/i2c.h         |  1 +
 2 files changed, 22 insertions(+), 37 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index ea76007ea..15281c58a 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1265,14 +1265,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);
 
@@ -1280,6 +1278,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
@@ -1294,7 +1301,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;
@@ -1311,27 +1319,16 @@ delete_device_store(struct device *dev, struct device_attribute *attr,
 	}
 
 	/* 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) {
+		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);
 
-	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);
@@ -1688,7 +1685,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);
@@ -1701,18 +1697,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 910a9b259..ba2564fe4 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.46.0

[PATCH v2 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 15281c58a..e633ca215 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1499,8 +1499,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 ba2564fe4..ada90df88 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.46.0

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

[Takashi Iwai]

On Thu, 22 Aug 2024 21:47:18 +0200,
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.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

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

[Wolfram Sang]

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

On Thu, Aug 22, 2024 at 09:45:37PM +0200, 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.

This looks promising and I like the idea from a high-level perspective.
Need to dive into the details. However, I think this is 6.13 material. I
want to let it cook in linux-next for a full cycle.


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

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

[Heiner Kallweit]

On 01.09.2024 11:30, Wolfram Sang wrote:
> On Thu, Aug 22, 2024 at 09:45:37PM +0200, 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.
> 
> This looks promising and I like the idea from a high-level perspective.
> Need to dive into the details. However, I think this is 6.13 material. I
> want to let it cook in linux-next for a full cycle.
> 
Fine with me.

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

[Heiner Kallweit]

On 01.09.2024 11:30, Wolfram Sang wrote:
> On Thu, Aug 22, 2024 at 09:45:37PM +0200, 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.
> 
> This looks promising and I like the idea from a high-level perspective.
> Need to dive into the details. However, I think this is 6.13 material. I
> want to let it cook in linux-next for a full cycle.
> 
Now that 6.12-rc1 is out: Are you going to push this to linux-next?

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

[Wolfram Sang]

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


> Now that 6.12-rc1 is out: Are you going to push this to linux-next?

Yes, hopefully this week.


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

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

[Wolfram Sang]

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


> Yes, hopefully this week.

Reviewing and testing now. What tests did you run?


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

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

[Wolfram Sang]

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


> @@ -1723,11 +1700,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);

Isn't here some replacement needed to delete clients when the adapter
goes away? AFAICS they get only removed when their driver goes away.


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

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

[Wolfram Sang]

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


> -	/* 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);

Same question as previous patch: why no removal when adapter gets
deleted?


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

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

[Wolfram Sang]

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

On Thu, Aug 22, 2024 at 09:49:29PM +0200, 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>

Why don't you just fold this into patches 2 + 3?


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

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

[Wolfram Sang]

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


> Isn't here some replacement needed to delete clients when the adapter
> goes away?

No, Because the clients are covered by the generic cleanup now...


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

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

[Heiner Kallweit]

On 08.10.2024 11:00, Wolfram Sang wrote:
> On Thu, Aug 22, 2024 at 09:49:29PM +0200, 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>
> 
> Why don't you just fold this into patches 2 + 3?
> 
I'm always trying to reduce patches with functional changes to a minimum,
hoping this makes review easier. If changes allow for further cleanups,
I typically submit these cleanups as separate patch in the series.

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

[Heiner Kallweit]

On 08.10.2024 10:43, Wolfram Sang wrote:
> 
>> Yes, hopefully this week.
> 
> Reviewing and testing now. What tests did you run?
> 
Of course compile-tested the series. Functional test cases:
- Create device from userspace
- Delete device from userspace
- Delete adapter (by removing adapter driver module) and check that
  userspace-created client device has been auto-removed

Driver auto-detection I couldn't check due to missing hw.

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

[Heiner Kallweit]

On 30.09.2024 12:59, Wolfram Sang wrote:
> 
>> Now that 6.12-rc1 is out: Are you going to push this to linux-next?
> 
> Yes, hopefully this week.
> 
Now we're at rc5. Is it still something for 6.13 or better postpone
to have a full cycle in linux-next?

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

[Wolfram Sang]

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

Hi Heiner,

sorry for the slow response. I am on the road for two weeks now which
doesn't leave a lot of review time.

The good (or bad?) news is that I finally found why I had the feeling of
"something still missing" from this very interesting approach.

> -	/* 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);

You remove using the lock here...

> -	i2c_for_each_dev(driver, __process_removed_driver);
> +	/* Satisfy __must_check, function can't fail */
> +	if (driver_for_each_device(&driver->driver, NULL, NULL,

... and here, because i2c_for_each_dev() utilizes the lock as well. This
is, you open a race window for deleting clients via removing the driver
and removing the adapter at the "same" time.

The obvious solution is to use the lock also when removing clients in
i2c_del_adapter(). But this needs careful thinking about potential side
effects.

Makes sense so far?

All the best,

   Wolfram


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

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

[Heiner Kallweit]

On 01.11.2024 08:36, Wolfram Sang wrote:
> Hi Heiner,
> 
> sorry for the slow response. I am on the road for two weeks now which
> doesn't leave a lot of review time.
> 
> The good (or bad?) news is that I finally found why I had the feeling of
> "something still missing" from this very interesting approach.
> 
>> -	/* 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);
> 
> You remove using the lock here...
> 
>> -	i2c_for_each_dev(driver, __process_removed_driver);
>> +	/* Satisfy __must_check, function can't fail */
>> +	if (driver_for_each_device(&driver->driver, NULL, NULL,
> 
> ... and here, because i2c_for_each_dev() utilizes the lock as well. This
> is, you open a race window for deleting clients via removing the driver
> and removing the adapter at the "same" time.
> 
I think this is right. However we may have the same issue already,
w/o my patches. In i2c_del_adapter() the following isn't protected:
device_for_each_child(&adap->dev, NULL, __unregister_client);
So it may race with a parallel driver removal.

> The obvious solution is to use the lock also when removing clients in
> i2c_del_adapter(). But this needs careful thinking about potential side
> effects.
> 
I think this is needed, however I have to spend a few more thoughts on
whether it's sufficient.

> Makes sense so far?
> 
IMO, yes.

> All the best,
> 
>    Wolfram
> 
Heiner

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

[Wolfram Sang]

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


> I think this is right. However we may have the same issue already,
> w/o my patches. In i2c_del_adapter() the following isn't protected:
> device_for_each_child(&adap->dev, NULL, __unregister_client);

The drivers have been removed already at that time. By a block of code
which got removed in your patch:

1761         mutex_lock(&core_lock);
1762         bus_for_each_drv(&i2c_bus_type, NULL, adap,
1763                                __process_removed_adapter);
1764         mutex_unlock(&core_lock);


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

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

[Heiner Kallweit]

On 01.11.2024 08:36, Wolfram Sang wrote:
> Hi Heiner,
> 
> sorry for the slow response. I am on the road for two weeks now which
> doesn't leave a lot of review time.
> 
> The good (or bad?) news is that I finally found why I had the feeling of
> "something still missing" from this very interesting approach.
> 
>> -	/* 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);
> 
> You remove using the lock here...
> 
>> -	i2c_for_each_dev(driver, __process_removed_driver);
>> +	/* Satisfy __must_check, function can't fail */
>> +	if (driver_for_each_device(&driver->driver, NULL, NULL,
> 
> ... and here, because i2c_for_each_dev() utilizes the lock as well. This
> is, you open a race window for deleting clients via removing the driver
> and removing the adapter at the "same" time.
> 
> The obvious solution is to use the lock also when removing clients in
> i2c_del_adapter(). But this needs careful thinking about potential side
> effects.
> 
After thinking twice, adding the lock here, and protecting the call to
driver_for_each_device() with the core_lock, should be sufficient.
And I don't see any potential deadlock scenarios for now.

> Makes sense so far?
> 
> All the best,
> 
>    Wolfram
>