]>
Commit | Line | Data |
---|---|---|
cd033818 SL |
1 | From 116fd01059b57e0a499b2d7fb92d37259efca9c3 Mon Sep 17 00:00:00 2001 |
2 | From: Phong Hoang <phong.hoang.wz@renesas.com> | |
3 | Date: Tue, 19 Mar 2019 19:40:08 +0900 | |
4 | Subject: pwm: Fix deadlock warning when removing PWM device | |
5 | MIME-Version: 1.0 | |
6 | Content-Type: text/plain; charset=UTF-8 | |
7 | Content-Transfer-Encoding: 8bit | |
8 | ||
9 | [ Upstream commit 347ab9480313737c0f1aaa08e8f2e1a791235535 ] | |
10 | ||
11 | This patch fixes deadlock warning if removing PWM device | |
12 | when CONFIG_PROVE_LOCKING is enabled. | |
13 | ||
14 | This issue can be reproceduced by the following steps on | |
15 | the R-Car H3 Salvator-X board if the backlight is disabled: | |
16 | ||
17 | # cd /sys/class/pwm/pwmchip0 | |
18 | # echo 0 > export | |
19 | # ls | |
20 | device export npwm power pwm0 subsystem uevent unexport | |
21 | # cd device/driver | |
22 | # ls | |
23 | bind e6e31000.pwm uevent unbind | |
24 | # echo e6e31000.pwm > unbind | |
25 | ||
26 | [ 87.659974] ====================================================== | |
27 | [ 87.666149] WARNING: possible circular locking dependency detected | |
28 | [ 87.672327] 5.0.0 #7 Not tainted | |
29 | [ 87.675549] ------------------------------------------------------ | |
30 | [ 87.681723] bash/2986 is trying to acquire lock: | |
31 | [ 87.686337] 000000005ea0e178 (kn->count#58){++++}, at: kernfs_remove_by_name_ns+0x50/0xa0 | |
32 | [ 87.694528] | |
33 | [ 87.694528] but task is already holding lock: | |
34 | [ 87.700353] 000000006313b17c (pwm_lock){+.+.}, at: pwmchip_remove+0x28/0x13c | |
35 | [ 87.707405] | |
36 | [ 87.707405] which lock already depends on the new lock. | |
37 | [ 87.707405] | |
38 | [ 87.715574] | |
39 | [ 87.715574] the existing dependency chain (in reverse order) is: | |
40 | [ 87.723048] | |
41 | [ 87.723048] -> #1 (pwm_lock){+.+.}: | |
42 | [ 87.728017] __mutex_lock+0x70/0x7e4 | |
43 | [ 87.732108] mutex_lock_nested+0x1c/0x24 | |
44 | [ 87.736547] pwm_request_from_chip.part.6+0x34/0x74 | |
45 | [ 87.741940] pwm_request_from_chip+0x20/0x40 | |
46 | [ 87.746725] export_store+0x6c/0x1f4 | |
47 | [ 87.750820] dev_attr_store+0x18/0x28 | |
48 | [ 87.754998] sysfs_kf_write+0x54/0x64 | |
49 | [ 87.759175] kernfs_fop_write+0xe4/0x1e8 | |
50 | [ 87.763615] __vfs_write+0x40/0x184 | |
51 | [ 87.767619] vfs_write+0xa8/0x19c | |
52 | [ 87.771448] ksys_write+0x58/0xbc | |
53 | [ 87.775278] __arm64_sys_write+0x18/0x20 | |
54 | [ 87.779721] el0_svc_common+0xd0/0x124 | |
55 | [ 87.783986] el0_svc_compat_handler+0x1c/0x24 | |
56 | [ 87.788858] el0_svc_compat+0x8/0x18 | |
57 | [ 87.792947] | |
58 | [ 87.792947] -> #0 (kn->count#58){++++}: | |
59 | [ 87.798260] lock_acquire+0xc4/0x22c | |
60 | [ 87.802353] __kernfs_remove+0x258/0x2c4 | |
61 | [ 87.806790] kernfs_remove_by_name_ns+0x50/0xa0 | |
62 | [ 87.811836] remove_files.isra.1+0x38/0x78 | |
63 | [ 87.816447] sysfs_remove_group+0x48/0x98 | |
64 | [ 87.820971] sysfs_remove_groups+0x34/0x4c | |
65 | [ 87.825583] device_remove_attrs+0x6c/0x7c | |
66 | [ 87.830197] device_del+0x11c/0x33c | |
67 | [ 87.834201] device_unregister+0x14/0x2c | |
68 | [ 87.838638] pwmchip_sysfs_unexport+0x40/0x4c | |
69 | [ 87.843509] pwmchip_remove+0xf4/0x13c | |
70 | [ 87.847773] rcar_pwm_remove+0x28/0x34 | |
71 | [ 87.852039] platform_drv_remove+0x24/0x64 | |
72 | [ 87.856651] device_release_driver_internal+0x18c/0x21c | |
73 | [ 87.862391] device_release_driver+0x14/0x1c | |
74 | [ 87.867175] unbind_store+0xe0/0x124 | |
75 | [ 87.871265] drv_attr_store+0x20/0x30 | |
76 | [ 87.875442] sysfs_kf_write+0x54/0x64 | |
77 | [ 87.879618] kernfs_fop_write+0xe4/0x1e8 | |
78 | [ 87.884055] __vfs_write+0x40/0x184 | |
79 | [ 87.888057] vfs_write+0xa8/0x19c | |
80 | [ 87.891887] ksys_write+0x58/0xbc | |
81 | [ 87.895716] __arm64_sys_write+0x18/0x20 | |
82 | [ 87.900154] el0_svc_common+0xd0/0x124 | |
83 | [ 87.904417] el0_svc_compat_handler+0x1c/0x24 | |
84 | [ 87.909289] el0_svc_compat+0x8/0x18 | |
85 | [ 87.913378] | |
86 | [ 87.913378] other info that might help us debug this: | |
87 | [ 87.913378] | |
88 | [ 87.921374] Possible unsafe locking scenario: | |
89 | [ 87.921374] | |
90 | [ 87.927286] CPU0 CPU1 | |
91 | [ 87.931808] ---- ---- | |
92 | [ 87.936331] lock(pwm_lock); | |
93 | [ 87.939293] lock(kn->count#58); | |
94 | [ 87.945120] lock(pwm_lock); | |
95 | [ 87.950599] lock(kn->count#58); | |
96 | [ 87.953908] | |
97 | [ 87.953908] *** DEADLOCK *** | |
98 | [ 87.953908] | |
99 | [ 87.959821] 4 locks held by bash/2986: | |
100 | [ 87.963563] #0: 00000000ace7bc30 (sb_writers#6){.+.+}, at: vfs_write+0x188/0x19c | |
101 | [ 87.971044] #1: 00000000287991b2 (&of->mutex){+.+.}, at: kernfs_fop_write+0xb4/0x1e8 | |
102 | [ 87.978872] #2: 00000000f739d016 (&dev->mutex){....}, at: device_release_driver_internal+0x40/0x21c | |
103 | [ 87.988001] #3: 000000006313b17c (pwm_lock){+.+.}, at: pwmchip_remove+0x28/0x13c | |
104 | [ 87.995481] | |
105 | [ 87.995481] stack backtrace: | |
106 | [ 87.999836] CPU: 0 PID: 2986 Comm: bash Not tainted 5.0.0 #7 | |
107 | [ 88.005489] Hardware name: Renesas Salvator-X board based on r8a7795 ES1.x (DT) | |
108 | [ 88.012791] Call trace: | |
109 | [ 88.015235] dump_backtrace+0x0/0x190 | |
110 | [ 88.018891] show_stack+0x14/0x1c | |
111 | [ 88.022204] dump_stack+0xb0/0xec | |
112 | [ 88.025514] print_circular_bug.isra.32+0x1d0/0x2e0 | |
113 | [ 88.030385] __lock_acquire+0x1318/0x1864 | |
114 | [ 88.034388] lock_acquire+0xc4/0x22c | |
115 | [ 88.037958] __kernfs_remove+0x258/0x2c4 | |
116 | [ 88.041874] kernfs_remove_by_name_ns+0x50/0xa0 | |
117 | [ 88.046398] remove_files.isra.1+0x38/0x78 | |
118 | [ 88.050487] sysfs_remove_group+0x48/0x98 | |
119 | [ 88.054490] sysfs_remove_groups+0x34/0x4c | |
120 | [ 88.058580] device_remove_attrs+0x6c/0x7c | |
121 | [ 88.062671] device_del+0x11c/0x33c | |
122 | [ 88.066154] device_unregister+0x14/0x2c | |
123 | [ 88.070070] pwmchip_sysfs_unexport+0x40/0x4c | |
124 | [ 88.074421] pwmchip_remove+0xf4/0x13c | |
125 | [ 88.078163] rcar_pwm_remove+0x28/0x34 | |
126 | [ 88.081906] platform_drv_remove+0x24/0x64 | |
127 | [ 88.085996] device_release_driver_internal+0x18c/0x21c | |
128 | [ 88.091215] device_release_driver+0x14/0x1c | |
129 | [ 88.095478] unbind_store+0xe0/0x124 | |
130 | [ 88.099048] drv_attr_store+0x20/0x30 | |
131 | [ 88.102704] sysfs_kf_write+0x54/0x64 | |
132 | [ 88.106359] kernfs_fop_write+0xe4/0x1e8 | |
133 | [ 88.110275] __vfs_write+0x40/0x184 | |
134 | [ 88.113757] vfs_write+0xa8/0x19c | |
135 | [ 88.117065] ksys_write+0x58/0xbc | |
136 | [ 88.120374] __arm64_sys_write+0x18/0x20 | |
137 | [ 88.124291] el0_svc_common+0xd0/0x124 | |
138 | [ 88.128034] el0_svc_compat_handler+0x1c/0x24 | |
139 | [ 88.132384] el0_svc_compat+0x8/0x18 | |
140 | ||
141 | The sysfs unexport in pwmchip_remove() is completely asymmetric | |
142 | to what we do in pwmchip_add_with_polarity() and commit 0733424c9ba9 | |
143 | ("pwm: Unexport children before chip removal") is a strong indication | |
144 | that this was wrong to begin with. We should just move | |
145 | pwmchip_sysfs_unexport() where it belongs, which is right after | |
146 | pwmchip_sysfs_unexport_children(). In that case, we do not need | |
147 | separate functions anymore either. | |
148 | ||
149 | We also really want to remove sysfs irrespective of whether or not | |
150 | the chip will be removed as a result of pwmchip_remove(). We can only | |
151 | assume that the driver will be gone after that, so we shouldn't leave | |
152 | any dangling sysfs files around. | |
153 | ||
154 | This warning disappears if we move pwmchip_sysfs_unexport() to | |
155 | the top of pwmchip_remove(), pwmchip_sysfs_unexport_children(). | |
156 | That way it is also outside of the pwm_lock section, which indeed | |
157 | doesn't seem to be needed. | |
158 | ||
159 | Moving the pwmchip_sysfs_export() call outside of that section also | |
160 | seems fine and it'd be perfectly symmetric with pwmchip_remove() again. | |
161 | ||
162 | So, this patch fixes them. | |
163 | ||
164 | Signed-off-by: Phong Hoang <phong.hoang.wz@renesas.com> | |
165 | [shimoda: revise the commit log and code] | |
166 | Fixes: 76abbdde2d95 ("pwm: Add sysfs interface") | |
167 | Fixes: 0733424c9ba9 ("pwm: Unexport children before chip removal") | |
168 | Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> | |
169 | Tested-by: Hoan Nguyen An <na-hoan@jinso.co.jp> | |
170 | Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> | |
171 | Reviewed-by: Simon Horman <horms+renesas@verge.net.au> | |
172 | Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> | |
173 | Signed-off-by: Thierry Reding <thierry.reding@gmail.com> | |
174 | Signed-off-by: Sasha Levin <sashal@kernel.org> | |
175 | --- | |
176 | drivers/pwm/core.c | 10 +++++----- | |
177 | drivers/pwm/sysfs.c | 14 +------------- | |
178 | include/linux/pwm.h | 5 ----- | |
179 | 3 files changed, 6 insertions(+), 23 deletions(-) | |
180 | ||
181 | diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c | |
182 | index ec84ff8ad1b4..6911f9662300 100644 | |
183 | --- a/drivers/pwm/core.c | |
184 | +++ b/drivers/pwm/core.c | |
185 | @@ -284,10 +284,12 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip, | |
186 | if (IS_ENABLED(CONFIG_OF)) | |
187 | of_pwmchip_add(chip); | |
188 | ||
189 | - pwmchip_sysfs_export(chip); | |
190 | - | |
191 | out: | |
192 | mutex_unlock(&pwm_lock); | |
193 | + | |
194 | + if (!ret) | |
195 | + pwmchip_sysfs_export(chip); | |
196 | + | |
197 | return ret; | |
198 | } | |
199 | EXPORT_SYMBOL_GPL(pwmchip_add_with_polarity); | |
200 | @@ -321,7 +323,7 @@ int pwmchip_remove(struct pwm_chip *chip) | |
201 | unsigned int i; | |
202 | int ret = 0; | |
203 | ||
204 | - pwmchip_sysfs_unexport_children(chip); | |
205 | + pwmchip_sysfs_unexport(chip); | |
206 | ||
207 | mutex_lock(&pwm_lock); | |
208 | ||
209 | @@ -341,8 +343,6 @@ int pwmchip_remove(struct pwm_chip *chip) | |
210 | ||
211 | free_pwms(chip); | |
212 | ||
213 | - pwmchip_sysfs_unexport(chip); | |
214 | - | |
215 | out: | |
216 | mutex_unlock(&pwm_lock); | |
217 | return ret; | |
218 | diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c | |
219 | index 375008e2be20..199370e41da9 100644 | |
220 | --- a/drivers/pwm/sysfs.c | |
221 | +++ b/drivers/pwm/sysfs.c | |
222 | @@ -338,19 +338,6 @@ void pwmchip_sysfs_export(struct pwm_chip *chip) | |
223 | } | |
224 | ||
225 | void pwmchip_sysfs_unexport(struct pwm_chip *chip) | |
226 | -{ | |
227 | - struct device *parent; | |
228 | - | |
229 | - parent = class_find_device(&pwm_class, NULL, chip, | |
230 | - pwmchip_sysfs_match); | |
231 | - if (parent) { | |
232 | - /* for class_find_device() */ | |
233 | - put_device(parent); | |
234 | - device_unregister(parent); | |
235 | - } | |
236 | -} | |
237 | - | |
238 | -void pwmchip_sysfs_unexport_children(struct pwm_chip *chip) | |
239 | { | |
240 | struct device *parent; | |
241 | unsigned int i; | |
242 | @@ -368,6 +355,7 @@ void pwmchip_sysfs_unexport_children(struct pwm_chip *chip) | |
243 | } | |
244 | ||
245 | put_device(parent); | |
246 | + device_unregister(parent); | |
247 | } | |
248 | ||
249 | static int __init pwm_sysfs_init(void) | |
250 | diff --git a/include/linux/pwm.h b/include/linux/pwm.h | |
251 | index aa8736d5b2f3..cfc3ed46cad2 100644 | |
252 | --- a/include/linux/pwm.h | |
253 | +++ b/include/linux/pwm.h | |
254 | @@ -331,7 +331,6 @@ static inline void pwm_remove_table(struct pwm_lookup *table, size_t num) | |
255 | #ifdef CONFIG_PWM_SYSFS | |
256 | void pwmchip_sysfs_export(struct pwm_chip *chip); | |
257 | void pwmchip_sysfs_unexport(struct pwm_chip *chip); | |
258 | -void pwmchip_sysfs_unexport_children(struct pwm_chip *chip); | |
259 | #else | |
260 | static inline void pwmchip_sysfs_export(struct pwm_chip *chip) | |
261 | { | |
262 | @@ -340,10 +339,6 @@ static inline void pwmchip_sysfs_export(struct pwm_chip *chip) | |
263 | static inline void pwmchip_sysfs_unexport(struct pwm_chip *chip) | |
264 | { | |
265 | } | |
266 | - | |
267 | -static inline void pwmchip_sysfs_unexport_children(struct pwm_chip *chip) | |
268 | -{ | |
269 | -} | |
270 | #endif /* CONFIG_PWM_SYSFS */ | |
271 | ||
272 | #endif /* __LINUX_PWM_H */ | |
273 | -- | |
274 | 2.20.1 | |
275 |