Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1344372 > unrolled thread
| Started by | Daniel Baluta <daniel.baluta@intel.com> |
|---|---|
| First post | 2016-02-26 17:00 +0100 |
| Last post | 2016-03-04 11:30 +0100 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [RFC PATCH 0/9] iio: Fix ABBA deadlock in inv-mpu6050 Daniel Baluta <daniel.baluta@intel.com> - 2016-02-26 17:00 +0100
Re: [RFC PATCH 0/9] iio: Fix ABBA deadlock in inv-mpu6050 Peter Rosin <peda@lysator.liu.se> - 2016-03-04 00:20 +0100
Re: [RFC PATCH 0/9] iio: Fix ABBA deadlock in inv-mpu6050 Daniel Baluta <daniel.baluta@intel.com> - 2016-03-04 11:30 +0100
| From | Daniel Baluta <daniel.baluta@intel.com> |
|---|---|
| Date | 2016-02-26 17:00 +0100 |
| Subject | Re: [RFC PATCH 0/9] iio: Fix ABBA deadlock in inv-mpu6050 |
| Message-ID | <r6sYa-5ut-17@gated-at.bofh.it> |
On Mon, Feb 22, 2016 at 1:17 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> On Thu, Feb 18, 2016 at 05:53:05PM +0200, Daniel Baluta wrote:
>> Sending this as an RFC because I don't know if style fixes are appropriate
>> for this driver and also not sure if deadlock fix is the best solution.
>>
>> I2C people should only look at patches 8/9 and 9/9.
>>
>> The mpu6050 accel+gyro combo has an auxiliary I2C bus, allowing for
>> an auxiliary sensor to be connected (eg. a magnetometer).
>>
>> The mpu can either act as an I2C master (functionality not currently
>> implemented in the driver) or it can use a bypass multiplexer which
>> directly connects the auxiliary I2C bus pins to the main I2C bus pins [1]
>> Currently the driver implements the bypass mode via an I2C mux.
>>
>> This patchset fixes a deadlock in inv-mpu6050 IMU which happens when
>> magnetometer (on auxiliary I2C bus) runs in parallel with accel or gyro.
>>
>> First 7 patches do some cleanup in order to make INV MPU6050 code easier
>> to read.
>>
>> Patch number 8 allows passing NULL select callbacks to i2c_mux_master_xfer
>> and i2c_mux_smbus_xfer.
>>
>> Patch number 9 actually fixes the deadlock.
>
> We recently had a bigger patch series fixing locking problems related to
> muxes. I sadly didn't have the time to review it. Can you have a look if
> it helps your case?
>
> http://thread.gmane.org/gmane.linux.drivers.i2c/26169
Hi Wolfram,
Tested this and the deadlock is still there :(.
It can be easily reproduced with following patch which enlarges
the race window:
iff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index a3f5070..279d6e0 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -232,6 +232,7 @@ inv_mpu6050_read_raw(struct iio_dev *indio_dev,
ret = IIO_VAL_INT;
result = 0;
mutex_lock(&indio_dev->mlock);
+ msleep(12000);
if (!st->chip_config.enable) {
result = inv_mpu6050_set_power_itg(st, true);
if (result)
using the following commands:
$ cat /sys/bus/iio/devices/iio:device0/in_accel_x_raw
$ cat /sys/bus/iio/devices/iio:device1/in_magn_x_raw
Off topic: I'm very curious why I didn't get a lockdep splat while running this,
might be because adapter lock is a rt_mutex.
thanks,
Daniel.
[toc] | [next] | [standalone]
| From | Peter Rosin <peda@lysator.liu.se> |
|---|---|
| Date | 2016-03-04 00:20 +0100 |
| Message-ID | <r8KHg-66N-23@gated-at.bofh.it> |
| In reply to | #1344372 |
From: Peter Rosin <peda@axentia.se>
Hi Daniel,
Daniel Baluta wrote:
>On Mon, Feb 22, 2016 at 1:17 AM, Wolfram Sang wrote:
>> On Thu, Feb 18, 2016 at 05:53:05PM +0200, Daniel Baluta wrote:
>>> Sending this as an RFC because I don't know if style fixes are appropriate
>>> for this driver and also not sure if deadlock fix is the best solution.
*snip*
>> We recently had a bigger patch series fixing locking problems related to
>> muxes. I sadly didn't have the time to review it. Can you have a look if
>> it helps your case?
>>
>> http://thread.gmane.org/gmane.linux.drivers.i2c/26169
>
> Tested this and the deadlock is still there :(.
I assume that when you tested v3 of my series, you did nothing
to actually make use of the new stuff available in the mux-core?
If you didn't do anything to make use of the new stuff, the
driver should behave as before.
So, please try this patch on top of my recently posted v4 of the
"i2c mux cleanup and locking update" series [1]. I have not tested
this patch at all, but I have the feeling it could do the trick...
Cheers,
Peter
[1] https://marc.info/?l=linux-iio&m=145704469628330&w=3
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 642f22013d17..02b56e631973 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -79,35 +79,6 @@ int inv_mpu6050_write_reg(struct inv_mpu6050_state *st, int reg, u8 d)
return i2c_smbus_write_i2c_block_data(st->client, reg, 1, &d);
}
-/*
- * The i2c read/write needs to happen in unlocked mode. As the parent
- * adapter is common. If we use locked versions, it will fail as
- * the mux adapter will lock the parent i2c adapter, while calling
- * select/deselect functions.
- */
-static int inv_mpu6050_write_reg_unlocked(struct inv_mpu6050_state *st,
- u8 reg, u8 d)
-{
- int ret;
- u8 buf[2];
- struct i2c_msg msg[1] = {
- {
- .addr = st->client->addr,
- .flags = 0,
- .len = sizeof(buf),
- .buf = buf,
- }
- };
-
- buf[0] = reg;
- buf[1] = d;
- ret = __i2c_transfer(st->client->adapter, msg, 1);
- if (ret != 1)
- return ret;
-
- return 0;
-}
-
static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
{
struct iio_dev *indio_dev = i2c_mux_priv(muxc);
@@ -117,8 +88,7 @@ static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
/* Use the same mutex which was used everywhere to protect power-op */
mutex_lock(&indio_dev->mlock);
if (!st->powerup_count) {
- ret = inv_mpu6050_write_reg_unlocked(st, st->reg->pwr_mgmt_1,
- 0);
+ ret = inv_mpu6050_write_reg(st, st->reg->pwr_mgmt_1, 0);
if (ret)
goto write_error;
@@ -126,9 +96,9 @@ static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
}
if (!ret) {
st->powerup_count++;
- ret = inv_mpu6050_write_reg_unlocked(st, st->reg->int_pin_cfg,
- st->client->irq |
- INV_MPU6050_BIT_BYPASS_EN);
+ ret = inv_mpu6050_write_reg(st, st->reg->int_pin_cfg,
+ st->client->irq |
+ INV_MPU6050_BIT_BYPASS_EN);
}
write_error:
mutex_unlock(&indio_dev->mlock);
@@ -143,12 +113,11 @@ static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 chan_id)
mutex_lock(&indio_dev->mlock);
/* It doesn't really mattter, if any of the calls fails */
- inv_mpu6050_write_reg_unlocked(st, st->reg->int_pin_cfg,
- st->client->irq);
+ inv_mpu6050_write_reg(st, st->reg->int_pin_cfg, st->client->irq);
st->powerup_count--;
if (!st->powerup_count)
- inv_mpu6050_write_reg_unlocked(st, st->reg->pwr_mgmt_1,
- INV_MPU6050_BIT_SLEEP);
+ inv_mpu6050_write_reg(st, st->reg->pwr_mgmt_1,
+ INV_MPU6050_BIT_SLEEP);
mutex_unlock(&indio_dev->mlock);
return 0;
@@ -839,8 +808,8 @@ static int inv_mpu_probe(struct i2c_client *client,
goto out_remove_trigger;
}
- st->muxc = i2c_mux_one_adapter(client->adapter, &client->dev, 0, 0,
- 0, 0, 0,
+ st->muxc = i2c_mux_one_adapter(client->adapter, &client->dev, 0,
+ I2C_CONTROLLED_MUX, 0, 0, 0,
inv_mpu6050_select_bypass,
inv_mpu6050_deselect_bypass);
if (IS_ERR(st->muxc)) {
--
2.1.4
[toc] | [prev] | [next] | [standalone]
| From | Daniel Baluta <daniel.baluta@intel.com> |
|---|---|
| Date | 2016-03-04 11:30 +0100 |
| Message-ID | <r8V9E-5bS-15@gated-at.bofh.it> |
| In reply to | #1349682 |
On Fri, Mar 4, 2016 at 1:09 AM, Peter Rosin <peda@lysator.liu.se> wrote:
> From: Peter Rosin <peda@axentia.se>
>
> Hi Daniel,
>
> Daniel Baluta wrote:
>>On Mon, Feb 22, 2016 at 1:17 AM, Wolfram Sang wrote:
>>> On Thu, Feb 18, 2016 at 05:53:05PM +0200, Daniel Baluta wrote:
>>>> Sending this as an RFC because I don't know if style fixes are appropriate
>>>> for this driver and also not sure if deadlock fix is the best solution.
>
> *snip*
>
>>> We recently had a bigger patch series fixing locking problems related to
>>> muxes. I sadly didn't have the time to review it. Can you have a look if
>>> it helps your case?
>>>
>>> http://thread.gmane.org/gmane.linux.drivers.i2c/26169
>>
>> Tested this and the deadlock is still there :(.
>
> I assume that when you tested v3 of my series, you did nothing
> to actually make use of the new stuff available in the mux-core?
> If you didn't do anything to make use of the new stuff, the
> driver should behave as before.
You are right. I used v3 as it is.
>
> So, please try this patch on top of my recently posted v4 of the
> "i2c mux cleanup and locking update" series [1]. I have not tested
> this patch at all, but I have the feeling it could do the trick...
This patch fixes the problem! :P
> Cheers,
> Peter
>
> [1] https://marc.info/?l=linux-iio&m=145704469628330&w=3
>
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 642f22013d17..02b56e631973 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -79,35 +79,6 @@ int inv_mpu6050_write_reg(struct inv_mpu6050_state *st, int reg, u8 d)
> return i2c_smbus_write_i2c_block_data(st->client, reg, 1, &d);
> }
>
> -/*
> - * The i2c read/write needs to happen in unlocked mode. As the parent
> - * adapter is common. If we use locked versions, it will fail as
> - * the mux adapter will lock the parent i2c adapter, while calling
> - * select/deselect functions.
> - */
> -static int inv_mpu6050_write_reg_unlocked(struct inv_mpu6050_state *st,
> - u8 reg, u8 d)
> -{
> - int ret;
> - u8 buf[2];
> - struct i2c_msg msg[1] = {
> - {
> - .addr = st->client->addr,
> - .flags = 0,
> - .len = sizeof(buf),
> - .buf = buf,
> - }
> - };
> -
> - buf[0] = reg;
> - buf[1] = d;
> - ret = __i2c_transfer(st->client->adapter, msg, 1);
> - if (ret != 1)
> - return ret;
> -
> - return 0;
> -}
> -
> static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
> {
> struct iio_dev *indio_dev = i2c_mux_priv(muxc);
> @@ -117,8 +88,7 @@ static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
> /* Use the same mutex which was used everywhere to protect power-op */
> mutex_lock(&indio_dev->mlock);
> if (!st->powerup_count) {
> - ret = inv_mpu6050_write_reg_unlocked(st, st->reg->pwr_mgmt_1,
> - 0);
> + ret = inv_mpu6050_write_reg(st, st->reg->pwr_mgmt_1, 0);
> if (ret)
> goto write_error;
>
> @@ -126,9 +96,9 @@ static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
> }
> if (!ret) {
> st->powerup_count++;
> - ret = inv_mpu6050_write_reg_unlocked(st, st->reg->int_pin_cfg,
> - st->client->irq |
> - INV_MPU6050_BIT_BYPASS_EN);
> + ret = inv_mpu6050_write_reg(st, st->reg->int_pin_cfg,
> + st->client->irq |
> + INV_MPU6050_BIT_BYPASS_EN);
> }
> write_error:
> mutex_unlock(&indio_dev->mlock);
> @@ -143,12 +113,11 @@ static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 chan_id)
>
> mutex_lock(&indio_dev->mlock);
> /* It doesn't really mattter, if any of the calls fails */
> - inv_mpu6050_write_reg_unlocked(st, st->reg->int_pin_cfg,
> - st->client->irq);
> + inv_mpu6050_write_reg(st, st->reg->int_pin_cfg, st->client->irq);
> st->powerup_count--;
> if (!st->powerup_count)
> - inv_mpu6050_write_reg_unlocked(st, st->reg->pwr_mgmt_1,
> - INV_MPU6050_BIT_SLEEP);
> + inv_mpu6050_write_reg(st, st->reg->pwr_mgmt_1,
> + INV_MPU6050_BIT_SLEEP);
> mutex_unlock(&indio_dev->mlock);
>
> return 0;
> @@ -839,8 +808,8 @@ static int inv_mpu_probe(struct i2c_client *client,
> goto out_remove_trigger;
> }
>
> - st->muxc = i2c_mux_one_adapter(client->adapter, &client->dev, 0, 0,
> - 0, 0, 0,
> + st->muxc = i2c_mux_one_adapter(client->adapter, &client->dev, 0,
> + I2C_CONTROLLED_MUX, 0, 0, 0,
> inv_mpu6050_select_bypass,
> inv_mpu6050_deselect_bypass);
> if (IS_ERR(st->muxc)) {
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web