On Thu, Feb 18, 2016 at 05:53:14PM +0200, Daniel Baluta wrote:
> From: Adriana Reus <[email protected]>
>
> This deadlock occurs if the accel/gyro and the sensor on the auxiliary
> I2C (in my setup it's an ak8975) are working at the same time.
>
> Scenario:
>
> T1 T2
> ==== ====
> inv_mpu6050_read_fifo aux sensor op (eg. ak8975_read_raw)
> | |
> mutex_lock(&indio_dev->mlock) i2c_transfer
> | |
> i2c transaction i2c adapter lock
> | |
> i2c adapter lock i2c_mux_master_xfer
> |
> inv_mpu6050_select_bypass
> |
> mutex_lock(&indio_dev->mlock)
>
> When we operate on an mpu sensor the order of locking is mpu lock
> followed by the i2c adapter lock. However, when we operate the auxiliary
> sensor the order of locking is the other way around.
>
> In order to avoid this enable the bypass mux bit once in the beginning
> and remove the select/deselect_bypass functions.
>
> This patch moves the bypass enabling in a separate function
> that is called once at probe and removes the functionality from
> inv_mpu_select/deselect_bypass.
>
> Another advantage of this approach is that power-wise the mpu chip isn't
> powered up at each auxiliary sensor i2c transaction so if only the
> compass is used this would be more power efficient.
I think the comments (power must be enabled on select) rendered this
solution not acceptable. What about using mutex_trylock in
inv_mpu6050_select_bypass() and returning -EAGAIN if the lock is already
held?
On Tue, Mar 1, 2016 at 10:50 PM, Wolfram Sang <[email protected]> wrote:
> On Thu, Feb 18, 2016 at 05:53:14PM +0200, Daniel Baluta wrote:
>> From: Adriana Reus <[email protected]>
>>
>> This deadlock occurs if the accel/gyro and the sensor on the auxiliary
>> I2C (in my setup it's an ak8975) are working at the same time.
>>
>> Scenario:
>>
>> T1 T2
>> ==== ====
>> inv_mpu6050_read_fifo aux sensor op (eg. ak8975_read_raw)
>> | |
>> mutex_lock(&indio_dev->mlock) i2c_transfer
>> | |
>> i2c transaction i2c adapter lock
>> | |
>> i2c adapter lock i2c_mux_master_xfer
>> |
>> inv_mpu6050_select_bypass
>> |
>> mutex_lock(&indio_dev->mlock)
>>
>> When we operate on an mpu sensor the order of locking is mpu lock
>> followed by the i2c adapter lock. However, when we operate the auxiliary
>> sensor the order of locking is the other way around.
>>
>> In order to avoid this enable the bypass mux bit once in the beginning
>> and remove the select/deselect_bypass functions.
>>
>> This patch moves the bypass enabling in a separate function
>> that is called once at probe and removes the functionality from
>> inv_mpu_select/deselect_bypass.
>>
>> Another advantage of this approach is that power-wise the mpu chip isn't
>> powered up at each auxiliary sensor i2c transaction so if only the
>> compass is used this would be more power efficient.
>
> I think the comments (power must be enabled on select) rendered this
> solution not acceptable. What about using mutex_trylock in
> inv_mpu6050_select_bypass() and returning -EAGAIN if the lock is already
> held?
Well, yes this would be a very good temporary solution. I'm afraid tough
that reading at high rates accel/gyro data will starve the aux sensor readings.
I looked into the I2C adapter / mux code but I got lost rapidly :). It
feels like
the natural solution would be for the I2C core to not hold the adapter lock
while doing transactions on the muxed child adapter.
Anyhow, sometimes starving is better than deadlocking so I will send a patch
with your suggestion.
thanks,
Daniel.
> I looked into the I2C adapter / mux code but I got lost rapidly :). It
> feels like the natural solution would be for the I2C core to not hold
> the adapter lock while doing transactions on the muxed child adapter.
The patch series I mentioned to you does exactly that. It locks only the
mux side of the muxed bus, not the whole parent adapter. It didn't work
for you because the mux driver maybe needed some adaptions as well?
However, I am still undecided if that series should go upstream because
it makes the mux code another magnitude more complex. And while this
seems to be the second issue which could be fixed by that series, both
issues are corner cases, so I am not sure it is worth the complexity.