| Message ID | d52c2722-5f2e-4224-a8b3-7c9d0cb431d0@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i2c: Replace lists of special clients with flagging of such clients | expand |
> @@ -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.
> 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...
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
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
> 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);
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 >
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 */
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(-)