* [RFC HACK] rtl2832: implement own lock for RegMap
@ 2014-12-18 10:29 Antti Palosaari
2014-12-18 11:21 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 4+ messages in thread
From: Antti Palosaari @ 2014-12-18 10:29 UTC (permalink / raw)
To: linux-media; +Cc: Antti Palosaari
Introduce own lock to silence locdep warning. I suspect lockdep checks
make wrong decision when two similar name (&map->mutex) locks were
taken recursively, even those are different mutexes in a two different
driver. After that patch, functionality remains same, but mutex names
are different.
=============================================
[ INFO: possible recursive locking detected ]
3.18.0-rc4+ #4 Tainted: G O
---------------------------------------------
kdvb-ad-0-fe-0/2814 is trying to acquire lock:
(&map->mutex){+.+.+.}, at: [<ffffffff814ec90f>] regmap_lock_mutex+0x2f/0x40
but task is already holding lock:
(&map->mutex){+.+.+.}, at: [<ffffffff814ec90f>] regmap_lock_mutex+0x2f/0x40
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(&map->mutex);
lock(&map->mutex);
*** DEADLOCK ***
May be due to missing lock nesting notation
1 lock held by kdvb-ad-0-fe-0/2814:
#0: (&map->mutex){+.+.+.}, at: [<ffffffff814ec90f>] regmap_lock_mutex+0x2f/0x40
stack backtrace:
CPU: 3 PID: 2814 Comm: kdvb-ad-0-fe-0 Tainted: G O 3.18.0-rc4+ #4
Hardware name: System manufacturer System Product Name/M5A78L-M/USB3, BIOS 2001 09/11/2014
0000000000000000 00000000410c8772 ffff880293af3868 ffffffff817a6f82
0000000000000000 ffff8800b3462be0 ffff880293af3968 ffffffff810e7f94
ffff880293af3888 00000000410c8772 ffffffff82dfee60 ffffffff81ab8f89
Call Trace:
[<ffffffff817a6f82>] dump_stack+0x4e/0x68
[<ffffffff810e7f94>] __lock_acquire+0x1ea4/0x1f50
[<ffffffff810e2a7d>] ? trace_hardirqs_off+0xd/0x10
[<ffffffff817b01f3>] ? _raw_spin_lock_irqsave+0x83/0xa0
[<ffffffff810e13e6>] ? up+0x16/0x50
[<ffffffff810e2a7d>] ? trace_hardirqs_off+0xd/0x10
[<ffffffff817af8bf>] ? _raw_spin_unlock_irqrestore+0x5f/0x70
[<ffffffff810e9069>] lock_acquire+0xc9/0x170
[<ffffffff814ec90f>] ? regmap_lock_mutex+0x2f/0x40
[<ffffffff817ab50e>] mutex_lock_nested+0x7e/0x430
[<ffffffff814ec90f>] ? regmap_lock_mutex+0x2f/0x40
[<ffffffff814ec90f>] ? regmap_lock_mutex+0x2f/0x40
[<ffffffff817a530b>] ? printk+0x70/0x86
[<ffffffff8110d9e8>] ? mod_timer+0x168/0x240
[<ffffffff814ec90f>] regmap_lock_mutex+0x2f/0x40
[<ffffffff814f08d9>] regmap_update_bits+0x29/0x60
[<ffffffffa03e9778>] rtl2832_select+0x38/0x70 [rtl2832]
[<ffffffffa039b03d>] i2c_mux_master_xfer+0x3d/0x90 [i2c_mux]
[<ffffffff815da493>] __i2c_transfer+0x73/0x2e0
[<ffffffff815dbaba>] i2c_transfer+0x5a/0xc0
[<ffffffff815dbb6e>] i2c_master_send+0x4e/0x70
[<ffffffffa03ff25a>] regmap_i2c_write+0x1a/0x50 [regmap_i2c]
[<ffffffff817ab713>] ? mutex_lock_nested+0x283/0x430
[<ffffffff814f06b2>] _regmap_raw_write+0x862/0x880
[<ffffffff814ec90f>] ? regmap_lock_mutex+0x2f/0x40
[<ffffffff814f0744>] _regmap_bus_raw_write+0x74/0xa0
[<ffffffff814ef3d2>] _regmap_write+0x92/0x140
[<ffffffff814f0b7b>] regmap_write+0x4b/0x70
[<ffffffffa032b090>] ? dvb_frontend_release+0x110/0x110 [dvb_core]
[<ffffffffa05141d4>] e4000_init+0x34/0x210 [e4000]
[<ffffffffa032a029>] dvb_frontend_init+0x59/0xc0 [dvb_core]
[<ffffffff810bde30>] ? finish_task_switch+0x80/0x180
[<ffffffff810bddf2>] ? finish_task_switch+0x42/0x180
[<ffffffffa032b116>] dvb_frontend_thread+0x86/0x7b0 [dvb_core]
[<ffffffff817a9203>] ? __schedule+0x343/0x930
[<ffffffffa032b090>] ? dvb_frontend_release+0x110/0x110 [dvb_core]
[<ffffffff810b826b>] kthread+0x10b/0x130
[<ffffffff81020099>] ? sched_clock+0x9/0x10
[<ffffffff810b8160>] ? kthread_create_on_node+0x250/0x250
[<ffffffff817b063c>] ret_from_fork+0x7c/0xb0
[<ffffffff810b8160>] ? kthread_create_on_node+0x250/0x250
Signed-off-by: Antti Palosaari <crope@iki.fi>
---
drivers/media/dvb-frontends/rtl2832.c | 49 +++++++++++++++++++++++-------
drivers/media/dvb-frontends/rtl2832_priv.h | 2 ++
2 files changed, 40 insertions(+), 11 deletions(-)
diff --git a/drivers/media/dvb-frontends/rtl2832.c b/drivers/media/dvb-frontends/rtl2832.c
index f44dc50..2ee5bcf 100644
--- a/drivers/media/dvb-frontends/rtl2832.c
+++ b/drivers/media/dvb-frontends/rtl2832.c
@@ -1028,6 +1028,31 @@ static int rtl2832_regmap_gather_write(void *context, const void *reg,
return 0;
}
+/*
+ * FIXME: Implement own RegMap locking in order to silence lockdep recursive
+ * lock warning. That happens when RegMap I2C client calls I2C mux adapter,
+ * which leads demod I2C repeater enable via demod RegMap. Operation takes two
+ * RegMap locks recursively - but those are different RegMap instances in a two
+ * different I2C drivers, so it should be deadlock.
+ */
+static void rtl2832_regmap_lock(void *__dev)
+{
+ struct rtl2832_dev *dev = __dev;
+ struct i2c_client *client = dev->client;
+
+ dev_dbg(&client->dev, "\n");
+ mutex_lock(&dev->regmap_mutex);
+}
+
+static void rtl2832_regmap_unlock(void *__dev)
+{
+ struct rtl2832_dev *dev = __dev;
+ struct i2c_client *client = dev->client;
+
+ dev_dbg(&client->dev, "\n");
+ mutex_unlock(&dev->regmap_mutex);
+}
+
static struct dvb_frontend *rtl2832_get_dvb_frontend(struct i2c_client *client)
{
struct rtl2832_dev *dev = i2c_get_clientdata(client);
@@ -1186,15 +1211,6 @@ static int rtl2832_probe(struct i2c_client *client,
.range_max = 5 * 0x100,
},
};
- static const struct regmap_config regmap_config = {
- .reg_bits = 8,
- .val_bits = 8,
- .volatile_reg = rtl2832_volatile_reg,
- .max_register = 5 * 0x100,
- .ranges = regmap_range_cfg,
- .num_ranges = ARRAY_SIZE(regmap_range_cfg),
- .cache_type = REGCACHE_RBTREE,
- };
dev_dbg(&client->dev, "\n");
@@ -1218,8 +1234,19 @@ static int rtl2832_probe(struct i2c_client *client,
INIT_DELAYED_WORK(&dev->i2c_gate_work, rtl2832_i2c_gate_work);
INIT_DELAYED_WORK(&dev->stat_work, rtl2832_stat_work);
/* create RegMap */
+ mutex_init(&dev->regmap_mutex);
+ dev->regmap_config.reg_bits = 8,
+ dev->regmap_config.val_bits = 8,
+ dev->regmap_config.lock = rtl2832_regmap_lock,
+ dev->regmap_config.unlock = rtl2832_regmap_unlock,
+ dev->regmap_config.lock_arg = dev,
+ dev->regmap_config.volatile_reg = rtl2832_volatile_reg,
+ dev->regmap_config.max_register = 5 * 0x100,
+ dev->regmap_config.ranges = regmap_range_cfg,
+ dev->regmap_config.num_ranges = ARRAY_SIZE(regmap_range_cfg),
+ dev->regmap_config.cache_type = REGCACHE_RBTREE,
dev->regmap = regmap_init(&client->dev, ®map_bus, client,
- ®map_config);
+ &dev->regmap_config);
if (IS_ERR(dev->regmap)) {
ret = PTR_ERR(dev->regmap);
goto err_kfree;
@@ -1232,7 +1259,7 @@ static int rtl2832_probe(struct i2c_client *client,
/* create muxed i2c adapter for demod tuner bus */
dev->i2c_adapter_tuner = i2c_add_mux_adapter(i2c, &i2c->dev, dev,
- 0, 0, 0, rtl2832_select, rtl2832_deselect);
+ 0, 1, 0, rtl2832_select, rtl2832_deselect);
if (dev->i2c_adapter_tuner == NULL) {
ret = -ENODEV;
goto err_regmap_exit;
diff --git a/drivers/media/dvb-frontends/rtl2832_priv.h b/drivers/media/dvb-frontends/rtl2832_priv.h
index 9ff4f65..c3a922c 100644
--- a/drivers/media/dvb-frontends/rtl2832_priv.h
+++ b/drivers/media/dvb-frontends/rtl2832_priv.h
@@ -33,6 +33,8 @@
struct rtl2832_dev {
struct rtl2832_platform_data *pdata;
struct i2c_client *client;
+ struct mutex regmap_mutex;
+ struct regmap_config regmap_config;
struct regmap *regmap;
struct i2c_adapter *i2c_adapter_tuner;
struct dvb_frontend fe;
--
http://palosaari.fi/
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC HACK] rtl2832: implement own lock for RegMap
2014-12-18 10:29 [RFC HACK] rtl2832: implement own lock for RegMap Antti Palosaari
@ 2014-12-18 11:21 ` Mauro Carvalho Chehab
2014-12-18 14:41 ` Antti Palosaari
0 siblings, 1 reply; 4+ messages in thread
From: Mauro Carvalho Chehab @ 2014-12-18 11:21 UTC (permalink / raw)
To: Antti Palosaari; +Cc: linux-media
Em Thu, 18 Dec 2014 12:29:46 +0200
Antti Palosaari <crope@iki.fi> escreveu:
> Introduce own lock to silence locdep warning. I suspect lockdep checks
> make wrong decision when two similar name (&map->mutex) locks were
> taken recursively, even those are different mutexes in a two different
> driver. After that patch, functionality remains same, but mutex names
> are different.
Please do not add a hack just to silence a lockdep warning.
Please take a look at: Documentation/locking/lockdep-design.txt
There are some documentation there about the usage of nested locks and
how to avoid lockdep to complain.
Regards,
Mauro
>
> =============================================
> [ INFO: possible recursive locking detected ]
> 3.18.0-rc4+ #4 Tainted: G O
> ---------------------------------------------
> kdvb-ad-0-fe-0/2814 is trying to acquire lock:
> (&map->mutex){+.+.+.}, at: [<ffffffff814ec90f>] regmap_lock_mutex+0x2f/0x40
>
> but task is already holding lock:
> (&map->mutex){+.+.+.}, at: [<ffffffff814ec90f>] regmap_lock_mutex+0x2f/0x40
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
> CPU0
> ----
> lock(&map->mutex);
> lock(&map->mutex);
>
> *** DEADLOCK ***
> May be due to missing lock nesting notation
> 1 lock held by kdvb-ad-0-fe-0/2814:
> #0: (&map->mutex){+.+.+.}, at: [<ffffffff814ec90f>] regmap_lock_mutex+0x2f/0x40
>
> stack backtrace:
> CPU: 3 PID: 2814 Comm: kdvb-ad-0-fe-0 Tainted: G O 3.18.0-rc4+ #4
> Hardware name: System manufacturer System Product Name/M5A78L-M/USB3, BIOS 2001 09/11/2014
> 0000000000000000 00000000410c8772 ffff880293af3868 ffffffff817a6f82
> 0000000000000000 ffff8800b3462be0 ffff880293af3968 ffffffff810e7f94
> ffff880293af3888 00000000410c8772 ffffffff82dfee60 ffffffff81ab8f89
> Call Trace:
> [<ffffffff817a6f82>] dump_stack+0x4e/0x68
> [<ffffffff810e7f94>] __lock_acquire+0x1ea4/0x1f50
> [<ffffffff810e2a7d>] ? trace_hardirqs_off+0xd/0x10
> [<ffffffff817b01f3>] ? _raw_spin_lock_irqsave+0x83/0xa0
> [<ffffffff810e13e6>] ? up+0x16/0x50
> [<ffffffff810e2a7d>] ? trace_hardirqs_off+0xd/0x10
> [<ffffffff817af8bf>] ? _raw_spin_unlock_irqrestore+0x5f/0x70
> [<ffffffff810e9069>] lock_acquire+0xc9/0x170
> [<ffffffff814ec90f>] ? regmap_lock_mutex+0x2f/0x40
> [<ffffffff817ab50e>] mutex_lock_nested+0x7e/0x430
> [<ffffffff814ec90f>] ? regmap_lock_mutex+0x2f/0x40
> [<ffffffff814ec90f>] ? regmap_lock_mutex+0x2f/0x40
> [<ffffffff817a530b>] ? printk+0x70/0x86
> [<ffffffff8110d9e8>] ? mod_timer+0x168/0x240
> [<ffffffff814ec90f>] regmap_lock_mutex+0x2f/0x40
> [<ffffffff814f08d9>] regmap_update_bits+0x29/0x60
> [<ffffffffa03e9778>] rtl2832_select+0x38/0x70 [rtl2832]
> [<ffffffffa039b03d>] i2c_mux_master_xfer+0x3d/0x90 [i2c_mux]
> [<ffffffff815da493>] __i2c_transfer+0x73/0x2e0
> [<ffffffff815dbaba>] i2c_transfer+0x5a/0xc0
> [<ffffffff815dbb6e>] i2c_master_send+0x4e/0x70
> [<ffffffffa03ff25a>] regmap_i2c_write+0x1a/0x50 [regmap_i2c]
> [<ffffffff817ab713>] ? mutex_lock_nested+0x283/0x430
> [<ffffffff814f06b2>] _regmap_raw_write+0x862/0x880
> [<ffffffff814ec90f>] ? regmap_lock_mutex+0x2f/0x40
> [<ffffffff814f0744>] _regmap_bus_raw_write+0x74/0xa0
> [<ffffffff814ef3d2>] _regmap_write+0x92/0x140
> [<ffffffff814f0b7b>] regmap_write+0x4b/0x70
> [<ffffffffa032b090>] ? dvb_frontend_release+0x110/0x110 [dvb_core]
> [<ffffffffa05141d4>] e4000_init+0x34/0x210 [e4000]
> [<ffffffffa032a029>] dvb_frontend_init+0x59/0xc0 [dvb_core]
> [<ffffffff810bde30>] ? finish_task_switch+0x80/0x180
> [<ffffffff810bddf2>] ? finish_task_switch+0x42/0x180
> [<ffffffffa032b116>] dvb_frontend_thread+0x86/0x7b0 [dvb_core]
> [<ffffffff817a9203>] ? __schedule+0x343/0x930
> [<ffffffffa032b090>] ? dvb_frontend_release+0x110/0x110 [dvb_core]
> [<ffffffff810b826b>] kthread+0x10b/0x130
> [<ffffffff81020099>] ? sched_clock+0x9/0x10
> [<ffffffff810b8160>] ? kthread_create_on_node+0x250/0x250
> [<ffffffff817b063c>] ret_from_fork+0x7c/0xb0
> [<ffffffff810b8160>] ? kthread_create_on_node+0x250/0x250
>
> Signed-off-by: Antti Palosaari <crope@iki.fi>
> ---
> drivers/media/dvb-frontends/rtl2832.c | 49 +++++++++++++++++++++++-------
> drivers/media/dvb-frontends/rtl2832_priv.h | 2 ++
> 2 files changed, 40 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/rtl2832.c b/drivers/media/dvb-frontends/rtl2832.c
> index f44dc50..2ee5bcf 100644
> --- a/drivers/media/dvb-frontends/rtl2832.c
> +++ b/drivers/media/dvb-frontends/rtl2832.c
> @@ -1028,6 +1028,31 @@ static int rtl2832_regmap_gather_write(void *context, const void *reg,
> return 0;
> }
>
> +/*
> + * FIXME: Implement own RegMap locking in order to silence lockdep recursive
> + * lock warning. That happens when RegMap I2C client calls I2C mux adapter,
> + * which leads demod I2C repeater enable via demod RegMap. Operation takes two
> + * RegMap locks recursively - but those are different RegMap instances in a two
> + * different I2C drivers, so it should be deadlock.
> + */
> +static void rtl2832_regmap_lock(void *__dev)
> +{
> + struct rtl2832_dev *dev = __dev;
> + struct i2c_client *client = dev->client;
> +
> + dev_dbg(&client->dev, "\n");
> + mutex_lock(&dev->regmap_mutex);
> +}
> +
> +static void rtl2832_regmap_unlock(void *__dev)
> +{
> + struct rtl2832_dev *dev = __dev;
> + struct i2c_client *client = dev->client;
> +
> + dev_dbg(&client->dev, "\n");
> + mutex_unlock(&dev->regmap_mutex);
> +}
> +
> static struct dvb_frontend *rtl2832_get_dvb_frontend(struct i2c_client *client)
> {
> struct rtl2832_dev *dev = i2c_get_clientdata(client);
> @@ -1186,15 +1211,6 @@ static int rtl2832_probe(struct i2c_client *client,
> .range_max = 5 * 0x100,
> },
> };
> - static const struct regmap_config regmap_config = {
> - .reg_bits = 8,
> - .val_bits = 8,
> - .volatile_reg = rtl2832_volatile_reg,
> - .max_register = 5 * 0x100,
> - .ranges = regmap_range_cfg,
> - .num_ranges = ARRAY_SIZE(regmap_range_cfg),
> - .cache_type = REGCACHE_RBTREE,
> - };
>
> dev_dbg(&client->dev, "\n");
>
> @@ -1218,8 +1234,19 @@ static int rtl2832_probe(struct i2c_client *client,
> INIT_DELAYED_WORK(&dev->i2c_gate_work, rtl2832_i2c_gate_work);
> INIT_DELAYED_WORK(&dev->stat_work, rtl2832_stat_work);
> /* create RegMap */
> + mutex_init(&dev->regmap_mutex);
> + dev->regmap_config.reg_bits = 8,
> + dev->regmap_config.val_bits = 8,
> + dev->regmap_config.lock = rtl2832_regmap_lock,
> + dev->regmap_config.unlock = rtl2832_regmap_unlock,
> + dev->regmap_config.lock_arg = dev,
> + dev->regmap_config.volatile_reg = rtl2832_volatile_reg,
> + dev->regmap_config.max_register = 5 * 0x100,
> + dev->regmap_config.ranges = regmap_range_cfg,
> + dev->regmap_config.num_ranges = ARRAY_SIZE(regmap_range_cfg),
> + dev->regmap_config.cache_type = REGCACHE_RBTREE,
> dev->regmap = regmap_init(&client->dev, ®map_bus, client,
> - ®map_config);
> + &dev->regmap_config);
> if (IS_ERR(dev->regmap)) {
> ret = PTR_ERR(dev->regmap);
> goto err_kfree;
> @@ -1232,7 +1259,7 @@ static int rtl2832_probe(struct i2c_client *client,
>
> /* create muxed i2c adapter for demod tuner bus */
> dev->i2c_adapter_tuner = i2c_add_mux_adapter(i2c, &i2c->dev, dev,
> - 0, 0, 0, rtl2832_select, rtl2832_deselect);
> + 0, 1, 0, rtl2832_select, rtl2832_deselect);
> if (dev->i2c_adapter_tuner == NULL) {
> ret = -ENODEV;
> goto err_regmap_exit;
> diff --git a/drivers/media/dvb-frontends/rtl2832_priv.h b/drivers/media/dvb-frontends/rtl2832_priv.h
> index 9ff4f65..c3a922c 100644
> --- a/drivers/media/dvb-frontends/rtl2832_priv.h
> +++ b/drivers/media/dvb-frontends/rtl2832_priv.h
> @@ -33,6 +33,8 @@
> struct rtl2832_dev {
> struct rtl2832_platform_data *pdata;
> struct i2c_client *client;
> + struct mutex regmap_mutex;
> + struct regmap_config regmap_config;
> struct regmap *regmap;
> struct i2c_adapter *i2c_adapter_tuner;
> struct dvb_frontend fe;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC HACK] rtl2832: implement own lock for RegMap
2014-12-18 11:21 ` Mauro Carvalho Chehab
@ 2014-12-18 14:41 ` Antti Palosaari
2014-12-18 14:46 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 4+ messages in thread
From: Antti Palosaari @ 2014-12-18 14:41 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media
On 12/18/2014 01:21 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 18 Dec 2014 12:29:46 +0200
> Antti Palosaari <crope@iki.fi> escreveu:
>
>> Introduce own lock to silence locdep warning. I suspect lockdep checks
>> make wrong decision when two similar name (&map->mutex) locks were
>> taken recursively, even those are different mutexes in a two different
>> driver. After that patch, functionality remains same, but mutex names
>> are different.
>
> Please do not add a hack just to silence a lockdep warning.
>
> Please take a look at: Documentation/locking/lockdep-design.txt
>
> There are some documentation there about the usage of nested locks and
> how to avoid lockdep to complain.
I cannot see any way how those lockdep things could be used on my driver
as problematic lock itself is located inside RegMap. I think it is
RegMap which needs some special lockdep things in order to allow nested
locking of different instances.
So I think demod I2C adapter is good place for that kind of hack until
better solution is find. Defining own RegMap lock here in I2C repeater
allows all the clients be hack free using default RegMap lock.
regards
Antti
>
> Regards,
> Mauro
>
>>
>> =============================================
>> [ INFO: possible recursive locking detected ]
>> 3.18.0-rc4+ #4 Tainted: G O
>> ---------------------------------------------
>> kdvb-ad-0-fe-0/2814 is trying to acquire lock:
>> (&map->mutex){+.+.+.}, at: [<ffffffff814ec90f>] regmap_lock_mutex+0x2f/0x40
>>
>> but task is already holding lock:
>> (&map->mutex){+.+.+.}, at: [<ffffffff814ec90f>] regmap_lock_mutex+0x2f/0x40
>>
>> other info that might help us debug this:
>> Possible unsafe locking scenario:
>> CPU0
>> ----
>> lock(&map->mutex);
>> lock(&map->mutex);
>>
>> *** DEADLOCK ***
>> May be due to missing lock nesting notation
>> 1 lock held by kdvb-ad-0-fe-0/2814:
>> #0: (&map->mutex){+.+.+.}, at: [<ffffffff814ec90f>] regmap_lock_mutex+0x2f/0x40
>>
>> stack backtrace:
>> CPU: 3 PID: 2814 Comm: kdvb-ad-0-fe-0 Tainted: G O 3.18.0-rc4+ #4
>> Hardware name: System manufacturer System Product Name/M5A78L-M/USB3, BIOS 2001 09/11/2014
>> 0000000000000000 00000000410c8772 ffff880293af3868 ffffffff817a6f82
>> 0000000000000000 ffff8800b3462be0 ffff880293af3968 ffffffff810e7f94
>> ffff880293af3888 00000000410c8772 ffffffff82dfee60 ffffffff81ab8f89
>> Call Trace:
>> [<ffffffff817a6f82>] dump_stack+0x4e/0x68
>> [<ffffffff810e7f94>] __lock_acquire+0x1ea4/0x1f50
>> [<ffffffff810e2a7d>] ? trace_hardirqs_off+0xd/0x10
>> [<ffffffff817b01f3>] ? _raw_spin_lock_irqsave+0x83/0xa0
>> [<ffffffff810e13e6>] ? up+0x16/0x50
>> [<ffffffff810e2a7d>] ? trace_hardirqs_off+0xd/0x10
>> [<ffffffff817af8bf>] ? _raw_spin_unlock_irqrestore+0x5f/0x70
>> [<ffffffff810e9069>] lock_acquire+0xc9/0x170
>> [<ffffffff814ec90f>] ? regmap_lock_mutex+0x2f/0x40
>> [<ffffffff817ab50e>] mutex_lock_nested+0x7e/0x430
>> [<ffffffff814ec90f>] ? regmap_lock_mutex+0x2f/0x40
>> [<ffffffff814ec90f>] ? regmap_lock_mutex+0x2f/0x40
>> [<ffffffff817a530b>] ? printk+0x70/0x86
>> [<ffffffff8110d9e8>] ? mod_timer+0x168/0x240
>> [<ffffffff814ec90f>] regmap_lock_mutex+0x2f/0x40
>> [<ffffffff814f08d9>] regmap_update_bits+0x29/0x60
>> [<ffffffffa03e9778>] rtl2832_select+0x38/0x70 [rtl2832]
>> [<ffffffffa039b03d>] i2c_mux_master_xfer+0x3d/0x90 [i2c_mux]
>> [<ffffffff815da493>] __i2c_transfer+0x73/0x2e0
>> [<ffffffff815dbaba>] i2c_transfer+0x5a/0xc0
>> [<ffffffff815dbb6e>] i2c_master_send+0x4e/0x70
>> [<ffffffffa03ff25a>] regmap_i2c_write+0x1a/0x50 [regmap_i2c]
>> [<ffffffff817ab713>] ? mutex_lock_nested+0x283/0x430
>> [<ffffffff814f06b2>] _regmap_raw_write+0x862/0x880
>> [<ffffffff814ec90f>] ? regmap_lock_mutex+0x2f/0x40
>> [<ffffffff814f0744>] _regmap_bus_raw_write+0x74/0xa0
>> [<ffffffff814ef3d2>] _regmap_write+0x92/0x140
>> [<ffffffff814f0b7b>] regmap_write+0x4b/0x70
>> [<ffffffffa032b090>] ? dvb_frontend_release+0x110/0x110 [dvb_core]
>> [<ffffffffa05141d4>] e4000_init+0x34/0x210 [e4000]
>> [<ffffffffa032a029>] dvb_frontend_init+0x59/0xc0 [dvb_core]
>> [<ffffffff810bde30>] ? finish_task_switch+0x80/0x180
>> [<ffffffff810bddf2>] ? finish_task_switch+0x42/0x180
>> [<ffffffffa032b116>] dvb_frontend_thread+0x86/0x7b0 [dvb_core]
>> [<ffffffff817a9203>] ? __schedule+0x343/0x930
>> [<ffffffffa032b090>] ? dvb_frontend_release+0x110/0x110 [dvb_core]
>> [<ffffffff810b826b>] kthread+0x10b/0x130
>> [<ffffffff81020099>] ? sched_clock+0x9/0x10
>> [<ffffffff810b8160>] ? kthread_create_on_node+0x250/0x250
>> [<ffffffff817b063c>] ret_from_fork+0x7c/0xb0
>> [<ffffffff810b8160>] ? kthread_create_on_node+0x250/0x250
>>
>> Signed-off-by: Antti Palosaari <crope@iki.fi>
>> ---
>> drivers/media/dvb-frontends/rtl2832.c | 49 +++++++++++++++++++++++-------
>> drivers/media/dvb-frontends/rtl2832_priv.h | 2 ++
>> 2 files changed, 40 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/dvb-frontends/rtl2832.c b/drivers/media/dvb-frontends/rtl2832.c
>> index f44dc50..2ee5bcf 100644
>> --- a/drivers/media/dvb-frontends/rtl2832.c
>> +++ b/drivers/media/dvb-frontends/rtl2832.c
>> @@ -1028,6 +1028,31 @@ static int rtl2832_regmap_gather_write(void *context, const void *reg,
>> return 0;
>> }
>>
>> +/*
>> + * FIXME: Implement own RegMap locking in order to silence lockdep recursive
>> + * lock warning. That happens when RegMap I2C client calls I2C mux adapter,
>> + * which leads demod I2C repeater enable via demod RegMap. Operation takes two
>> + * RegMap locks recursively - but those are different RegMap instances in a two
>> + * different I2C drivers, so it should be deadlock.
>> + */
>> +static void rtl2832_regmap_lock(void *__dev)
>> +{
>> + struct rtl2832_dev *dev = __dev;
>> + struct i2c_client *client = dev->client;
>> +
>> + dev_dbg(&client->dev, "\n");
>> + mutex_lock(&dev->regmap_mutex);
>> +}
>> +
>> +static void rtl2832_regmap_unlock(void *__dev)
>> +{
>> + struct rtl2832_dev *dev = __dev;
>> + struct i2c_client *client = dev->client;
>> +
>> + dev_dbg(&client->dev, "\n");
>> + mutex_unlock(&dev->regmap_mutex);
>> +}
>> +
>> static struct dvb_frontend *rtl2832_get_dvb_frontend(struct i2c_client *client)
>> {
>> struct rtl2832_dev *dev = i2c_get_clientdata(client);
>> @@ -1186,15 +1211,6 @@ static int rtl2832_probe(struct i2c_client *client,
>> .range_max = 5 * 0x100,
>> },
>> };
>> - static const struct regmap_config regmap_config = {
>> - .reg_bits = 8,
>> - .val_bits = 8,
>> - .volatile_reg = rtl2832_volatile_reg,
>> - .max_register = 5 * 0x100,
>> - .ranges = regmap_range_cfg,
>> - .num_ranges = ARRAY_SIZE(regmap_range_cfg),
>> - .cache_type = REGCACHE_RBTREE,
>> - };
>>
>> dev_dbg(&client->dev, "\n");
>>
>> @@ -1218,8 +1234,19 @@ static int rtl2832_probe(struct i2c_client *client,
>> INIT_DELAYED_WORK(&dev->i2c_gate_work, rtl2832_i2c_gate_work);
>> INIT_DELAYED_WORK(&dev->stat_work, rtl2832_stat_work);
>> /* create RegMap */
>> + mutex_init(&dev->regmap_mutex);
>> + dev->regmap_config.reg_bits = 8,
>> + dev->regmap_config.val_bits = 8,
>> + dev->regmap_config.lock = rtl2832_regmap_lock,
>> + dev->regmap_config.unlock = rtl2832_regmap_unlock,
>> + dev->regmap_config.lock_arg = dev,
>> + dev->regmap_config.volatile_reg = rtl2832_volatile_reg,
>> + dev->regmap_config.max_register = 5 * 0x100,
>> + dev->regmap_config.ranges = regmap_range_cfg,
>> + dev->regmap_config.num_ranges = ARRAY_SIZE(regmap_range_cfg),
>> + dev->regmap_config.cache_type = REGCACHE_RBTREE,
>> dev->regmap = regmap_init(&client->dev, ®map_bus, client,
>> - ®map_config);
>> + &dev->regmap_config);
>> if (IS_ERR(dev->regmap)) {
>> ret = PTR_ERR(dev->regmap);
>> goto err_kfree;
>> @@ -1232,7 +1259,7 @@ static int rtl2832_probe(struct i2c_client *client,
>>
>> /* create muxed i2c adapter for demod tuner bus */
>> dev->i2c_adapter_tuner = i2c_add_mux_adapter(i2c, &i2c->dev, dev,
>> - 0, 0, 0, rtl2832_select, rtl2832_deselect);
>> + 0, 1, 0, rtl2832_select, rtl2832_deselect);
>> if (dev->i2c_adapter_tuner == NULL) {
>> ret = -ENODEV;
>> goto err_regmap_exit;
>> diff --git a/drivers/media/dvb-frontends/rtl2832_priv.h b/drivers/media/dvb-frontends/rtl2832_priv.h
>> index 9ff4f65..c3a922c 100644
>> --- a/drivers/media/dvb-frontends/rtl2832_priv.h
>> +++ b/drivers/media/dvb-frontends/rtl2832_priv.h
>> @@ -33,6 +33,8 @@
>> struct rtl2832_dev {
>> struct rtl2832_platform_data *pdata;
>> struct i2c_client *client;
>> + struct mutex regmap_mutex;
>> + struct regmap_config regmap_config;
>> struct regmap *regmap;
>> struct i2c_adapter *i2c_adapter_tuner;
>> struct dvb_frontend fe;
--
http://palosaari.fi/
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC HACK] rtl2832: implement own lock for RegMap
2014-12-18 14:41 ` Antti Palosaari
@ 2014-12-18 14:46 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 4+ messages in thread
From: Mauro Carvalho Chehab @ 2014-12-18 14:46 UTC (permalink / raw)
To: Antti Palosaari; +Cc: linux-media
Em Thu, 18 Dec 2014 16:41:28 +0200
Antti Palosaari <crope@iki.fi> escreveu:
> On 12/18/2014 01:21 PM, Mauro Carvalho Chehab wrote:
> > Em Thu, 18 Dec 2014 12:29:46 +0200
> > Antti Palosaari <crope@iki.fi> escreveu:
> >
> >> Introduce own lock to silence locdep warning. I suspect lockdep checks
> >> make wrong decision when two similar name (&map->mutex) locks were
> >> taken recursively, even those are different mutexes in a two different
> >> driver. After that patch, functionality remains same, but mutex names
> >> are different.
> >
> > Please do not add a hack just to silence a lockdep warning.
> >
> > Please take a look at: Documentation/locking/lockdep-design.txt
> >
> > There are some documentation there about the usage of nested locks and
> > how to avoid lockdep to complain.
>
> I cannot see any way how those lockdep things could be used on my driver
> as problematic lock itself is located inside RegMap. I think it is
> RegMap which needs some special lockdep things in order to allow nested
> locking of different instances.
Ok. So, do a patch for RegMap and send for its maintainers.
Regards,
Mauro
>
> So I think demod I2C adapter is good place for that kind of hack until
> better solution is find. Defining own RegMap lock here in I2C repeater
> allows all the clients be hack free using default RegMap lock.
>
> regards
> Antti
>
>
> >
> > Regards,
> > Mauro
> >
> >>
> >> =============================================
> >> [ INFO: possible recursive locking detected ]
> >> 3.18.0-rc4+ #4 Tainted: G O
> >> ---------------------------------------------
> >> kdvb-ad-0-fe-0/2814 is trying to acquire lock:
> >> (&map->mutex){+.+.+.}, at: [<ffffffff814ec90f>] regmap_lock_mutex+0x2f/0x40
> >>
> >> but task is already holding lock:
> >> (&map->mutex){+.+.+.}, at: [<ffffffff814ec90f>] regmap_lock_mutex+0x2f/0x40
> >>
> >> other info that might help us debug this:
> >> Possible unsafe locking scenario:
> >> CPU0
> >> ----
> >> lock(&map->mutex);
> >> lock(&map->mutex);
> >>
> >> *** DEADLOCK ***
> >> May be due to missing lock nesting notation
> >> 1 lock held by kdvb-ad-0-fe-0/2814:
> >> #0: (&map->mutex){+.+.+.}, at: [<ffffffff814ec90f>] regmap_lock_mutex+0x2f/0x40
> >>
> >> stack backtrace:
> >> CPU: 3 PID: 2814 Comm: kdvb-ad-0-fe-0 Tainted: G O 3.18.0-rc4+ #4
> >> Hardware name: System manufacturer System Product Name/M5A78L-M/USB3, BIOS 2001 09/11/2014
> >> 0000000000000000 00000000410c8772 ffff880293af3868 ffffffff817a6f82
> >> 0000000000000000 ffff8800b3462be0 ffff880293af3968 ffffffff810e7f94
> >> ffff880293af3888 00000000410c8772 ffffffff82dfee60 ffffffff81ab8f89
> >> Call Trace:
> >> [<ffffffff817a6f82>] dump_stack+0x4e/0x68
> >> [<ffffffff810e7f94>] __lock_acquire+0x1ea4/0x1f50
> >> [<ffffffff810e2a7d>] ? trace_hardirqs_off+0xd/0x10
> >> [<ffffffff817b01f3>] ? _raw_spin_lock_irqsave+0x83/0xa0
> >> [<ffffffff810e13e6>] ? up+0x16/0x50
> >> [<ffffffff810e2a7d>] ? trace_hardirqs_off+0xd/0x10
> >> [<ffffffff817af8bf>] ? _raw_spin_unlock_irqrestore+0x5f/0x70
> >> [<ffffffff810e9069>] lock_acquire+0xc9/0x170
> >> [<ffffffff814ec90f>] ? regmap_lock_mutex+0x2f/0x40
> >> [<ffffffff817ab50e>] mutex_lock_nested+0x7e/0x430
> >> [<ffffffff814ec90f>] ? regmap_lock_mutex+0x2f/0x40
> >> [<ffffffff814ec90f>] ? regmap_lock_mutex+0x2f/0x40
> >> [<ffffffff817a530b>] ? printk+0x70/0x86
> >> [<ffffffff8110d9e8>] ? mod_timer+0x168/0x240
> >> [<ffffffff814ec90f>] regmap_lock_mutex+0x2f/0x40
> >> [<ffffffff814f08d9>] regmap_update_bits+0x29/0x60
> >> [<ffffffffa03e9778>] rtl2832_select+0x38/0x70 [rtl2832]
> >> [<ffffffffa039b03d>] i2c_mux_master_xfer+0x3d/0x90 [i2c_mux]
> >> [<ffffffff815da493>] __i2c_transfer+0x73/0x2e0
> >> [<ffffffff815dbaba>] i2c_transfer+0x5a/0xc0
> >> [<ffffffff815dbb6e>] i2c_master_send+0x4e/0x70
> >> [<ffffffffa03ff25a>] regmap_i2c_write+0x1a/0x50 [regmap_i2c]
> >> [<ffffffff817ab713>] ? mutex_lock_nested+0x283/0x430
> >> [<ffffffff814f06b2>] _regmap_raw_write+0x862/0x880
> >> [<ffffffff814ec90f>] ? regmap_lock_mutex+0x2f/0x40
> >> [<ffffffff814f0744>] _regmap_bus_raw_write+0x74/0xa0
> >> [<ffffffff814ef3d2>] _regmap_write+0x92/0x140
> >> [<ffffffff814f0b7b>] regmap_write+0x4b/0x70
> >> [<ffffffffa032b090>] ? dvb_frontend_release+0x110/0x110 [dvb_core]
> >> [<ffffffffa05141d4>] e4000_init+0x34/0x210 [e4000]
> >> [<ffffffffa032a029>] dvb_frontend_init+0x59/0xc0 [dvb_core]
> >> [<ffffffff810bde30>] ? finish_task_switch+0x80/0x180
> >> [<ffffffff810bddf2>] ? finish_task_switch+0x42/0x180
> >> [<ffffffffa032b116>] dvb_frontend_thread+0x86/0x7b0 [dvb_core]
> >> [<ffffffff817a9203>] ? __schedule+0x343/0x930
> >> [<ffffffffa032b090>] ? dvb_frontend_release+0x110/0x110 [dvb_core]
> >> [<ffffffff810b826b>] kthread+0x10b/0x130
> >> [<ffffffff81020099>] ? sched_clock+0x9/0x10
> >> [<ffffffff810b8160>] ? kthread_create_on_node+0x250/0x250
> >> [<ffffffff817b063c>] ret_from_fork+0x7c/0xb0
> >> [<ffffffff810b8160>] ? kthread_create_on_node+0x250/0x250
> >>
> >> Signed-off-by: Antti Palosaari <crope@iki.fi>
> >> ---
> >> drivers/media/dvb-frontends/rtl2832.c | 49 +++++++++++++++++++++++-------
> >> drivers/media/dvb-frontends/rtl2832_priv.h | 2 ++
> >> 2 files changed, 40 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/media/dvb-frontends/rtl2832.c b/drivers/media/dvb-frontends/rtl2832.c
> >> index f44dc50..2ee5bcf 100644
> >> --- a/drivers/media/dvb-frontends/rtl2832.c
> >> +++ b/drivers/media/dvb-frontends/rtl2832.c
> >> @@ -1028,6 +1028,31 @@ static int rtl2832_regmap_gather_write(void *context, const void *reg,
> >> return 0;
> >> }
> >>
> >> +/*
> >> + * FIXME: Implement own RegMap locking in order to silence lockdep recursive
> >> + * lock warning. That happens when RegMap I2C client calls I2C mux adapter,
> >> + * which leads demod I2C repeater enable via demod RegMap. Operation takes two
> >> + * RegMap locks recursively - but those are different RegMap instances in a two
> >> + * different I2C drivers, so it should be deadlock.
> >> + */
> >> +static void rtl2832_regmap_lock(void *__dev)
> >> +{
> >> + struct rtl2832_dev *dev = __dev;
> >> + struct i2c_client *client = dev->client;
> >> +
> >> + dev_dbg(&client->dev, "\n");
> >> + mutex_lock(&dev->regmap_mutex);
> >> +}
> >> +
> >> +static void rtl2832_regmap_unlock(void *__dev)
> >> +{
> >> + struct rtl2832_dev *dev = __dev;
> >> + struct i2c_client *client = dev->client;
> >> +
> >> + dev_dbg(&client->dev, "\n");
> >> + mutex_unlock(&dev->regmap_mutex);
> >> +}
> >> +
> >> static struct dvb_frontend *rtl2832_get_dvb_frontend(struct i2c_client *client)
> >> {
> >> struct rtl2832_dev *dev = i2c_get_clientdata(client);
> >> @@ -1186,15 +1211,6 @@ static int rtl2832_probe(struct i2c_client *client,
> >> .range_max = 5 * 0x100,
> >> },
> >> };
> >> - static const struct regmap_config regmap_config = {
> >> - .reg_bits = 8,
> >> - .val_bits = 8,
> >> - .volatile_reg = rtl2832_volatile_reg,
> >> - .max_register = 5 * 0x100,
> >> - .ranges = regmap_range_cfg,
> >> - .num_ranges = ARRAY_SIZE(regmap_range_cfg),
> >> - .cache_type = REGCACHE_RBTREE,
> >> - };
> >>
> >> dev_dbg(&client->dev, "\n");
> >>
> >> @@ -1218,8 +1234,19 @@ static int rtl2832_probe(struct i2c_client *client,
> >> INIT_DELAYED_WORK(&dev->i2c_gate_work, rtl2832_i2c_gate_work);
> >> INIT_DELAYED_WORK(&dev->stat_work, rtl2832_stat_work);
> >> /* create RegMap */
> >> + mutex_init(&dev->regmap_mutex);
> >> + dev->regmap_config.reg_bits = 8,
> >> + dev->regmap_config.val_bits = 8,
> >> + dev->regmap_config.lock = rtl2832_regmap_lock,
> >> + dev->regmap_config.unlock = rtl2832_regmap_unlock,
> >> + dev->regmap_config.lock_arg = dev,
> >> + dev->regmap_config.volatile_reg = rtl2832_volatile_reg,
> >> + dev->regmap_config.max_register = 5 * 0x100,
> >> + dev->regmap_config.ranges = regmap_range_cfg,
> >> + dev->regmap_config.num_ranges = ARRAY_SIZE(regmap_range_cfg),
> >> + dev->regmap_config.cache_type = REGCACHE_RBTREE,
> >> dev->regmap = regmap_init(&client->dev, ®map_bus, client,
> >> - ®map_config);
> >> + &dev->regmap_config);
> >> if (IS_ERR(dev->regmap)) {
> >> ret = PTR_ERR(dev->regmap);
> >> goto err_kfree;
> >> @@ -1232,7 +1259,7 @@ static int rtl2832_probe(struct i2c_client *client,
> >>
> >> /* create muxed i2c adapter for demod tuner bus */
> >> dev->i2c_adapter_tuner = i2c_add_mux_adapter(i2c, &i2c->dev, dev,
> >> - 0, 0, 0, rtl2832_select, rtl2832_deselect);
> >> + 0, 1, 0, rtl2832_select, rtl2832_deselect);
> >> if (dev->i2c_adapter_tuner == NULL) {
> >> ret = -ENODEV;
> >> goto err_regmap_exit;
> >> diff --git a/drivers/media/dvb-frontends/rtl2832_priv.h b/drivers/media/dvb-frontends/rtl2832_priv.h
> >> index 9ff4f65..c3a922c 100644
> >> --- a/drivers/media/dvb-frontends/rtl2832_priv.h
> >> +++ b/drivers/media/dvb-frontends/rtl2832_priv.h
> >> @@ -33,6 +33,8 @@
> >> struct rtl2832_dev {
> >> struct rtl2832_platform_data *pdata;
> >> struct i2c_client *client;
> >> + struct mutex regmap_mutex;
> >> + struct regmap_config regmap_config;
> >> struct regmap *regmap;
> >> struct i2c_adapter *i2c_adapter_tuner;
> >> struct dvb_frontend fe;
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-12-18 14:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-18 10:29 [RFC HACK] rtl2832: implement own lock for RegMap Antti Palosaari
2014-12-18 11:21 ` Mauro Carvalho Chehab
2014-12-18 14:41 ` Antti Palosaari
2014-12-18 14:46 ` Mauro Carvalho Chehab
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).