| 1 |
From 2787710f73fcce4a9bdab540aaf1aef778a27462 Mon Sep 17 00:00:00 2001 |
| 2 |
From: Daniel Thompson <daniel.thompson@linaro.org> |
| 3 |
Date: Fri, 28 Jan 2022 17:46:25 +0000 |
| 4 |
Subject: HID: i2c-hid: goodix: Fix a lockdep splat |
| 5 |
|
| 6 |
From: Daniel Thompson <daniel.thompson@linaro.org> |
| 7 |
|
| 8 |
commit 2787710f73fcce4a9bdab540aaf1aef778a27462 upstream. |
| 9 |
|
| 10 |
I'm was on the receiving end of a lockdep splat from this driver and after |
| 11 |
scratching my head I couldn't be entirely sure it was a false positive |
| 12 |
given we would also have to think about whether the regulator locking is |
| 13 |
safe (since the notifier is called whilst holding regulator locks which |
| 14 |
are also needed for regulator_is_enabled() ). |
| 15 |
|
| 16 |
Regardless of whether it is a real bug or not, the mutex isn't needed. |
| 17 |
We can use reference counting tricks instead to avoid races with the |
| 18 |
notifier calls. |
| 19 |
|
| 20 |
The observed splat follows: |
| 21 |
|
| 22 |
------------------------------------------------------ |
| 23 |
kworker/u16:3/127 is trying to acquire lock: |
| 24 |
ffff00008021fb20 (&ihid_goodix->regulator_mutex){+.+.}-{4:4}, at: ihid_goodix_vdd_notify+0x30/0x94 |
| 25 |
|
| 26 |
but task is already holding lock: |
| 27 |
ffff0000835c60c0 (&(&rdev->notifier)->rwsem){++++}-{4:4}, at: blocking_notifier_call_chain+0x30/0x70 |
| 28 |
|
| 29 |
which lock already depends on the new lock. |
| 30 |
|
| 31 |
the existing dependency chain (in reverse order) is: |
| 32 |
|
| 33 |
-> #1 (&(&rdev->notifier)->rwsem){++++}-{4:4}: |
| 34 |
down_write+0x68/0x8c |
| 35 |
blocking_notifier_chain_register+0x54/0x70 |
| 36 |
regulator_register_notifier+0x1c/0x24 |
| 37 |
devm_regulator_register_notifier+0x58/0x98 |
| 38 |
i2c_hid_of_goodix_probe+0xdc/0x158 |
| 39 |
i2c_device_probe+0x25d/0x270 |
| 40 |
really_probe+0x174/0x2cc |
| 41 |
__driver_probe_device+0xc0/0xd8 |
| 42 |
driver_probe_device+0x50/0xe4 |
| 43 |
__device_attach_driver+0xa8/0xc0 |
| 44 |
bus_for_each_drv+0x9c/0xc0 |
| 45 |
__device_attach_async_helper+0x6c/0xbc |
| 46 |
async_run_entry_fn+0x38/0x100 |
| 47 |
process_one_work+0x294/0x438 |
| 48 |
worker_thread+0x180/0x258 |
| 49 |
kthread+0x120/0x130 |
| 50 |
ret_from_fork+0x10/0x20 |
| 51 |
|
| 52 |
-> #0 (&ihid_goodix->regulator_mutex){+.+.}-{4:4}: |
| 53 |
__lock_acquire+0xd24/0xfe8 |
| 54 |
lock_acquire+0x288/0x2f4 |
| 55 |
__mutex_lock+0xa0/0x338 |
| 56 |
mutex_lock_nested+0x3c/0x5c |
| 57 |
ihid_goodix_vdd_notify+0x30/0x94 |
| 58 |
notifier_call_chain+0x6c/0x8c |
| 59 |
blocking_notifier_call_chain+0x48/0x70 |
| 60 |
_notifier_call_chain.isra.0+0x18/0x20 |
| 61 |
_regulator_enable+0xc0/0x178 |
| 62 |
regulator_enable+0x40/0x7c |
| 63 |
goodix_i2c_hid_power_up+0x18/0x20 |
| 64 |
i2c_hid_core_power_up.isra.0+0x1c/0x2c |
| 65 |
i2c_hid_core_probe+0xd8/0x3d4 |
| 66 |
i2c_hid_of_goodix_probe+0x14c/0x158 |
| 67 |
i2c_device_probe+0x25c/0x270 |
| 68 |
really_probe+0x174/0x2cc |
| 69 |
__driver_probe_device+0xc0/0xd8 |
| 70 |
driver_probe_device+0x50/0xe4 |
| 71 |
__device_attach_driver+0xa8/0xc0 |
| 72 |
bus_for_each_drv+0x9c/0xc0 |
| 73 |
__device_attach_async_helper+0x6c/0xbc |
| 74 |
async_run_entry_fn+0x38/0x100 |
| 75 |
process_one_work+0x294/0x438 |
| 76 |
worker_thread+0x180/0x258 |
| 77 |
kthread+0x120/0x130 |
| 78 |
ret_from_fork+0x10/0x20 |
| 79 |
|
| 80 |
other info that might help us debug this: |
| 81 |
|
| 82 |
Possible unsafe locking scenario: |
| 83 |
|
| 84 |
CPU0 CPU1 |
| 85 |
---- ---- |
| 86 |
lock(&(&rdev->notifier)->rwsem); |
| 87 |
lock(&ihid_goodix->regulator_mutex); |
| 88 |
lock(&(&rdev->notifier)->rwsem); |
| 89 |
lock(&ihid_goodix->regulator_mutex); |
| 90 |
|
| 91 |
*** DEADLOCK *** |
| 92 |
|
| 93 |
Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> |
| 94 |
Fixes: 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to true state of the regulator") |
| 95 |
Reviewed-by: Douglas Anderson <dianders@chromium.org> |
| 96 |
Signed-off-by: Jiri Kosina <jkosina@suse.cz> |
| 97 |
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| 98 |
--- |
| 99 |
drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 28 ++++++++++++---------------- |
| 100 |
1 file changed, 12 insertions(+), 16 deletions(-) |
| 101 |
|
| 102 |
--- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c |
| 103 |
+++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c |
| 104 |
@@ -27,7 +27,6 @@ struct i2c_hid_of_goodix { |
| 105 |
|
| 106 |
struct regulator *vdd; |
| 107 |
struct notifier_block nb; |
| 108 |
- struct mutex regulator_mutex; |
| 109 |
struct gpio_desc *reset_gpio; |
| 110 |
const struct goodix_i2c_hid_timing_data *timings; |
| 111 |
}; |
| 112 |
@@ -67,8 +66,6 @@ static int ihid_goodix_vdd_notify(struct |
| 113 |
container_of(nb, struct i2c_hid_of_goodix, nb); |
| 114 |
int ret = NOTIFY_OK; |
| 115 |
|
| 116 |
- mutex_lock(&ihid_goodix->regulator_mutex); |
| 117 |
- |
| 118 |
switch (event) { |
| 119 |
case REGULATOR_EVENT_PRE_DISABLE: |
| 120 |
gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1); |
| 121 |
@@ -87,8 +84,6 @@ static int ihid_goodix_vdd_notify(struct |
| 122 |
break; |
| 123 |
} |
| 124 |
|
| 125 |
- mutex_unlock(&ihid_goodix->regulator_mutex); |
| 126 |
- |
| 127 |
return ret; |
| 128 |
} |
| 129 |
|
| 130 |
@@ -102,8 +97,6 @@ static int i2c_hid_of_goodix_probe(struc |
| 131 |
if (!ihid_goodix) |
| 132 |
return -ENOMEM; |
| 133 |
|
| 134 |
- mutex_init(&ihid_goodix->regulator_mutex); |
| 135 |
- |
| 136 |
ihid_goodix->ops.power_up = goodix_i2c_hid_power_up; |
| 137 |
ihid_goodix->ops.power_down = goodix_i2c_hid_power_down; |
| 138 |
|
| 139 |
@@ -130,25 +123,28 @@ static int i2c_hid_of_goodix_probe(struc |
| 140 |
* long. Holding the controller in reset apparently draws extra |
| 141 |
* power. |
| 142 |
*/ |
| 143 |
- mutex_lock(&ihid_goodix->regulator_mutex); |
| 144 |
ihid_goodix->nb.notifier_call = ihid_goodix_vdd_notify; |
| 145 |
ret = devm_regulator_register_notifier(ihid_goodix->vdd, &ihid_goodix->nb); |
| 146 |
- if (ret) { |
| 147 |
- mutex_unlock(&ihid_goodix->regulator_mutex); |
| 148 |
+ if (ret) |
| 149 |
return dev_err_probe(&client->dev, ret, |
| 150 |
"regulator notifier request failed\n"); |
| 151 |
- } |
| 152 |
|
| 153 |
/* |
| 154 |
* If someone else is holding the regulator on (or the regulator is |
| 155 |
* an always-on one) we might never be told to deassert reset. Do it |
| 156 |
- * now. Here we'll assume that someone else might have _just |
| 157 |
- * barely_ turned the regulator on so we'll do the full |
| 158 |
- * "post_power_delay" just in case. |
| 159 |
+ * now... and temporarily bump the regulator reference count just to |
| 160 |
+ * make sure it is impossible for this to race with our own notifier! |
| 161 |
+ * We also assume that someone else might have _just barely_ turned |
| 162 |
+ * the regulator on so we'll do the full "post_power_delay" just in |
| 163 |
+ * case. |
| 164 |
*/ |
| 165 |
- if (ihid_goodix->reset_gpio && regulator_is_enabled(ihid_goodix->vdd)) |
| 166 |
+ if (ihid_goodix->reset_gpio && regulator_is_enabled(ihid_goodix->vdd)) { |
| 167 |
+ ret = regulator_enable(ihid_goodix->vdd); |
| 168 |
+ if (ret) |
| 169 |
+ return ret; |
| 170 |
goodix_i2c_hid_deassert_reset(ihid_goodix, true); |
| 171 |
- mutex_unlock(&ihid_goodix->regulator_mutex); |
| 172 |
+ regulator_disable(ihid_goodix->vdd); |
| 173 |
+ } |
| 174 |
|
| 175 |
return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0); |
| 176 |
} |
Parent Directory
| 
Revision Log