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

[Heiner Kallweit <hkallweit1@xxxxxxxxx>]

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