Re: [RFC PATCH 0/9] iio: Fix ABBA deadlock in inv-mpu6050

[Daniel Baluta <daniel.baluta-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Fri, Mar 4, 2016 at 1:09 AM, Peter Rosin <peda-SamgB31n2u5IcsJQ0EH25Q@public.gmane.org> wrote:
> From: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
>
> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html