Skip to contentSkip to conversation

i2c: core: adapter registration fixes

PATCH In review Johan Hovold 2 participants 23 messages May 11 – Jun 4

Subsystems (3)
Stable branch I2C subsystem host drivers I2C
lore
Share & tools
lore

Johan Hovold

author
#0 lore raw

v3 cover i2c: core: adapter registration fixes

This series fixes a number of bugs related to adapter registration found through inspection (when looking into addressing i2c lifetime issues).

Included are also two related cleanups.

Johan

Changes in v3:
- split NULL-deref fix from probe loop fix (2-3/10)
- add missing adapter name error handling (5/10)
- replace adapter in IDR also on registration failures (8/10)

Changes in v2:
- fix irq domain leak (new, flagged by Sashiko when reviewing v1)
- add adapter to IDR before registration as needed by i2c-dev
- honour devicetree alias also when using i2c_add_numbered_adapter()

Johan Hovold (10):
i2c: core: fix irq domain leak on adapter registration failure
i2c: core: fix hang on adapter registration failure
i2c: core: fix NULL-deref on adapter registration failure
i2c: core: fix adapter probe deferral loop
i2c: core: fix adapter debugfs creation
i2c: core: disable runtime PM on adapter registration failure
i2c: core: fix adapter registration race
i2c: core: fix adapter deregistration race
i2c: core: clean up bus id allocation
i2c: core: clean up adapter registration error label

 drivers/i2c/i2c-core-base.c | 173 +++++++++++++++++++++---------------
 1 file changed, 100 insertions(+), 73 deletions(-)
Signature · 1 line
2.53.0
author
#1 lore raw

PATCH v3 1/10 i2c: core: fix irq domain leak on adapter registration failure

Make sure to tear down the host notify irq domain on adapter registration failure to avoid leaking it.

This issue was flagged by Sashiko when reviewing another adapter registration fix.

Fixes 4d5538f5882a ("i2c: use an IRQ to report Host Notify events, not alert")
Cc [email protected] # 4.10
Cc Benjamin Tissoires <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/i2c/i2c-core-base.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/i2c/i2c-core-base.c +3 -1
index 9c46147e3506..abe8341c1d6e 100644
@@ -1574,7 +1574,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	if (res) {
 		pr_err("adapter '%s': can't register device (%d)\n", adap->name, res);
 		put_device(&adap->dev);
-		goto out_list;
+		goto err_remove_irq_domain;
 	}
 
 	adap->debugfs = debugfs_create_dir(dev_name(&adap->dev), i2c_debugfs_root);

@@ -1609,6 +1609,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	init_completion(&adap->dev_released);
 	device_unregister(&adap->dev);
 	wait_for_completion(&adap->dev_released);
+err_remove_irq_domain:
+	i2c_host_notify_irq_teardown(adap);
 out_list:
 	mutex_lock(&core_lock);
 	idr_remove(&i2c_adapter_idr, adap->nr);
Signature · 1 line
2.53.0
Reviewed-by on v3 1/10
#2 lore raw

Re: [PATCH v3 01/10] i2c: core: fix irq domain leak on adapter registration failure

Quoted 10 lines · Johan Hovold

Make sure to tear down the host notify irq domain on adapter registration failure to avoid leaking it.

This issue was flagged by Sashiko when reviewing another adapter registration fix.

Fixes 4d5538f5882a ("i2c: use an IRQ to report Host Notify events, not alert")
Cc [email protected] # 4.10
Cc Benjamin Tissoires <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
Reviewed-by Wolfram Sang <[email protected]>
author
#3 lore raw

PATCH v3 2/10 i2c: core: fix hang on adapter registration failure

In reply to Johan Hovold

Clients may be registered from bus notifier callbacks when the adapter is registered. On a subsequent error during registration, the adapter references taken by such clients prevent the wait for the references to be released from ever completing.

Fix this by refactoring client deregistration and deregistering also on late adapter registration failures.

Fixes f8756c67b3de ("i2c: core: call of_i2c_setup_smbus_alert in i2c_register_adapter")
Cc [email protected] # 4.15
Cc Phil Reid <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/i2c/i2c-core-base.c | 49 ++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 20 deletions(-)
drivers/i2c/i2c-core-base.c +29 -20
index abe8341c1d6e..e42851a10098 100644
@@ -63,6 +63,7 @@
 static DEFINE_MUTEX(core_lock);
 static DEFINE_IDR(i2c_adapter_idr);
 
+static void i2c_deregister_clients(struct i2c_adapter *adap);
 static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver);
 
 static DEFINE_STATIC_KEY_FALSE(i2c_trace_msg_key);

@@ -1605,6 +1606,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	return 0;
 
 out_reg:
+	i2c_deregister_clients(adap);
 	debugfs_remove_recursive(adap->debugfs);
 	init_completion(&adap->dev_released);
 	device_unregister(&adap->dev);

@@ -1746,29 +1748,10 @@ static int __process_removed_adapter(struct device_driver *d, void *data)
 	return 0;
 }
 
-/**
- * i2c_del_adapter - unregister I2C adapter
- * @adap: the adapter being unregistered
- * Context: can sleep
- *
- * This unregisters an I2C adapter which was previously registered
- * by @i2c_add_adapter or @i2c_add_numbered_adapter.
- */
-void i2c_del_adapter(struct i2c_adapter *adap)
+static void i2c_deregister_clients(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);
-	found = idr_find(&i2c_adapter_idr, adap->nr);
-	mutex_unlock(&core_lock);
-	if (found != adap) {
-		pr_debug("attempting to delete unregistered adapter [%s]\n", adap->name);
-		return;
-	}
-
-	i2c_acpi_remove_space_handler(adap);
 	/* Tell drivers about this removal */
 	mutex_lock(&core_lock);
 	bus_for_each_drv(&i2c_bus_type, NULL, adap,

@@ -1794,6 +1777,32 @@ void i2c_del_adapter(struct i2c_adapter *adap)
 	 * them up properly, so we give them a chance to do that first. */
 	device_for_each_child(&adap->dev, NULL, __unregister_client);
 	device_for_each_child(&adap->dev, NULL, __unregister_dummy);
+}
+
+/**
+ * i2c_del_adapter - unregister I2C adapter
+ * @adap: the adapter being unregistered
+ * Context: can sleep
+ *
+ * This unregisters an I2C adapter which was previously registered
+ * by @i2c_add_adapter or @i2c_add_numbered_adapter.
+ */
+void i2c_del_adapter(struct i2c_adapter *adap)
+{
+	struct i2c_adapter *found;
+
+	/* First make sure that this adapter was ever added */
+	mutex_lock(&core_lock);
+	found = idr_find(&i2c_adapter_idr, adap->nr);
+	mutex_unlock(&core_lock);
+	if (found != adap) {
+		pr_debug("attempting to delete unregistered adapter [%s]\n", adap->name);
+		return;
+	}
+
+	i2c_acpi_remove_space_handler(adap);
+
+	i2c_deregister_clients(adap);
 
 	/* device name is gone after device_unregister */
 	dev_dbg(&adap->dev, "adapter [%s] unregistered\n", adap->name);
Signature · 1 line
2.53.0
Reviewed-by on v3 2/10
#4 lore raw

Re: [PATCH v3 02/10] i2c: core: fix hang on adapter registration failure

Quoted 5 lines · Johan Hovold

Clients may be registered from bus notifier callbacks when the adapter is registered. On a subsequent error during registration, the adapter references taken by such clients prevent the wait for the references to be released from ever completing.

Quoted 7 lines

Fix this by refactoring client deregistration and deregistering also on late adapter registration failures.

Fixes f8756c67b3de ("i2c: core: call of_i2c_setup_smbus_alert in i2c_register_adapter")
Cc [email protected] # 4.15
Cc Phil Reid <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
Reviewed-by Wolfram Sang <[email protected]>

Until tomorrow noon latest, I will have all patches reviewed.

author
#5 lore raw

PATCH v3 3/10 i2c: core: fix NULL-deref on adapter registration failure

In reply to Johan Hovold

If adapter registration ever fails the release callback would trigger a NULL-pointer dereference as the completion struct has not been initialised.

Note that before the offending commit this would instead have resulted in a minor memory leak of the adapter name.

Fixes 3f8c4f5e9a57 ("i2c: core: fix reference leak in i2c_register_adapter()")
Cc [email protected]
Cc Joe Hattori <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/i2c/i2c-core-base.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/i2c/i2c-core-base.c +5 -4
index e42851a10098..fa9db415e219 100644
@@ -1574,8 +1574,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	res = device_add(&adap->dev);
 	if (res) {
 		pr_err("adapter '%s': can't register device (%d)\n", adap->name, res);
-		put_device(&adap->dev);
-		goto err_remove_irq_domain;
+		goto err_put_adap;
 	}
 
 	adap->debugfs = debugfs_create_dir(dev_name(&adap->dev), i2c_debugfs_root);

@@ -1608,10 +1607,12 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 out_reg:
 	i2c_deregister_clients(adap);
 	debugfs_remove_recursive(adap->debugfs);
+	device_del(&adap->dev);
+err_put_adap:
 	init_completion(&adap->dev_released);
-	device_unregister(&adap->dev);
+	put_device(&adap->dev);
 	wait_for_completion(&adap->dev_released);
-err_remove_irq_domain:
+
 	i2c_host_notify_irq_teardown(adap);
 out_list:
 	mutex_lock(&core_lock);
Signature · 1 line
2.53.0
Reviewed-by on v3 3/10
#6 lore raw

Re: [PATCH v3 03/10] i2c: core: fix NULL-deref on adapter registration failure

Quoted 11 lines · Johan Hovold

If adapter registration ever fails the release callback would trigger a NULL-pointer dereference as the completion struct has not been initialised.

Note that before the offending commit this would instead have resulted in a minor memory leak of the adapter name.

Fixes 3f8c4f5e9a57 ("i2c: core: fix reference leak in i2c_register_adapter()")
Cc [email protected]
Cc Joe Hattori <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
Reviewed-by Wolfram Sang <[email protected]>
author
#7 lore raw

PATCH v3 4/10 i2c: core: fix adapter probe deferral loop

In reply to Johan Hovold

Drivers must not probe defer after having registered devices as that will trigger a probe loop if the devices bind to a driver (cf. commit fbc35b45f9f6 ("Add documentation on meaning of -EPROBE_DEFER")).

Move the recovery initialisation, where the GPIO lookup may fail, before registering the adapter to prevent this.

Fixes 75820314de26 ("i2c: core: add generic I2C GPIO recovery")
Cc [email protected] # 5.9
Cc Codrin Ciubotariu <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/i2c/i2c-core-base.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/i2c/i2c-core-base.c +4 -4
index fa9db415e219..1caaa3b3ee10 100644
@@ -1562,6 +1562,10 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	adap->dev.type = &i2c_adapter_type;
 	device_initialize(&adap->dev);
 
+	res = i2c_init_recovery(adap);
+	if (res == -EPROBE_DEFER)
+		goto err_put_adap;
+
 	/*
 	 * This adapter can be used as a parent immediately after device_add(),
 	 * setup runtime-pm (especially ignore-children) before hand.

@@ -1583,10 +1587,6 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	if (res)
 		goto out_reg;
 
-	res = i2c_init_recovery(adap);
-	if (res == -EPROBE_DEFER)
-		goto out_reg;
-
 	dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name);
 
 	/* create pre-declared device nodes */
Signature · 1 line
2.53.0
Reviewed-by on v3 4/10
#8 lore raw

Re: [PATCH v3 04/10] i2c: core: fix adapter probe deferral loop

Quoted 11 lines · Johan Hovold

Drivers must not probe defer after having registered devices as that will trigger a probe loop if the devices bind to a driver (cf. commit fbc35b45f9f6 ("Add documentation on meaning of -EPROBE_DEFER")).

Move the recovery initialisation, where the GPIO lookup may fail, before registering the adapter to prevent this.

Fixes 75820314de26 ("i2c: core: add generic I2C GPIO recovery")
Cc [email protected] # 5.9
Cc Codrin Ciubotariu <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
Reviewed-by Wolfram Sang <[email protected]>
author
#9 lore raw

PATCH v3 5/10 i2c: core: fix adapter debugfs creation

In reply to Johan Hovold

Clients can be registered from bus notifier callbacks so the debugfs directory needs to be created before registering the adapter as clients use that directory as their debugfs parent.

Move debugfs creation before adapter registration to avoid having clients create their debugfs directories in the debugfs root (which is also more likely to fail due to name collisions).

Note that failure to allocate the adapter name must now be handled explicitly as debugfs_create_dir() cannot handle a NULL name (unlike device_add() which returns an error).

Fixes 73febd775bdb ("i2c: create debugfs entry per adapter")
Cc [email protected] # 6.8
Cc Wolfram Sang <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/i2c/i2c-core-base.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)
drivers/i2c/i2c-core-base.c +10 -6
index 1caaa3b3ee10..25d66de41287 100644
@@ -1557,7 +1557,10 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 		goto out_list;
 	}
 
-	dev_set_name(&adap->dev, "i2c-%d", adap->nr);
+	res = dev_set_name(&adap->dev, "i2c-%d", adap->nr);
+	if (res)
+		goto err_remove_irq_domain;
+
 	adap->dev.bus = &i2c_bus_type;
 	adap->dev.type = &i2c_adapter_type;
 	device_initialize(&adap->dev);

@@ -1575,14 +1578,14 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	pm_suspend_ignore_children(&adap->dev, true);
 	pm_runtime_enable(&adap->dev);
 
+	adap->debugfs = debugfs_create_dir(dev_name(&adap->dev), i2c_debugfs_root);
+
 	res = device_add(&adap->dev);
 	if (res) {
 		pr_err("adapter '%s': can't register device (%d)\n", adap->name, res);
-		goto err_put_adap;
+		goto err_remove_debugfs;
 	}
 
-	adap->debugfs = debugfs_create_dir(dev_name(&adap->dev), i2c_debugfs_root);
-
 	res = i2c_setup_smbus_alert(adap);
 	if (res)
 		goto out_reg;

@@ -1606,13 +1609,14 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 
 out_reg:
 	i2c_deregister_clients(adap);
-	debugfs_remove_recursive(adap->debugfs);
 	device_del(&adap->dev);
+err_remove_debugfs:
+	debugfs_remove_recursive(adap->debugfs);
 err_put_adap:
 	init_completion(&adap->dev_released);
 	put_device(&adap->dev);
 	wait_for_completion(&adap->dev_released);
-
+err_remove_irq_domain:
 	i2c_host_notify_irq_teardown(adap);
 out_list:
 	mutex_lock(&core_lock);
Signature · 1 line
2.53.0
Reviewed-by on v3 5/10
#10 lore raw

Re: [PATCH v3 05/10] i2c: core: fix adapter debugfs creation

Quoted 8 lines · Johan Hovold

Clients can be registered from bus notifier callbacks so the debugfs directory needs to be created before registering the adapter as clients use that directory as their debugfs parent.

Move debugfs creation before adapter registration to avoid having clients create their debugfs directories in the debugfs root (which is also more likely to fail due to name collisions).

Quoted 8 lines

Note that failure to allocate the adapter name must now be handled explicitly as debugfs_create_dir() cannot handle a NULL name (unlike device_add() which returns an error).

Fixes 73febd775bdb ("i2c: create debugfs entry per adapter")
Cc [email protected] # 6.8
Cc Wolfram Sang <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
Reviewed-by Wolfram Sang <[email protected]>
author
#11 lore raw

PATCH v3 6/10 i2c: core: disable runtime PM on adapter registration failure

In reply to Johan Hovold

Runtime PM is disabled by driver core when deregistering a device (and on registration failure) but add an explicit disable to balance the enable call when adapter registration fails for symmetry.

Fixes 23a698fe65ec ("i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs")
Cc Codrin Ciubotariu <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/i2c/i2c-core-base.c | 1 +
 1 file changed, 1 insertion(+)
drivers/i2c/i2c-core-base.c +1 -0
index 25d66de41287..fdf7d7d50f79 100644
@@ -1612,6 +1612,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	device_del(&adap->dev);
 err_remove_debugfs:
 	debugfs_remove_recursive(adap->debugfs);
+	pm_runtime_disable(&adap->dev);
 err_put_adap:
 	init_completion(&adap->dev_released);
 	put_device(&adap->dev);
Signature · 1 line
2.53.0
Reviewed-by on v3 6/10
#12 lore raw

Re: [PATCH v3 06/10] i2c: core: disable runtime PM on adapter registration failure

Quoted 7 lines · Johan Hovold

Runtime PM is disabled by driver core when deregistering a device (and on registration failure) but add an explicit disable to balance the enable call when adapter registration fails for symmetry.

Fixes 23a698fe65ec ("i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs")
Cc Codrin Ciubotariu <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
Reviewed-by Wolfram Sang <[email protected]>

Yes, we do the same when removing, too.

author
#13 lore raw

PATCH v3 7/10 i2c: core: fix adapter registration race

In reply to Johan Hovold

Adapters can be looked up based on their id using i2c_get_adapter() which takes a reference to the embedded struct device.

Make sure that the adapter (including its struct device) has been initialised before adding it to the IDR to avoid accessing uninitialised data which could, for example, lead to NULL-pointer dereferences or use-after-free.

Note that the i2c-dev chardev, which is registered from a bus notifier, currently uses i2c_get_adapter() so the adapter needs to be added to the IDR before registration.

Fixes 6e13e6418418 ("i2c: Add i2c_add_numbered_adapter()")
Cc [email protected] # 2.6.22
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/i2c/i2c-core-base.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/i2c/i2c-core-base.c +6 -2
index fdf7d7d50f79..01a984d3ca0e 100644
@@ -1580,6 +1580,10 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 
 	adap->debugfs = debugfs_create_dir(dev_name(&adap->dev), i2c_debugfs_root);
 
+	mutex_lock(&core_lock);
+	idr_replace(&i2c_adapter_idr, adap, adap->nr);
+	mutex_unlock(&core_lock);
+
 	res = device_add(&adap->dev);
 	if (res) {
 		pr_err("adapter '%s': can't register device (%d)\n", adap->name, res);

@@ -1638,7 +1642,7 @@ static int __i2c_add_numbered_adapter(struct i2c_adapter *adap)
 	int id;
 
 	mutex_lock(&core_lock);
-	id = idr_alloc(&i2c_adapter_idr, adap, adap->nr, adap->nr + 1, GFP_KERNEL);
+	id = idr_alloc(&i2c_adapter_idr, NULL, adap->nr, adap->nr + 1, GFP_KERNEL);
 	mutex_unlock(&core_lock);
 	if (WARN(id < 0, "couldn't get idr"))
 		return id == -ENOSPC ? -EBUSY : id;

@@ -1672,7 +1676,7 @@ int i2c_add_adapter(struct i2c_adapter *adapter)
 	}
 
 	mutex_lock(&core_lock);
-	id = idr_alloc(&i2c_adapter_idr, adapter,
+	id = idr_alloc(&i2c_adapter_idr, NULL,
 		       __i2c_first_dynamic_bus_num, 0, GFP_KERNEL);
 	mutex_unlock(&core_lock);
 	if (WARN(id < 0, "couldn't get idr"))
Signature · 1 line
2.53.0
Reviewed-by on v3 7/10
#14 lore raw

Re: [PATCH v3 07/10] i2c: core: fix adapter registration race

Quoted 8 lines · Johan Hovold

Adapters can be looked up based on their id using i2c_get_adapter() which takes a reference to the embedded struct device.

Make sure that the adapter (including its struct device) has been initialised before adding it to the IDR to avoid accessing uninitialised data which could, for example, lead to NULL-pointer dereferences or use-after-free.

Quoted 7 lines

Note that the i2c-dev chardev, which is registered from a bus notifier, currently uses i2c_get_adapter() so the adapter needs to be added to the IDR before registration.

Fixes 6e13e6418418 ("i2c: Add i2c_add_numbered_adapter()")
Cc [email protected] # 2.6.22
Signed-off-by Johan Hovold <[email protected]>
Reviewed-by Wolfram Sang <[email protected]>
author
#15 lore raw

PATCH v3 8/10 i2c: core: fix adapter deregistration race

In reply to Johan Hovold

Adapters can be looked up by their id using i2c_get_adapter() which takes a reference to the embedded struct device.

Remove the adapter from the IDR before tearing it down during deregistration (and on registration failure) to make sure its resources are not accessed after having been freed (e.g. the device name).

Fixes 35fc37f81881 ("i2c: Limit core locking to the necessary sections")
Cc [email protected] # 2.6.31
Cc Jean Delvare <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/i2c/i2c-core-base.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/i2c/i2c-core-base.c +7 -2
index 01a984d3ca0e..38f425aecef8 100644
@@ -1587,7 +1587,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	res = device_add(&adap->dev);
 	if (res) {
 		pr_err("adapter '%s': can't register device (%d)\n", adap->name, res);
-		goto err_remove_debugfs;
+		goto err_replace_id;
 	}
 
 	res = i2c_setup_smbus_alert(adap);

@@ -1614,7 +1614,10 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 out_reg:
 	i2c_deregister_clients(adap);
 	device_del(&adap->dev);
-err_remove_debugfs:
+err_replace_id:
+	mutex_lock(&core_lock);
+	idr_replace(&i2c_adapter_idr, NULL, adap->nr);
+	mutex_unlock(&core_lock);
 	debugfs_remove_recursive(adap->debugfs);
 	pm_runtime_disable(&adap->dev);
 err_put_adap:

@@ -1804,6 +1807,8 @@ void i2c_del_adapter(struct i2c_adapter *adap)
 	/* First make sure that this adapter was ever added */
 	mutex_lock(&core_lock);
 	found = idr_find(&i2c_adapter_idr, adap->nr);
+	if (found == adap)
+		idr_replace(&i2c_adapter_idr, NULL, adap->nr);
 	mutex_unlock(&core_lock);
 	if (found != adap) {
 		pr_debug("attempting to delete unregistered adapter [%s]\n", adap->name);
Signature · 1 line
2.53.0
Reviewed-by on v3 8/10
#16 lore raw

Re: [PATCH v3 08/10] i2c: core: fix adapter deregistration race

Quoted 11 lines · Johan Hovold

Adapters can be looked up by their id using i2c_get_adapter() which takes a reference to the embedded struct device.

Remove the adapter from the IDR before tearing it down during deregistration (and on registration failure) to make sure its resources are not accessed after having been freed (e.g. the device name).

Fixes 35fc37f81881 ("i2c: Limit core locking to the necessary sections")
Cc [email protected] # 2.6.31
Cc Jean Delvare <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
Reviewed-by Wolfram Sang <[email protected]>
author
#17 lore raw

PATCH v3 9/10 i2c: core: clean up bus id allocation

In reply to Johan Hovold

Clean up bus id allocation by using a common helper and deferring it until it is needed during adapter registration.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/i2c/i2c-core-base.c | 89 +++++++++++++++++++------------------
 1 file changed, 45 insertions(+), 44 deletions(-)
drivers/i2c/i2c-core-base.c +45 -44
index 38f425aecef8..2cd384433d83 100644
@@ -1517,23 +1517,48 @@ int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr)
 }
 EXPORT_SYMBOL_GPL(i2c_handle_smbus_host_notify);
 
+static int i2c_allocate_adapter_id(struct i2c_adapter *adap)
+{
+	int id, start, end;
+
+	if (adap->nr == -1) {
+		start = __i2c_first_dynamic_bus_num;
+		end = 0;
+	} else {
+		start = adap->nr;
+		end = adap->nr + 1;
+	}
+
+	mutex_lock(&core_lock);
+	id = idr_alloc(&i2c_adapter_idr, NULL, start, end, GFP_KERNEL);
+	mutex_unlock(&core_lock);
+	if (id < 0) {
+		if (adap->nr != -1 && id == -ENOSPC)
+			id = -EBUSY;
+		pr_err("adapter '%s': failed to allocate id: %d\n", adap->name, id);
+		return id;
+	}
+
+	adap->nr = id;
+
+	return 0;
+}
+
 static int i2c_register_adapter(struct i2c_adapter *adap)
 {
-	int res = -EINVAL;
+	int res;
 
 	/* Can't register until after driver model init */
-	if (WARN_ON(!is_registered)) {
-		res = -EAGAIN;
-		goto out_list;
-	}
+	if (WARN_ON(!is_registered))
+		return -EAGAIN;
 
 	/* Sanity checks */
 	if (WARN(!adap->name[0], "i2c adapter has no name"))
-		goto out_list;
+		return -EINVAL;
 
 	if (!adap->algo) {
 		pr_err("adapter '%s': no algo supplied!\n", adap->name);
-		goto out_list;
+		return -EINVAL;
 	}
 
 	if (!adap->lock_ops)

@@ -1554,13 +1579,17 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	if (res) {
 		pr_err("adapter '%s': can't create Host Notify IRQs (%d)\n",
 		       adap->name, res);
-		goto out_list;
+		return res;
 	}
 
-	res = dev_set_name(&adap->dev, "i2c-%d", adap->nr);
+	res = i2c_allocate_adapter_id(adap);
 	if (res)
 		goto err_remove_irq_domain;
 
+	res = dev_set_name(&adap->dev, "i2c-%d", adap->nr);
+	if (res)
+		goto err_free_id;
+
 	adap->dev.bus = &i2c_bus_type;
 	adap->dev.type = &i2c_adapter_type;
 	device_initialize(&adap->dev);

@@ -1624,33 +1653,14 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	init_completion(&adap->dev_released);
 	put_device(&adap->dev);
 	wait_for_completion(&adap->dev_released);
-err_remove_irq_domain:
-	i2c_host_notify_irq_teardown(adap);
-out_list:
+err_free_id:
 	mutex_lock(&core_lock);
 	idr_remove(&i2c_adapter_idr, adap->nr);
 	mutex_unlock(&core_lock);
-	return res;
-}
-
-/**
- * __i2c_add_numbered_adapter - i2c_add_numbered_adapter where nr is never -1
- * @adap: the adapter to register (with adap->nr initialized)
- * Context: can sleep
- *
- * See i2c_add_numbered_adapter() for details.
- */
-static int __i2c_add_numbered_adapter(struct i2c_adapter *adap)
-{
-	int id;
-
-	mutex_lock(&core_lock);
-	id = idr_alloc(&i2c_adapter_idr, NULL, adap->nr, adap->nr + 1, GFP_KERNEL);
-	mutex_unlock(&core_lock);
-	if (WARN(id < 0, "couldn't get idr"))
-		return id == -ENOSPC ? -EBUSY : id;
+err_remove_irq_domain:
+	i2c_host_notify_irq_teardown(adap);
 
-	return i2c_register_adapter(adap);
+	return res;
 }
 
 /**

@@ -1673,17 +1683,8 @@ int i2c_add_adapter(struct i2c_adapter *adapter)
 	int id;
 
 	id = of_alias_get_id(dev->of_node, "i2c");
-	if (id >= 0) {
-		adapter->nr = id;
-		return __i2c_add_numbered_adapter(adapter);
-	}
-
-	mutex_lock(&core_lock);
-	id = idr_alloc(&i2c_adapter_idr, NULL,
-		       __i2c_first_dynamic_bus_num, 0, GFP_KERNEL);
-	mutex_unlock(&core_lock);
-	if (WARN(id < 0, "couldn't get idr"))
-		return id;
+	if (id < 0)
+		id = -1;
 
 	adapter->nr = id;
 

@@ -1719,7 +1720,7 @@ int i2c_add_numbered_adapter(struct i2c_adapter *adap)
 	if (adap->nr == -1) /* -1 means dynamically assign bus id */
 		return i2c_add_adapter(adap);
 
-	return __i2c_add_numbered_adapter(adap);
+	return i2c_register_adapter(adap);
 }
 EXPORT_SYMBOL_GPL(i2c_add_numbered_adapter);
Signature · 1 line
2.53.0
author
#19 lore raw

PATCH v3 10/10 i2c: core: clean up adapter registration error label

In reply to Johan Hovold

Clean up the adapter registration error labels by making sure that also the last one is named after what it does.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/i2c/i2c-core-base.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/i2c/i2c-core-base.c +2 -2
index 2cd384433d83..5ddd985dfccb 100644
@@ -1621,7 +1621,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 
 	res = i2c_setup_smbus_alert(adap);
 	if (res)
-		goto out_reg;
+		goto err_deregister_clients;
 
 	dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name);
 

@@ -1640,7 +1640,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 
 	return 0;
 
-out_reg:
+err_deregister_clients:
 	i2c_deregister_clients(adap);
 	device_del(&adap->dev);
 err_replace_id:
Signature · 1 line
2.53.0
#21 lore raw
In reply to Johan Hovold
Quoted 2 lines · Johan Hovold

This series fixes a number of bugs related to adapter registration found through inspection (when looking into addressing i2c lifetime issues).

I intend to review the series. I wonder if it makes sense to wait until you don't see any valuable input from Sashiko anymore. Otherwise a reworked series X+1 will be sent while I am still looking at series X.

author
#22 lore raw
Quoted 7 lines · Wolfram Sang
Quoted 2 lines · Johan Hovold

This series fixes a number of bugs related to adapter registration found through inspection (when looking into addressing i2c lifetime issues).

I intend to review the series. I wonder if it makes sense to wait until you don't see any valuable input from Sashiko anymore. Otherwise a reworked series X+1 will be sent while I am still looking at series X.

You can always wait a few hours until the Sashiko output is available if you're worried about that, but I don't expect there to be any further revisions of this series.

Johan