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
