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 >
