| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 4 Mar 2016 12:20:16 +0200
From: Daniel Baluta <daniel.baluta@...el.com>
To: Peter Rosin <peda@...ator.liu.se>
Cc: Daniel Baluta <daniel.baluta@...el.com>,
Peter Rosin <peda@...ntia.se>,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
Wolfram Sang <wsa@...-dreams.de>, Ge Gao <GGao@...ensense.com>,
Jonathan Cameron <jic23@...nel.org>, linux-i2c@...r.kernel.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>
Subject: Re: [RFC PATCH 0/9] iio: Fix ABBA deadlock in inv-mpu6050
On Fri, Mar 4, 2016 at 1:09 AM, Peter Rosin <peda@...ator.liu.se> wrote:
> From: Peter Rosin <peda@...ntia.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@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists