I'm was on the receiving end of a lockdep splat from this driver and after
scratching my head I couldn't be entirely sure it was a false positive
given we would also have to think about whether the regulator locking is
safe (since the notifier is called whilst holding regulator locks which
are also needed for regulator_is_enabled() ).
Regardless of whether it is a real bug or not, the mutex isn't needed.
We can use reference counting tricks instead to avoid races with the
notifier calls.
The observed splat follows:
------------------------------------------------------
kworker/u16:3/127 is trying to acquire lock:
ffff00008021fb20 (&ihid_goodix->regulator_mutex){+.+.}-{4:4}, at: ihid_goodix_vdd_notify+0x30/0x94
but task is already holding lock:
ffff0000835c60c0 (&(&rdev->notifier)->rwsem){++++}-{4:4}, at: blocking_notifier_call_chain+0x30/0x70
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (&(&rdev->notifier)->rwsem){++++}-{4:4}:
down_write+0x68/0x8c
blocking_notifier_chain_register+0x54/0x70
regulator_register_notifier+0x1c/0x24
devm_regulator_register_notifier+0x58/0x98
i2c_hid_of_goodix_probe+0xdc/0x158
i2c_device_probe+0x25d/0x270
really_probe+0x174/0x2cc
__driver_probe_device+0xc0/0xd8
driver_probe_device+0x50/0xe4
__device_attach_driver+0xa8/0xc0
bus_for_each_drv+0x9c/0xc0
__device_attach_async_helper+0x6c/0xbc
async_run_entry_fn+0x38/0x100
process_one_work+0x294/0x438
worker_thread+0x180/0x258
kthread+0x120/0x130
ret_from_fork+0x10/0x20
-> #0 (&ihid_goodix->regulator_mutex){+.+.}-{4:4}:
__lock_acquire+0xd24/0xfe8
lock_acquire+0x288/0x2f4
__mutex_lock+0xa0/0x338
mutex_lock_nested+0x3c/0x5c
ihid_goodix_vdd_notify+0x30/0x94
notifier_call_chain+0x6c/0x8c
blocking_notifier_call_chain+0x48/0x70
_notifier_call_chain.isra.0+0x18/0x20
_regulator_enable+0xc0/0x178
regulator_enable+0x40/0x7c
goodix_i2c_hid_power_up+0x18/0x20
i2c_hid_core_power_up.isra.0+0x1c/0x2c
i2c_hid_core_probe+0xd8/0x3d4
i2c_hid_of_goodix_probe+0x14c/0x158
i2c_device_probe+0x25c/0x270
really_probe+0x174/0x2cc
__driver_probe_device+0xc0/0xd8
driver_probe_device+0x50/0xe4
__device_attach_driver+0xa8/0xc0
bus_for_each_drv+0x9c/0xc0
__device_attach_async_helper+0x6c/0xbc
async_run_entry_fn+0x38/0x100
process_one_work+0x294/0x438
worker_thread+0x180/0x258
kthread+0x120/0x130
ret_from_fork+0x10/0x20
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&(&rdev->notifier)->rwsem);
lock(&ihid_goodix->regulator_mutex);
lock(&(&rdev->notifier)->rwsem);
lock(&ihid_goodix->regulator_mutex);
*** DEADLOCK ***
Signed-off-by: Daniel Thompson <[email protected]>
---
drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 28 +++++++++++--------------
1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
index b4dad66fa954d..ec6c73f75ffe0 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
@@ -27,7 +27,6 @@ struct i2c_hid_of_goodix {
struct regulator *vdd;
struct notifier_block nb;
- struct mutex regulator_mutex;
struct gpio_desc *reset_gpio;
const struct goodix_i2c_hid_timing_data *timings;
};
@@ -67,8 +66,6 @@ static int ihid_goodix_vdd_notify(struct notifier_block *nb,
container_of(nb, struct i2c_hid_of_goodix, nb);
int ret = NOTIFY_OK;
- mutex_lock(&ihid_goodix->regulator_mutex);
-
switch (event) {
case REGULATOR_EVENT_PRE_DISABLE:
gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
@@ -87,8 +84,6 @@ static int ihid_goodix_vdd_notify(struct notifier_block *nb,
break;
}
- mutex_unlock(&ihid_goodix->regulator_mutex);
-
return ret;
}
@@ -102,8 +97,6 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client,
if (!ihid_goodix)
return -ENOMEM;
- mutex_init(&ihid_goodix->regulator_mutex);
-
ihid_goodix->ops.power_up = goodix_i2c_hid_power_up;
ihid_goodix->ops.power_down = goodix_i2c_hid_power_down;
@@ -130,25 +123,28 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client,
* long. Holding the controller in reset apparently draws extra
* power.
*/
- mutex_lock(&ihid_goodix->regulator_mutex);
ihid_goodix->nb.notifier_call = ihid_goodix_vdd_notify;
ret = devm_regulator_register_notifier(ihid_goodix->vdd, &ihid_goodix->nb);
- if (ret) {
- mutex_unlock(&ihid_goodix->regulator_mutex);
+ if (ret)
return dev_err_probe(&client->dev, ret,
"regulator notifier request failed\n");
- }
/*
* If someone else is holding the regulator on (or the regulator is
* an always-on one) we might never be told to deassert reset. Do it
- * now. Here we'll assume that someone else might have _just
- * barely_ turned the regulator on so we'll do the full
- * "post_power_delay" just in case.
+ * now... and temporarily bump the regulator reference count just to
+ * make sure it is impossible for this to race with our own notifier!
+ * We also assume that someone else might have _just barely_ turned
+ * the regulator on so we'll do the full "post_power_delay" just in
+ * case.
*/
- if (ihid_goodix->reset_gpio && regulator_is_enabled(ihid_goodix->vdd))
+ if (ihid_goodix->reset_gpio && regulator_is_enabled(ihid_goodix->vdd)) {
+ ret = regulator_enable(ihid_goodix->vdd);
+ if (ret)
+ return ret;
goodix_i2c_hid_deassert_reset(ihid_goodix, true);
- mutex_unlock(&ihid_goodix->regulator_mutex);
+ regulator_disable(ihid_goodix->vdd);
+ }
return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
}
base-commit: e783362eb54cd99b2cac8b3a9aeac942e6f6ac07
--
2.34.1
Hi,
On Fri, Jan 28, 2022 at 9:48 AM Daniel Thompson
<[email protected]> wrote:
>
> I'm was on the receiving end of a lockdep splat from this driver and after
> scratching my head I couldn't be entirely sure it was a false positive
> given we would also have to think about whether the regulator locking is
> safe (since the notifier is called whilst holding regulator locks which
> are also needed for regulator_is_enabled() ).
>
> Regardless of whether it is a real bug or not, the mutex isn't needed.
> We can use reference counting tricks instead to avoid races with the
> notifier calls.
>
> The observed splat follows:
>
> ------------------------------------------------------
> kworker/u16:3/127 is trying to acquire lock:
> ffff00008021fb20 (&ihid_goodix->regulator_mutex){+.+.}-{4:4}, at: ihid_goodix_vdd_notify+0x30/0x94
>
> but task is already holding lock:
> ffff0000835c60c0 (&(&rdev->notifier)->rwsem){++++}-{4:4}, at: blocking_notifier_call_chain+0x30/0x70
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (&(&rdev->notifier)->rwsem){++++}-{4:4}:
> down_write+0x68/0x8c
> blocking_notifier_chain_register+0x54/0x70
> regulator_register_notifier+0x1c/0x24
> devm_regulator_register_notifier+0x58/0x98
> i2c_hid_of_goodix_probe+0xdc/0x158
> i2c_device_probe+0x25d/0x270
> really_probe+0x174/0x2cc
> __driver_probe_device+0xc0/0xd8
> driver_probe_device+0x50/0xe4
> __device_attach_driver+0xa8/0xc0
> bus_for_each_drv+0x9c/0xc0
> __device_attach_async_helper+0x6c/0xbc
> async_run_entry_fn+0x38/0x100
> process_one_work+0x294/0x438
> worker_thread+0x180/0x258
> kthread+0x120/0x130
> ret_from_fork+0x10/0x20
>
> -> #0 (&ihid_goodix->regulator_mutex){+.+.}-{4:4}:
> __lock_acquire+0xd24/0xfe8
> lock_acquire+0x288/0x2f4
> __mutex_lock+0xa0/0x338
> mutex_lock_nested+0x3c/0x5c
> ihid_goodix_vdd_notify+0x30/0x94
> notifier_call_chain+0x6c/0x8c
> blocking_notifier_call_chain+0x48/0x70
> _notifier_call_chain.isra.0+0x18/0x20
> _regulator_enable+0xc0/0x178
> regulator_enable+0x40/0x7c
> goodix_i2c_hid_power_up+0x18/0x20
> i2c_hid_core_power_up.isra.0+0x1c/0x2c
> i2c_hid_core_probe+0xd8/0x3d4
> i2c_hid_of_goodix_probe+0x14c/0x158
> i2c_device_probe+0x25c/0x270
> really_probe+0x174/0x2cc
> __driver_probe_device+0xc0/0xd8
> driver_probe_device+0x50/0xe4
> __device_attach_driver+0xa8/0xc0
> bus_for_each_drv+0x9c/0xc0
> __device_attach_async_helper+0x6c/0xbc
> async_run_entry_fn+0x38/0x100
> process_one_work+0x294/0x438
> worker_thread+0x180/0x258
> kthread+0x120/0x130
> ret_from_fork+0x10/0x20
>
> other info that might help us debug this:
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&(&rdev->notifier)->rwsem);
> lock(&ihid_goodix->regulator_mutex);
> lock(&(&rdev->notifier)->rwsem);
> lock(&ihid_goodix->regulator_mutex);
>
> *** DEADLOCK ***
>
> Signed-off-by: Daniel Thompson <[email protected]>
> ---
> drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 28 +++++++++++--------------
> 1 file changed, 12 insertions(+), 16 deletions(-)
Yes, this seems like a reasonable solution, thanks! Probably want:
Fixes: 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to true
state of the regulator")
Reviewed-by: Douglas Anderson <[email protected]>
On Fri, 28 Jan 2022, Doug Anderson wrote:
> > I'm was on the receiving end of a lockdep splat from this driver and after
> > scratching my head I couldn't be entirely sure it was a false positive
> > given we would also have to think about whether the regulator locking is
> > safe (since the notifier is called whilst holding regulator locks which
> > are also needed for regulator_is_enabled() ).
[ ... snip ... ]
> > Signed-off-by: Daniel Thompson <[email protected]>
> > ---
> > drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 28 +++++++++++--------------
> > 1 file changed, 12 insertions(+), 16 deletions(-)
>
> Yes, this seems like a reasonable solution, thanks! Probably want:
>
> Fixes: 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to true
> state of the regulator")
>
> Reviewed-by: Douglas Anderson <[email protected]>
Thanks, queued for 5.17.
--
Jiri Kosina
SUSE Labs