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

[Heiner Kallweit <hkallweit1@gmail.com>]

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
>