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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri,  4 Mar 2016 00:09:58 +0100
From:	Peter Rosin <peda@...ator.liu.se>
To:	Daniel Baluta <daniel.baluta@...el.com>
Cc:	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>,
	Peter Rosin <peda@...ator.liu.se>, linux-i2c@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [RFC PATCH 0/9] iio: Fix ABBA deadlock in inv-mpu6050

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.

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