| 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
| ||
|
Message-ID: <CAEnQRZBEUC4DUmZci0MrXWzEBFXDR0bLN_taDuWR+mt6LMe-5w@mail.gmail.com>
Date: Mon, 18 Apr 2016 10:37:44 +0300
From: Daniel Baluta <daniel.baluta@...el.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Peter Rosin <peda@...ator.liu.se>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Peter Rosin <peda@...ntia.se>,
Wolfram Sang <wsa@...-dreams.de>,
Jonathan Corbet <corbet@....net>,
Peter Korsgaard <peter.korsgaard@...co.com>,
Guenter Roeck <linux@...ck-us.net>,
Hartmut Knaack <knaack.h@....de>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald <pmeerw@...erw.net>,
Antti Palosaari <crope@....fi>,
Mauro Carvalho Chehab <mchehab@....samsung.com>,
Rob Herring <robh+dt@...nel.org>,
Frank Rowand <frowand.list@...il.com>,
Grant Likely <grant.likely@...aro.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"David S. Miller" <davem@...emloft.net>,
Kalle Valo <kvalo@...eaurora.org>,
Joe Perches <joe@...ches.com>, Jiri Slaby <jslaby@...e.com>,
Daniel Baluta <daniel.baluta@...el.com>,
Adriana Reus <adriana.reus@...el.com>,
Lucas De Marchi <lucas.demarchi@...el.com>,
Matt Ranostay <matt.ranostay@...el.com>,
Krzysztof Kozlowski <k.kozlowski@...sung.com>,
Terry Heo <terryheo@...gle.com>,
Hans Verkuil <hans.verkuil@...co.com>,
Arnd Bergmann <arnd@...db.de>,
Tommi Rantala <tt.rantala@...il.com>,
Linux I2C <linux-i2c@...r.kernel.org>,
linux-doc@...r.kernel.org,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
linux-media@...r.kernel.org,
Devicetree List <devicetree@...r.kernel.org>
Subject: Re: [PATCH v6 20/24] iio: imu: inv_mpu6050: change the i2c gate to be mux-locked
On Sun, Apr 3, 2016 at 1:54 PM, Jonathan Cameron <jic23@...nel.org> wrote:
> On 03/04/16 09:52, Peter Rosin wrote:
>> From: Peter Rosin <peda@...ntia.se>
>>
>> The root i2c adapter lock is then no longer held by the i2c mux during
>> accesses behind the i2c gate, and such accesses need to take that lock
>> just like any other ordinary i2c accesses do.
>>
>> So, declare the i2c gate mux-locked, and zap the code that makes the
>> unlocked i2c accesses and just use ordinary regmap_write accesses.
>>
>> This also happens to fix the deadlock described in
>> http://patchwork.ozlabs.org/patch/584776/ authored by
>> Adriana Reus <adriana.reus@...el.com> and submitted by
>> Daniel Baluta <daniel.baluta@...el.com>
>>
>> ----------8<----------
>> iio: imu: inv_mpu6050: Fix deadlock between i2c adapter lock and mpu lock
>>
>> 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.
>>
>> ...
>> ----------8<----------
>>
>> The reason this patch fixes the deadlock is that T2 does not grab the
>> i2c adapter lock until the very end (and grabs the newfangled i2c mux
>> lock where it previously grabbed the i2c adapter lock).
>>
>> Signed-off-by: Peter Rosin <peda@...ntia.se>
> This one obviously wants a ack from Adriana or Daniel in addition to mine.
> I'm more than happy for these to go through the i2c tree btw.
>
> Acked-by: Jonathan Cameron <jic23@...nel.org>
Acked-by: Daniel Baluta <daniel.baluta@...el.com>
>> ---
>> Documentation/i2c/i2c-topology | 2 +-
>> drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 56 +++++++------------------------
>> 2 files changed, 13 insertions(+), 45 deletions(-)
>>
>> diff --git a/Documentation/i2c/i2c-topology b/Documentation/i2c/i2c-topology
>> index 7a10edd0874f..346623a80bd1 100644
>> --- a/Documentation/i2c/i2c-topology
>> +++ b/Documentation/i2c/i2c-topology
>> @@ -50,7 +50,7 @@ i2c-mux-pinctrl Normally parent-locked, mux-locked iff
>> i2c-mux-reg Parent-locked
>>
>> In drivers/iio/
>> -imu/inv_mpu6050/ Parent-locked
>> +imu/inv_mpu6050/ Mux-locked
>>
>> In drivers/media/
>> dvb-frontends/m88ds3103 Parent-locked
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>> index 0d429d788106..71ad31a275c9 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>> @@ -24,45 +24,16 @@ static const struct regmap_config inv_mpu_regmap_config = {
>> .val_bits = 8,
>> };
>>
>> -/*
>> - * 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 i2c_client *client,
>> - u8 reg, u8 d)
>> -{
>> - int ret;
>> - u8 buf[2] = {reg, d};
>> - struct i2c_msg msg[1] = {
>> - {
>> - .addr = client->addr,
>> - .flags = 0,
>> - .len = sizeof(buf),
>> - .buf = buf,
>> - }
>> - };
>> -
>> - ret = __i2c_transfer(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 i2c_client *client = i2c_mux_priv(muxc);
>> - struct iio_dev *indio_dev = dev_get_drvdata(&client->dev);
>> + struct iio_dev *indio_dev = i2c_mux_priv(muxc);
>> struct inv_mpu6050_state *st = iio_priv(indio_dev);
>> int ret = 0;
>>
>> /* 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(client,
>> - st->reg->pwr_mgmt_1, 0);
>> + ret = regmap_write(st->map, st->reg->pwr_mgmt_1, 0);
>> if (ret)
>> goto write_error;
>>
>> @@ -71,10 +42,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(client,
>> - st->reg->int_pin_cfg,
>> - INV_MPU6050_INT_PIN_CFG |
>> - INV_MPU6050_BIT_BYPASS_EN);
>> + ret = regmap_write(st->map, st->reg->int_pin_cfg,
>> + INV_MPU6050_INT_PIN_CFG |
>> + INV_MPU6050_BIT_BYPASS_EN);
>> }
>> write_error:
>> mutex_unlock(&indio_dev->mlock);
>> @@ -84,18 +54,16 @@ write_error:
>>
>> static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 chan_id)
>> {
>> - struct i2c_client *client = i2c_mux_priv(muxc);
>> - struct iio_dev *indio_dev = dev_get_drvdata(&client->dev);
>> + struct iio_dev *indio_dev = i2c_mux_priv(muxc);
>> struct inv_mpu6050_state *st = iio_priv(indio_dev);
>>
>> mutex_lock(&indio_dev->mlock);
>> /* It doesn't really mattter, if any of the calls fails */
>> - inv_mpu6050_write_reg_unlocked(client, st->reg->int_pin_cfg,
>> - INV_MPU6050_INT_PIN_CFG);
>> + regmap_write(st->map, st->reg->int_pin_cfg, INV_MPU6050_INT_PIN_CFG);
>> st->powerup_count--;
>> if (!st->powerup_count)
>> - inv_mpu6050_write_reg_unlocked(client, st->reg->pwr_mgmt_1,
>> - INV_MPU6050_BIT_SLEEP);
>> + regmap_write(st->map, st->reg->pwr_mgmt_1,
>> + INV_MPU6050_BIT_SLEEP);
>> mutex_unlock(&indio_dev->mlock);
>>
>> return 0;
>> @@ -133,15 +101,15 @@ static int inv_mpu_probe(struct i2c_client *client,
>> return result;
>>
>> st = iio_priv(dev_get_drvdata(&client->dev));
>> - 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_MUX_LOCKED, 0, 0, 0,
>> inv_mpu6050_select_bypass,
>> inv_mpu6050_deselect_bypass);
>> if (IS_ERR(st->muxc)) {
>> result = PTR_ERR(st->muxc);
>> goto out_unreg_device;
>> }
>> - st->muxc->priv = client;
>> + st->muxc->priv = dev_get_drvdata(&client->dev);
>>
>> result = inv_mpu_acpi_create_mux_client(client);
>> if (result)
>>
>
> --
> 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