Re: [Patch v3] i2c: Multiplexed I2C bus core support

[Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org>]

Jean Delvare said the following:
> Hi Michael,
> 
>>  drivers/i2c/i2c-mux.c       |  165
>> > +++++++++++++++++++++++++++++++++++++++++++
> As you can see, your e-mail client wrapped long lines again. I had to
> manually fix the patch so that I could apply it...
> 
I'm really sorry about that :-(
I've tested it by sending to an alternative address and it worked.
No idea where this wrap comes from.
...
>> @@ -656,9 +709,9 @@ i2c_sysfs_new_device(struct device *dev, struct
>> device_attribute *attr,
>>  		return -EINVAL;
>> 
>>  	/* Keep track of the added device */
>> -	i2c_lock_adapter(adap);
>> +	rt_mutex_lock(&adap->bus_lock);
>>  	list_add_tail(&client->detected, &adap->userspace_clients);
>> -	i2c_unlock_adapter(adap);
>> +	rt_mutex_unlock(&adap->bus_lock);
>>  	dev_info(dev, "%s: Instantiated device %s at 0x%02hx\n", "new_device",
>>  		 info.type, info.addr);
>> 
> 
> I expected us to just use i2c_lock/unlock_adapter() everywhere for
> simplicity. Now I have to agree that using the segment's mutex works
> too, as the operation is both local to the mutex and unrelated to
> the other use cases of i2c_lock/unlock_adapter(). But it becomes a
> little tricky, so it should be all documented clearly (which I will
> do, don't worry.)
> 
>> @@ -697,7 +750,7 @@ i2c_sysfs_delete_device(struct device *dev, struct
>> device_attribute *attr,
>> 
>>  	/* Make sure the device was added through sysfs */
>>  	res = -ENOENT;
>> -	i2c_lock_adapter(adap);
>> +	rt_mutex_lock(&adap->bus_lock);
>>  	list_for_each_entry_safe(client, next, &adap->userspace_clients,
>>  				 detected) {
>>  		if (client->addr == addr) {
>> @@ -710,7 +763,7 @@ i2c_sysfs_delete_device(struct device *dev, struct
>> device_attribute *attr,
>>  			break;
>>  		}
>>  	}
>> -	i2c_unlock_adapter(adap);
>> +	rt_mutex_unlock(&adap->bus_lock);
>> 
>>  	if (res < 0)
>>  		dev_err(dev, "%s: Can't find device in list\n",

In i2c_sysfs_delete_device you need a local lock, otherwise you'll get
a deadlock on removing sub-clients/tree. This in turn brings the local
lock to i2c_sysfs_new_device().

Thank you for your review.
ToDo after release: Inventing mux-adapter-name definition in mux-client
code instead of mux-code. You were right in your last review, this is
terribly necessary.

-- 
KR
Michael Lawnick