[PATCH] pwm: Avoid double mutex lock on pwm_enable

[PATCH] pwm: Avoid double mutex lock on pwm_enable

[Anand Moon]

Commit d1cd21427747f15920cd726f5f67a07880e7dee4
(pwm: Set enable state properly on failed call to enable)
introduce double lock of mutex on pwm_enable

Changes fix the following debug lock warning.

[    2.701720] WARNING: CPU: 3 PID: 0 at kernel/locking/mutex.c:526 __mutex_lock_slowpath+0x3bc/0x404()
[    2.701731] DEBUG_LOCKS_WARN_ON(in_interrupt())
[    2.701737] Modules linked in:
[    2.701748] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.4.0-rc1-xu4f #24
[    2.701753] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[    2.701787] [<c0015f48>] (unwind_backtrace) from [<c0012d04>] (show_stack+0x10/0x14)
[    2.701808] [<c0012d04>] (show_stack) from [<c01f83fc>] (dump_stack+0x84/0xc4)
[    2.701824] [<c01f83fc>] (dump_stack) from [<c0023494>] (warn_slowpath_common+0x80/0xb0)
[    2.701836] [<c0023494>] (warn_slowpath_common) from [<c00234f4>] (warn_slowpath_fmt+0x30/0x40)
[    2.701849] [<c00234f4>] (warn_slowpath_fmt) from [<c056e6b8>] (__mutex_lock_slowpath+0x3bc/0x404)
[    2.701864] [<c056e6b8>] (__mutex_lock_slowpath) from [<c056e70c>] (mutex_lock+0xc/0x24)
[    2.701884] [<c056e70c>] (mutex_lock) from [<c0228984>] (pwm_enable+0x20/0x7c)
[    2.701903] [<c0228984>] (pwm_enable) from [<c03f0000>] (led_heartbeat_function+0x74/0x144)
[    2.701919] [<c03f0000>] (led_heartbeat_function) from [<c0074368>] (call_timer_fn+0x24/0x98)
[    2.701932] [<c0074368>] (call_timer_fn) from [<c007453c>] (run_timer_softirq+0x160/0x21c)
[    2.701946] [<c007453c>] (run_timer_softirq) from [<c0026e10>] (__do_softirq+0x110/0x228)
[    2.701959] [<c0026e10>] (__do_softirq) from [<c00271c8>] (irq_exit+0xc0/0xfc)
[    2.701976] [<c00271c8>] (irq_exit) from [<c0065180>] (__handle_domain_irq+0x80/0xec)
[    2.701990] [<c0065180>] (__handle_domain_irq) from [<c0009494>] (gic_handle_irq+0x54/0x94)
[    2.702001] [<c0009494>] (gic_handle_irq) from [<c0013794>] (__irq_svc+0x54/0x90)
[    2.702008] Exception stack(0xee8bdf88 to 0xee8bdfd0)
[    2.702019] df80:                   00000001 00000000 00000000 c001b720 ee8bc000 c0573354
[    2.702031] dfa0: 00000000 00000000 ee8bdfe0 c07f92e4 c08004b4 c08004bc f0806640 ee8bdfd8
[    2.702039] dfc0: c0010180 c0010184 60000013 ffffffff
[    2.702053] [<c0013794>] (__irq_svc) from [<c0010184>] (arch_cpu_idle+0x38/0x3c)
[    2.702073] [<c0010184>] (arch_cpu_idle) from [<c0058ed4>] (cpu_startup_entry+0x1ec/0x270)
[    2.702086] [<c0058ed4>] (cpu_startup_entry) from [<4000956c>] (0x4000956c)
[    2.702093] ---[ end trace 539af70491f4f1a9 ]---

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/pwm/core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index d24ca5f..b8f035a 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -506,16 +506,12 @@ int pwm_enable(struct pwm_device *pwm)
 	if (!pwm)
 		return -EINVAL;
 
-	mutex_lock(&pwm->lock);
-
 	if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
 		err = pwm->chip->ops->enable(pwm->chip, pwm);
 		if (err)
 			clear_bit(PWMF_ENABLED, &pwm->flags);
 	}
 
-	mutex_unlock(&pwm->lock);
-
 	return err;
 }
 EXPORT_SYMBOL_GPL(pwm_enable);
-- 
2.1.4

Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable

[Krzysztof Kozlowski]

W dniu 21.11.2015 o 01:59, Anand Moon pisze:
> Commit d1cd21427747f15920cd726f5f67a07880e7dee4
> (pwm: Set enable state properly on failed call to enable)
> introduce double lock of mutex on pwm_enable
> 
> Changes fix the following debug lock warning.
> 
> [    2.701720] WARNING: CPU: 3 PID: 0 at kernel/locking/mutex.c:526 __mutex_lock_slowpath+0x3bc/0x404()
> [    2.701731] DEBUG_LOCKS_WARN_ON(in_interrupt())

Hi Anand!

Yeah, I am hitting this as well. Annoying. However your description is
inaccurate. Double lock of mutex does not cause warnings of sleeping or
locking in atomic context (if you build with debug sleep atomic you will
see different warning). More over you are partially reverting the commit
d1cd21427747 ("pwm: Set enable state properly on failed call to enable")
without proper explanation:
1. why this locking is inappropriate;
2. why it is safe to remove the mutex_lock().

Please find the real problem, fix the real problem and throughly explain
the real issue.

I was suspecting some weird changes in timers. For !irqsafe timer I
think it is safe to use mutexes... but apparently not. Maybe a work
should be scheduled from function called by timer?

Warning with debug atomic sleep:
[   16.047517] BUG: sleeping function called from invalid context at
../kernel/locking/mutex.c:617
[   16.054866] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
[   16.061402] INFO: lockdep is turned off.
[   16.065281] Preemption disabled at:[<  (null)>]   (null)
[   16.070524]
[   16.072002] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
4.4.0-rc1-00040-g28c429565d4f #290
[   16.080150] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[   16.086215] [<c0016780>] (unwind_backtrace) from [<c00132f0>]
(show_stack+0x10/0x14)
[   16.093938] [<c00132f0>] (show_stack) from [<c0223ba4>]
(dump_stack+0x70/0xbc)
[   16.101122] [<c0223ba4>] (dump_stack) from [<c05ed8e0>]
(mutex_lock_nested+0x24/0x474)
[   16.109009] [<c05ed8e0>] (mutex_lock_nested) from [<c0259154>]
(pwm_enable+0x20/0x7c)
[   16.116799] [<c0259154>] (pwm_enable) from [<c04400bc>]
(led_heartbeat_function+0xdc/0x140)
[   16.125119] [<c04400bc>] (led_heartbeat_function) from [<c00864c8>]
(call_timer_fn+0x6c/0xf4)
[   16.133606] [<c00864c8>] (call_timer_fn) from [<c00866f8>]
(run_timer_softirq+0x1a8/0x230)
[   16.141846] [<c00866f8>] (run_timer_softirq) from [<c0028e68>]
(__do_softirq+0x134/0x2c0)
[   16.149982] [<c0028e68>] (__do_softirq) from [<c0029334>]
(irq_exit+0xd0/0x114)
[   16.157255] [<c0029334>] (irq_exit) from [<c0076610>]
(__handle_domain_irq+0x70/0xe4)
[   16.165056] [<c0076610>] (__handle_domain_irq) from [<c00094e8>]
(gic_handle_irq+0x4c/0x94)
[   16.173376] [<c00094e8>] (gic_handle_irq) from [<c0013db8>]
(__irq_svc+0x58/0x98)
[   16.180817] Exception stack(0xc08cdf58 to 0xc08cdfa0)
[   16.185823] df40:
   c0010dcc 00000000
[   16.193997] df60: c08cdfa8 00000000 c05f57c4 00000000 c08ca520
c08ce4bc c08c7364 c08ce4b4
[   16.202141] df80: c09121ca 00000000 00000001 c08cdfa8 c0010dcc
c0010dd0 600f0013 ffffffff
[   16.210291] [<c0013db8>] (__irq_svc) from [<c0010dd0>]
(arch_cpu_idle+0x20/0x3c)
[   16.217661] [<c0010dd0>] (arch_cpu_idle) from [<c0063174>]
(cpu_startup_entry+0x17c/0x26c)
[   16.225899] [<c0063174>] (cpu_startup_entry) from [<c0860c40>]
(start_kernel+0x368/0x3d0)

Best regards,
Krzysztof


> [    2.701737] Modules linked in:
> [    2.701748] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.4.0-rc1-xu4f #24
> [    2.701753] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [    2.701787] [<c0015f48>] (unwind_backtrace) from [<c0012d04>] (show_stack+0x10/0x14)
> [    2.701808] [<c0012d04>] (show_stack) from [<c01f83fc>] (dump_stack+0x84/0xc4)
> [    2.701824] [<c01f83fc>] (dump_stack) from [<c0023494>] (warn_slowpath_common+0x80/0xb0)
> [    2.701836] [<c0023494>] (warn_slowpath_common) from [<c00234f4>] (warn_slowpath_fmt+0x30/0x40)
> [    2.701849] [<c00234f4>] (warn_slowpath_fmt) from [<c056e6b8>] (__mutex_lock_slowpath+0x3bc/0x404)
> [    2.701864] [<c056e6b8>] (__mutex_lock_slowpath) from [<c056e70c>] (mutex_lock+0xc/0x24)
> [    2.701884] [<c056e70c>] (mutex_lock) from [<c0228984>] (pwm_enable+0x20/0x7c)
> [    2.701903] [<c0228984>] (pwm_enable) from [<c03f0000>] (led_heartbeat_function+0x74/0x144)
> [    2.701919] [<c03f0000>] (led_heartbeat_function) from [<c0074368>] (call_timer_fn+0x24/0x98)
> [    2.701932] [<c0074368>] (call_timer_fn) from [<c007453c>] (run_timer_softirq+0x160/0x21c)
> [    2.701946] [<c007453c>] (run_timer_softirq) from [<c0026e10>] (__do_softirq+0x110/0x228)
> [    2.701959] [<c0026e10>] (__do_softirq) from [<c00271c8>] (irq_exit+0xc0/0xfc)
> [    2.701976] [<c00271c8>] (irq_exit) from [<c0065180>] (__handle_domain_irq+0x80/0xec)
> [    2.701990] [<c0065180>] (__handle_domain_irq) from [<c0009494>] (gic_handle_irq+0x54/0x94)
> [    2.702001] [<c0009494>] (gic_handle_irq) from [<c0013794>] (__irq_svc+0x54/0x90)
> [    2.702008] Exception stack(0xee8bdf88 to 0xee8bdfd0)
> [    2.702019] df80:                   00000001 00000000 00000000 c001b720 ee8bc000 c0573354
> [    2.702031] dfa0: 00000000 00000000 ee8bdfe0 c07f92e4 c08004b4 c08004bc f0806640 ee8bdfd8
> [    2.702039] dfc0: c0010180 c0010184 60000013 ffffffff
> [    2.702053] [<c0013794>] (__irq_svc) from [<c0010184>] (arch_cpu_idle+0x38/0x3c)
> [    2.702073] [<c0010184>] (arch_cpu_idle) from [<c0058ed4>] (cpu_startup_entry+0x1ec/0x270)
> [    2.702086] [<c0058ed4>] (cpu_startup_entry) from [<4000956c>] (0x4000956c)
> [    2.702093] ---[ end trace 539af70491f4f1a9 ]---
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  drivers/pwm/core.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index d24ca5f..b8f035a 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -506,16 +506,12 @@ int pwm_enable(struct pwm_device *pwm)
>  	if (!pwm)
>  		return -EINVAL;
>  
> -	mutex_lock(&pwm->lock);
> -
>  	if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
>  		err = pwm->chip->ops->enable(pwm->chip, pwm);
>  		if (err)
>  			clear_bit(PWMF_ENABLED, &pwm->flags);
>  	}
>  
> -	mutex_unlock(&pwm->lock);
> -
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(pwm_enable);
> 

Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable

[Anand Moon]

[-- Attachment #1: Type: text/plain, Size: 7929 bytes --]

hi ​Krzysztof,

On 21 November 2015 at 09:56, Krzysztof Kozlowski <k.kozlowski@samsung.com>
wrote:

> W dniu 21.11.2015 o 01:59, Anand Moon pisze:
> > Commit d1cd21427747f15920cd726f5f67a07880e7dee4
> > (pwm: Set enable state properly on failed call to enable)
> > introduce double lock of mutex on pwm_enable
> >
> > Changes fix the following debug lock warning.
> >
> > [    2.701720] WARNING: CPU: 3 PID: 0 at kernel/locking/mutex.c:526
> __mutex_lock_slowpath+0x3bc/0x404()
> > [    2.701731] DEBUG_LOCKS_WARN_ON(in_interrupt())
>
> Hi Anand!
>
> Yeah, I am hitting this as well. Annoying. However your description is
> inaccurate. Double lock of mutex does not cause warnings of sleeping or
> locking in atomic context (if you build with debug sleep atomic you will
> see different warning). More over you are partially reverting the commit
> d1cd21427747 ("pwm: Set enable state properly on failed call to enable")
> without proper explanation:
> 1. why this locking is inappropriate;
> 2. why it is safe to remove the mutex_lock().
>
> Please find the real problem, fix the real problem and throughly explain
> the real issue.
>
> I was suspecting some weird changes in timers. For !irqsafe timer I
> think it is safe to use mutexes... but apparently not. Maybe a work
> should be scheduled from function called by timer?
>
> Warning with debug atomic sleep:
> [   16.047517] BUG: sleeping function called from invalid context at
> ../kernel/locking/mutex.c:617
> [   16.054866] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
> [   16.061402] INFO: lockdep is turned off.
> [   16.065281] Preemption disabled at:[<  (null)>]   (null)
> [   16.070524]
> [   16.072002] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
> 4.4.0-rc1-00040-g28c429565d4f #290
> [   16.080150] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [   16.086215] [<c0016780>] (unwind_backtrace) from [<c00132f0>]
> (show_stack+0x10/0x14)
> [   16.093938] [<c00132f0>] (show_stack) from [<c0223ba4>]
> (dump_stack+0x70/0xbc)
> [   16.101122] [<c0223ba4>] (dump_stack) from [<c05ed8e0>]
> (mutex_lock_nested+0x24/0x474)
> [   16.109009] [<c05ed8e0>] (mutex_lock_nested) from [<c0259154>]
> (pwm_enable+0x20/0x7c)
> [   16.116799] [<c0259154>] (pwm_enable) from [<c04400bc>]
> (led_heartbeat_function+0xdc/0x140)
> [   16.125119] [<c04400bc>] (led_heartbeat_function) from [<c00864c8>]
> (call_timer_fn+0x6c/0xf4)
> [   16.133606] [<c00864c8>] (call_timer_fn) from [<c00866f8>]
> (run_timer_softirq+0x1a8/0x230)
> [   16.141846] [<c00866f8>] (run_timer_softirq) from [<c0028e68>]
> (__do_softirq+0x134/0x2c0)
> [   16.149982] [<c0028e68>] (__do_softirq) from [<c0029334>]
> (irq_exit+0xd0/0x114)
> [   16.157255] [<c0029334>] (irq_exit) from [<c0076610>]
> (__handle_domain_irq+0x70/0xe4)
> [   16.165056] [<c0076610>] (__handle_domain_irq) from [<c00094e8>]
> (gic_handle_irq+0x4c/0x94)
> [   16.173376] [<c00094e8>] (gic_handle_irq) from [<c0013db8>]
> (__irq_svc+0x58/0x98)
> [   16.180817] Exception stack(0xc08cdf58 to 0xc08cdfa0)
> [   16.185823] df40:
>    c0010dcc 00000000
> [   16.193997] df60: c08cdfa8 00000000 c05f57c4 00000000 c08ca520
> c08ce4bc c08c7364 c08ce4b4
> [   16.202141] df80: c09121ca 00000000 00000001 c08cdfa8 c0010dcc
> c0010dd0 600f0013 ffffffff
> [   16.210291] [<c0013db8>] (__irq_svc) from [<c0010dd0>]
> (arch_cpu_idle+0x20/0x3c)
> [   16.217661] [<c0010dd0>] (arch_cpu_idle) from [<c0063174>]
> (cpu_startup_entry+0x17c/0x26c)
> [   16.225899] [<c0063174>] (cpu_startup_entry) from [<c0860c40>]
> (start_kernel+0x368/0x3d0)
>
> Best regards,
> ​​
> Krzysztof
>
>
> > [    2.701737] Modules linked in:
> > [    2.701748] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.4.0-rc1-xu4f
> #24
> > [    2.701753] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> > [    2.701787] [<c0015f48>] (unwind_backtrace) from [<c0012d04>]
> (show_stack+0x10/0x14)
> > [    2.701808] [<c0012d04>] (show_stack) from [<c01f83fc>]
> (dump_stack+0x84/0xc4)
> > [    2.701824] [<c01f83fc>] (dump_stack) from [<c0023494>]
> (warn_slowpath_common+0x80/0xb0)
> > [    2.701836] [<c0023494>] (warn_slowpath_common) from [<c00234f4>]
> (warn_slowpath_fmt+0x30/0x40)
> > [    2.701849] [<c00234f4>] (warn_slowpath_fmt) from [<c056e6b8>]
> (__mutex_lock_slowpath+0x3bc/0x404)
> > [    2.701864] [<c056e6b8>] (__mutex_lock_slowpath) from [<c056e70c>]
> (mutex_lock+0xc/0x24)
> > [    2.701884] [<c056e70c>] (mutex_lock) from [<c0228984>]
> (pwm_enable+0x20/0x7c)
> > [    2.701903] [<c0228984>] (pwm_enable) from [<c03f0000>]
> (led_heartbeat_function+0x74/0x144)
> > [    2.701919] [<c03f0000>] (led_heartbeat_function) from [<c0074368>]
> (call_timer_fn+0x24/0x98)
> > [    2.701932] [<c0074368>] (call_timer_fn) from [<c007453c>]
> (run_timer_softirq+0x160/0x21c)
> > [    2.701946] [<c007453c>] (run_timer_softirq) from [<c0026e10>]
> (__do_softirq+0x110/0x228)
> > [    2.701959] [<c0026e10>] (__do_softirq) from [<c00271c8>]
> (irq_exit+0xc0/0xfc)
> > [    2.701976] [<c00271c8>] (irq_exit) from [<c0065180>]
> (__handle_domain_irq+0x80/0xec)
> > [    2.701990] [<c0065180>] (__handle_domain_irq) from [<c0009494>]
> (gic_handle_irq+0x54/0x94)
> > [    2.702001] [<c0009494>] (gic_handle_irq) from [<c0013794>]
> (__irq_svc+0x54/0x90)
> > [    2.702008] Exception stack(0xee8bdf88 to 0xee8bdfd0)
> > [    2.702019] df80:                   00000001 00000000 00000000
> c001b720 ee8bc000 c0573354
> > [    2.702031] dfa0: 00000000 00000000 ee8bdfe0 c07f92e4 c08004b4
> c08004bc f0806640 ee8bdfd8
> > [    2.702039] dfc0: c0010180 c0010184 60000013 ffffffff
> > [    2.702053] [<c0013794>] (__irq_svc) from [<c0010184>]
> (arch_cpu_idle+0x38/0x3c)
> > [    2.702073] [<c0010184>] (arch_cpu_idle) from [<c0058ed4>]
> (cpu_startup_entry+0x1ec/0x270)
> > [    2.702086] [<c0058ed4>] (cpu_startup_entry) from [<4000956c>]
> (0x4000956c)
> > [    2.702093] ---[ end trace 539af70491f4f1a9 ]---
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> >  drivers/pwm/core.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index d24ca5f..b8f035a 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -506,16 +506,12 @@ int pwm_enable(struct pwm_device *pwm)
> >       if (!pwm)
> >               return -EINVAL;
> >
> > -     mutex_lock(&pwm->lock);
> > -
> >       if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
> >               err = pwm->chip->ops->enable(pwm->chip, pwm);
> >               if (err)
> >                       clear_bit(PWMF_ENABLED, &pwm->flags);
> >       }
> >
> > -     mutex_unlock(&pwm->lock);
> > -
> >       return err;
> >  }
> >  EXPORT_SYMBOL_GPL(pwm_enable);
> >
>

​Adding Jonathan Richardson.​

​Yes I am ​

aware I am reverting some part of the ​d1cd21427747 ("pwm: Set enable state
properly on failed call to enable")

Please take a look at this below commit.

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pwm/core.c?id=d1cd21427747f15920cd726f5f67a07880e7dee4

Actually reverting this change it work fine.

This changes introduce the new mutex lock pwm->lock to protect enabled bit
by drivers while setting polarity.

Well pwm_set_polarity already acquires the pwm->lock and calls
pwm_is_enabled function.
Again within pwm_is_enabled we are trying to acquire the same mutex lock.
I am just trying to remove this double mutex locking of pwm->lock.

Well I have tested this changes with CONFIG_DEBUG_ATOMIC_SLEEP=y
and I did not find any more above mutex WARM / timer opps.

I will update the commit message with the reason of the fix.
I will take close look if I can fix it differently.
Thanks for you inputs.

Please share your thoughts on this.

-Anand Moon

[-- Attachment #2: Type: text/html, Size: 11041 bytes --]

Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable

[Krzysztof Kozlowski]

2015-11-21 18:40 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
> hi Krzysztof,
>
> On 21 November 2015 at 09:56, Krzysztof Kozlowski <k.kozlowski@samsung.com>
> wrote:
>>
>> W dniu 21.11.2015 o 01:59, Anand Moon pisze:
>> > Commit d1cd21427747f15920cd726f5f67a07880e7dee4
>> > (pwm: Set enable state properly on failed call to enable)
>> > introduce double lock of mutex on pwm_enable
>> >
>> > Changes fix the following debug lock warning.
>> >
>> > [    2.701720] WARNING: CPU: 3 PID: 0 at kernel/locking/mutex.c:526
>> > __mutex_lock_slowpath+0x3bc/0x404()
>> > [    2.701731] DEBUG_LOCKS_WARN_ON(in_interrupt())
>>
>> Hi Anand!
>>
>> Yeah, I am hitting this as well. Annoying. However your description is
>> inaccurate. Double lock of mutex does not cause warnings of sleeping or
>> locking in atomic context (if you build with debug sleep atomic you will
>> see different warning). More over you are partially reverting the commit
>> d1cd21427747 ("pwm: Set enable state properly on failed call to enable")
>> without proper explanation:
>> 1. why this locking is inappropriate;
>> 2. why it is safe to remove the mutex_lock().
>>
>> Please find the real problem, fix the real problem and throughly explain
>> the real issue.
>>
>> I was suspecting some weird changes in timers. For !irqsafe timer I
>> think it is safe to use mutexes... but apparently not. Maybe a work
>> should be scheduled from function called by timer?
>>
>> Warning with debug atomic sleep:
>> [   16.047517] BUG: sleeping function called from invalid context at
>> ../kernel/locking/mutex.c:617
>> [   16.054866] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
>> [   16.061402] INFO: lockdep is turned off.
>> [   16.065281] Preemption disabled at:[<  (null)>]   (null)
>> [   16.070524]
>> [   16.072002] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
>> 4.4.0-rc1-00040-g28c429565d4f #290
>> [   16.080150] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>> [   16.086215] [<c0016780>] (unwind_backtrace) from [<c00132f0>]
>> (show_stack+0x10/0x14)
>> [   16.093938] [<c00132f0>] (show_stack) from [<c0223ba4>]
>> (dump_stack+0x70/0xbc)
>> [   16.101122] [<c0223ba4>] (dump_stack) from [<c05ed8e0>]
>> (mutex_lock_nested+0x24/0x474)
>> [   16.109009] [<c05ed8e0>] (mutex_lock_nested) from [<c0259154>]
>> (pwm_enable+0x20/0x7c)
>> [   16.116799] [<c0259154>] (pwm_enable) from [<c04400bc>]
>> (led_heartbeat_function+0xdc/0x140)
>> [   16.125119] [<c04400bc>] (led_heartbeat_function) from [<c00864c8>]
>> (call_timer_fn+0x6c/0xf4)
>> [   16.133606] [<c00864c8>] (call_timer_fn) from [<c00866f8>]
>> (run_timer_softirq+0x1a8/0x230)
>> [   16.141846] [<c00866f8>] (run_timer_softirq) from [<c0028e68>]
>> (__do_softirq+0x134/0x2c0)
>> [   16.149982] [<c0028e68>] (__do_softirq) from [<c0029334>]
>> (irq_exit+0xd0/0x114)
>> [   16.157255] [<c0029334>] (irq_exit) from [<c0076610>]
>> (__handle_domain_irq+0x70/0xe4)
>> [   16.165056] [<c0076610>] (__handle_domain_irq) from [<c00094e8>]
>> (gic_handle_irq+0x4c/0x94)
>> [   16.173376] [<c00094e8>] (gic_handle_irq) from [<c0013db8>]
>> (__irq_svc+0x58/0x98)
>> [   16.180817] Exception stack(0xc08cdf58 to 0xc08cdfa0)
>> [   16.185823] df40:
>>    c0010dcc 00000000
>> [   16.193997] df60: c08cdfa8 00000000 c05f57c4 00000000 c08ca520
>> c08ce4bc c08c7364 c08ce4b4
>> [   16.202141] df80: c09121ca 00000000 00000001 c08cdfa8 c0010dcc
>> c0010dd0 600f0013 ffffffff
>> [   16.210291] [<c0013db8>] (__irq_svc) from [<c0010dd0>]
>> (arch_cpu_idle+0x20/0x3c)
>> [   16.217661] [<c0010dd0>] (arch_cpu_idle) from [<c0063174>]
>> (cpu_startup_entry+0x17c/0x26c)
>> [   16.225899] [<c0063174>] (cpu_startup_entry) from [<c0860c40>]
>> (start_kernel+0x368/0x3d0)
>>
>> Best regards,
>> Krzysztof
>>
>>
>> > [    2.701737] Modules linked in:
>> > [    2.701748] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.4.0-rc1-xu4f
>> > #24
>> > [    2.701753] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>> > [    2.701787] [<c0015f48>] (unwind_backtrace) from [<c0012d04>]
>> > (show_stack+0x10/0x14)
>> > [    2.701808] [<c0012d04>] (show_stack) from [<c01f83fc>]
>> > (dump_stack+0x84/0xc4)
>> > [    2.701824] [<c01f83fc>] (dump_stack) from [<c0023494>]
>> > (warn_slowpath_common+0x80/0xb0)
>> > [    2.701836] [<c0023494>] (warn_slowpath_common) from [<c00234f4>]
>> > (warn_slowpath_fmt+0x30/0x40)
>> > [    2.701849] [<c00234f4>] (warn_slowpath_fmt) from [<c056e6b8>]
>> > (__mutex_lock_slowpath+0x3bc/0x404)
>> > [    2.701864] [<c056e6b8>] (__mutex_lock_slowpath) from [<c056e70c>]
>> > (mutex_lock+0xc/0x24)
>> > [    2.701884] [<c056e70c>] (mutex_lock) from [<c0228984>]
>> > (pwm_enable+0x20/0x7c)
>> > [    2.701903] [<c0228984>] (pwm_enable) from [<c03f0000>]
>> > (led_heartbeat_function+0x74/0x144)
>> > [    2.701919] [<c03f0000>] (led_heartbeat_function) from [<c0074368>]
>> > (call_timer_fn+0x24/0x98)
>> > [    2.701932] [<c0074368>] (call_timer_fn) from [<c007453c>]
>> > (run_timer_softirq+0x160/0x21c)
>> > [    2.701946] [<c007453c>] (run_timer_softirq) from [<c0026e10>]
>> > (__do_softirq+0x110/0x228)
>> > [    2.701959] [<c0026e10>] (__do_softirq) from [<c00271c8>]
>> > (irq_exit+0xc0/0xfc)
>> > [    2.701976] [<c00271c8>] (irq_exit) from [<c0065180>]
>> > (__handle_domain_irq+0x80/0xec)
>> > [    2.701990] [<c0065180>] (__handle_domain_irq) from [<c0009494>]
>> > (gic_handle_irq+0x54/0x94)
>> > [    2.702001] [<c0009494>] (gic_handle_irq) from [<c0013794>]
>> > (__irq_svc+0x54/0x90)
>> > [    2.702008] Exception stack(0xee8bdf88 to 0xee8bdfd0)
>> > [    2.702019] df80:                   00000001 00000000 00000000
>> > c001b720 ee8bc000 c0573354
>> > [    2.702031] dfa0: 00000000 00000000 ee8bdfe0 c07f92e4 c08004b4
>> > c08004bc f0806640 ee8bdfd8
>> > [    2.702039] dfc0: c0010180 c0010184 60000013 ffffffff
>> > [    2.702053] [<c0013794>] (__irq_svc) from [<c0010184>]
>> > (arch_cpu_idle+0x38/0x3c)
>> > [    2.702073] [<c0010184>] (arch_cpu_idle) from [<c0058ed4>]
>> > (cpu_startup_entry+0x1ec/0x270)
>> > [    2.702086] [<c0058ed4>] (cpu_startup_entry) from [<4000956c>]
>> > (0x4000956c)
>> > [    2.702093] ---[ end trace 539af70491f4f1a9 ]---
>> >
>> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>> > ---
>> >  drivers/pwm/core.c | 4 ----
>> >  1 file changed, 4 deletions(-)
>> >
>> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>> > index d24ca5f..b8f035a 100644
>> > --- a/drivers/pwm/core.c
>> > +++ b/drivers/pwm/core.c
>> > @@ -506,16 +506,12 @@ int pwm_enable(struct pwm_device *pwm)
>> >       if (!pwm)
>> >               return -EINVAL;
>> >
>> > -     mutex_lock(&pwm->lock);
>> > -
>> >       if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
>> >               err = pwm->chip->ops->enable(pwm->chip, pwm);
>> >               if (err)
>> >                       clear_bit(PWMF_ENABLED, &pwm->flags);
>> >       }
>> >
>> > -     mutex_unlock(&pwm->lock);
>> > -
>> >       return err;
>> >  }
>> >  EXPORT_SYMBOL_GPL(pwm_enable);
>> >
>
>
> Adding Jonathan Richardson.
>
> Yes I am
>
> aware I am reverting some part of the d1cd21427747 ("pwm: Set enable state
> properly on failed call to enable")
>
> Please take a look at this below commit.
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pwm/core.c?id=d1cd21427747f15920cd726f5f67a07880e7dee4
>
> Actually reverting this change it work fine.
>
> This changes introduce the new mutex lock pwm->lock to protect enabled bit
> by drivers while setting polarity.
>
> Well pwm_set_polarity already acquires the pwm->lock and calls
> pwm_is_enabled function.
> Again within pwm_is_enabled we are trying to acquire the same mutex lock.

You are describing a lockdown by trying to acquire the same mutex twice.

However pwm_is_enabled() does not acquire mutex.

Again, please look at generated warnings:
1. BUG: sleeping function called from invalid context
2. DEBUG_LOCKS_WARN_ON(in_interrupt())

They are not related anyhow to what you described (acquiring already
locked mutex).

Best regards,
Krzysztof

Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable

[Anand Moon]

[-- Attachment #1: Type: text/plain, Size: 9577 bytes --]

Hi krzysztof,

On 21 November 2015 at 15:22, Krzysztof Kozlowski <k.kozlowski@samsung.com>
wrote:

> 2015-11-21 18:40 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
> > hi Krzysztof,
> >
> > On 21 November 2015 at 09:56, Krzysztof Kozlowski <
> k.kozlowski@samsung.com>
> > wrote:
> >>
> >> W dniu 21.11.2015 o 01:59, Anand Moon pisze:
> >> > Commit d1cd21427747f15920cd726f5f67a07880e7dee4
> >> > (pwm: Set enable state properly on failed call to enable)
> >> > introduce double lock of mutex on pwm_enable
> >> >
> >> > Changes fix the following debug lock warning.
> >> >
> >> > [    2.701720] WARNING: CPU: 3 PID: 0 at kernel/locking/mutex.c:526
> >> > __mutex_lock_slowpath+0x3bc/0x404()
> >> > [    2.701731] DEBUG_LOCKS_WARN_ON(in_interrupt())
> >>
> >> Hi Anand!
> >>
> >> Yeah, I am hitting this as well. Annoying. However your description is
> >> inaccurate. Double lock of mutex does not cause warnings of sleeping or
> >> locking in atomic context (if you build with debug sleep atomic you will
> >> see different warning). More over you are partially reverting the commit
> >> d1cd21427747 ("pwm: Set enable state properly on failed call to enable")
> >> without proper explanation:
> >> 1. why this locking is inappropriate;
> >> 2. why it is safe to remove the mutex_lock().
> >>
> >> Please find the real problem, fix the real problem and throughly explain
> >> the real issue.
> >>
> >> I was suspecting some weird changes in timers. For !irqsafe timer I
> >> think it is safe to use mutexes... but apparently not. Maybe a work
> >> should be scheduled from function called by timer?
> >>
> >> Warning with debug atomic sleep:
> >> [   16.047517] BUG: sleeping function called from invalid context at
> >> ../kernel/locking/mutex.c:617
> >> [   16.054866] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name:
> swapper/0
> >> [   16.061402] INFO: lockdep is turned off.
> >> [   16.065281] Preemption disabled at:[<  (null)>]   (null)
> >> [   16.070524]
> >> [   16.072002] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
> >> 4.4.0-rc1-00040-g28c429565d4f #290
> >> [   16.080150] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> >> [   16.086215] [<c0016780>] (unwind_backtrace) from [<c00132f0>]
> >> (show_stack+0x10/0x14)
> >> [   16.093938] [<c00132f0>] (show_stack) from [<c0223ba4>]
> >> (dump_stack+0x70/0xbc)
> >> [   16.101122] [<c0223ba4>] (dump_stack) from [<c05ed8e0>]
> >> (mutex_lock_nested+0x24/0x474)
> >> [   16.109009] [<c05ed8e0>] (mutex_lock_nested) from [<c0259154>]
> >> (pwm_enable+0x20/0x7c)
> >> [   16.116799] [<c0259154>] (pwm_enable) from [<c04400bc>]
> >> (led_heartbeat_function+0xdc/0x140)
> >> [   16.125119] [<c04400bc>] (led_heartbeat_function) from [<c00864c8>]
> >> (call_timer_fn+0x6c/0xf4)
> >> [   16.133606] [<c00864c8>] (call_timer_fn) from [<c00866f8>]
> >> (run_timer_softirq+0x1a8/0x230)
> >> [   16.141846] [<c00866f8>] (run_timer_softirq) from [<c0028e68>]
> >> (__do_softirq+0x134/0x2c0)
> >> [   16.149982] [<c0028e68>] (__do_softirq) from [<c0029334>]
> >> (irq_exit+0xd0/0x114)
> >> [   16.157255] [<c0029334>] (irq_exit) from [<c0076610>]
> >> (__handle_domain_irq+0x70/0xe4)
> >> [   16.165056] [<c0076610>] (__handle_domain_irq) from [<c00094e8>]
> >> (gic_handle_irq+0x4c/0x94)
> >> [   16.173376] [<c00094e8>] (gic_handle_irq) from [<c0013db8>]
> >> (__irq_svc+0x58/0x98)
> >> [   16.180817] Exception stack(0xc08cdf58 to 0xc08cdfa0)
> >> [   16.185823] df40:
> >>    c0010dcc 00000000
> >> [   16.193997] df60: c08cdfa8 00000000 c05f57c4 00000000 c08ca520
> >> c08ce4bc c08c7364 c08ce4b4
> >> [   16.202141] df80: c09121ca 00000000 00000001 c08cdfa8 c0010dcc
> >> c0010dd0 600f0013 ffffffff
> >> [   16.210291] [<c0013db8>] (__irq_svc) from [<c0010dd0>]
> >> (arch_cpu_idle+0x20/0x3c)
> >> [   16.217661] [<c0010dd0>] (arch_cpu_idle) from [<c0063174>]
> >> (cpu_startup_entry+0x17c/0x26c)
> >> [   16.225899] [<c0063174>] (cpu_startup_entry) from [<c0860c40>]
> >> (start_kernel+0x368/0x3d0)
> >>
> >> Best regards,
> >> Krzysztof
> >>
> >>
> >> > [    2.701737] Modules linked in:
> >> > [    2.701748] CPU: 3 PID: 0 Comm: swapper/3 Not tainted
> 4.4.0-rc1-xu4f
> >> > #24
> >> > [    2.701753] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> >> > [    2.701787] [<c0015f48>] (unwind_backtrace) from [<c0012d04>]
> >> > (show_stack+0x10/0x14)
> >> > [    2.701808] [<c0012d04>] (show_stack) from [<c01f83fc>]
> >> > (dump_stack+0x84/0xc4)
> >> > [    2.701824] [<c01f83fc>] (dump_stack) from [<c0023494>]
> >> > (warn_slowpath_common+0x80/0xb0)
> >> > [    2.701836] [<c0023494>] (warn_slowpath_common) from [<c00234f4>]
> >> > (warn_slowpath_fmt+0x30/0x40)
> >> > [    2.701849] [<c00234f4>] (warn_slowpath_fmt) from [<c056e6b8>]
> >> > (__mutex_lock_slowpath+0x3bc/0x404)
> >> > [    2.701864] [<c056e6b8>] (__mutex_lock_slowpath) from [<c056e70c>]
> >> > (mutex_lock+0xc/0x24)
> >> > [    2.701884] [<c056e70c>] (mutex_lock) from [<c0228984>]
> >> > (pwm_enable+0x20/0x7c)
> >> > [    2.701903] [<c0228984>] (pwm_enable) from [<c03f0000>]
> >> > (led_heartbeat_function+0x74/0x144)
> >> > [    2.701919] [<c03f0000>] (led_heartbeat_function) from [<c0074368>]
> >> > (call_timer_fn+0x24/0x98)
> >> > [    2.701932] [<c0074368>] (call_timer_fn) from [<c007453c>]
> >> > (run_timer_softirq+0x160/0x21c)
> >> > [    2.701946] [<c007453c>] (run_timer_softirq) from [<c0026e10>]
> >> > (__do_softirq+0x110/0x228)
> >> > [    2.701959] [<c0026e10>] (__do_softirq) from [<c00271c8>]
> >> > (irq_exit+0xc0/0xfc)
> >> > [    2.701976] [<c00271c8>] (irq_exit) from [<c0065180>]
> >> > (__handle_domain_irq+0x80/0xec)
> >> > [    2.701990] [<c0065180>] (__handle_domain_irq) from [<c0009494>]
> >> > (gic_handle_irq+0x54/0x94)
> >> > [    2.702001] [<c0009494>] (gic_handle_irq) from [<c0013794>]
> >> > (__irq_svc+0x54/0x90)
> >> > [    2.702008] Exception stack(0xee8bdf88 to 0xee8bdfd0)
> >> > [    2.702019] df80:                   00000001 00000000 00000000
> >> > c001b720 ee8bc000 c0573354
> >> > [    2.702031] dfa0: 00000000 00000000 ee8bdfe0 c07f92e4 c08004b4
> >> > c08004bc f0806640 ee8bdfd8
> >> > [    2.702039] dfc0: c0010180 c0010184 60000013 ffffffff
> >> > [    2.702053] [<c0013794>] (__irq_svc) from [<c0010184>]
> >> > (arch_cpu_idle+0x38/0x3c)
> >> > [    2.702073] [<c0010184>] (arch_cpu_idle) from [<c0058ed4>]
> >> > (cpu_startup_entry+0x1ec/0x270)
> >> > [    2.702086] [<c0058ed4>] (cpu_startup_entry) from [<4000956c>]
> >> > (0x4000956c)
> >> > [    2.702093] ---[ end trace 539af70491f4f1a9 ]---
> >> >
> >> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> >> > ---
> >> >  drivers/pwm/core.c | 4 ----
> >> >  1 file changed, 4 deletions(-)
> >> >
> >> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> >> > index d24ca5f..b8f035a 100644
> >> > --- a/drivers/pwm/core.c
> >> > +++ b/drivers/pwm/core.c
> >> > @@ -506,16 +506,12 @@ int pwm_enable(struct pwm_device *pwm)
> >> >       if (!pwm)
> >> >               return -EINVAL;
> >> >
> >> > -     mutex_lock(&pwm->lock);
> >> > -
> >> >       if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
> >> >               err = pwm->chip->ops->enable(pwm->chip, pwm);
> >> >               if (err)
> >> >                       clear_bit(PWMF_ENABLED, &pwm->flags);
> >> >       }
> >> >
> >> > -     mutex_unlock(&pwm->lock);
> >> > -
> >> >       return err;
> >> >  }
> >> >  EXPORT_SYMBOL_GPL(pwm_enable);
> >> >
> >
> >
> > Adding Jonathan Richardson.
> >
> > Yes I am
> >
> > aware I am reverting some part of the d1cd21427747 ("pwm: Set enable
> state
> > properly on failed call to enable")
> >
> > Please take a look at this below commit.
> >
> >
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pwm/core.c?id=d1cd21427747f15920cd726f5f67a07880e7dee4
> >
> > Actually reverting this change it work fine.
> >
> > This changes introduce the new mutex lock pwm->lock to protect enabled
> bit
> > by drivers while setting polarity.
> >
> > Well pwm_set_polarity already acquires the pwm->lock and calls
> > pwm_is_enabled function.
> > Again within pwm_is_enabled we are trying to acquire the same mutex lock.
>
> You are describing a lockdown by trying to acquire the same mutex twice.
>
> However pwm_is_enabled() does not acquire mutex.
>
> Again, please look at generated warnings:
> 1. BUG: sleeping function called from invalid context
> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
>
> They are not related anyhow to what you described (acquiring already
> locked mutex).
>
> Best regards,
> Krzysztof
>

​Please take a look at the snip of the commit
d1cd21427747f15920cd726f5f67a07880e7dee4

@@ -494,10 +501,22 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
*/
int pwm_enable(struct pwm_device *pwm)
{
- if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags))
- return pwm->chip->ops->enable(pwm->chip, pwm);
+ int err = 0;
+
+ if (!pwm)
+ return -EINVAL;
+
+ mutex_lock(&pwm->lock);
+
+ if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
+ err = pwm->chip->ops->enable(pwm->chip, pwm);
+ if (err)
+ clear_bit(PWMF_ENABLED, &pwm->flags);
+ }
+
+ mutex_unlock(&pwm->lock);
- return pwm ? 0 : -EINVAL;
+ return err;
}
EXPORT_SYMBOL_GPL(pwm_enable);

Yes it a lock down of mutex.

I don't know how the backtrace will get generate.
1 (BUG: sleeping function called from invalid context)
2 DEBUG_LOCKS_WARN_ON(in_interrupt())

-Anand Moon

[-- Attachment #2: Type: text/html, Size: 13349 bytes --]

Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable

[Anand Moon]

hi Krzysztof,

On 21 November 2015 at 15:22, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> 2015-11-21 18:40 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>> hi Krzysztof,
>>
>> On 21 November 2015 at 09:56, Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> wrote:
>>>
>>> W dniu 21.11.2015 o 01:59, Anand Moon pisze:
>>> > Commit d1cd21427747f15920cd726f5f67a07880e7dee4
>>> > (pwm: Set enable state properly on failed call to enable)
>>> > introduce double lock of mutex on pwm_enable
>>> >
>>> > Changes fix the following debug lock warning.
>>> >
>>> > [    2.701720] WARNING: CPU: 3 PID: 0 at kernel/locking/mutex.c:526
>>> > __mutex_lock_slowpath+0x3bc/0x404()
>>> > [    2.701731] DEBUG_LOCKS_WARN_ON(in_interrupt())
>>>
>>> Hi Anand!
>>>
>>> Yeah, I am hitting this as well. Annoying. However your description is
>>> inaccurate. Double lock of mutex does not cause warnings of sleeping or
>>> locking in atomic context (if you build with debug sleep atomic you will
>>> see different warning). More over you are partially reverting the commit
>>> d1cd21427747 ("pwm: Set enable state properly on failed call to enable")
>>> without proper explanation:
>>> 1. why this locking is inappropriate;
>>> 2. why it is safe to remove the mutex_lock().
>>>
>>> Please find the real problem, fix the real problem and throughly explain
>>> the real issue.
>>>
>>> I was suspecting some weird changes in timers. For !irqsafe timer I
>>> think it is safe to use mutexes... but apparently not. Maybe a work
>>> should be scheduled from function called by timer?
>>>
>>> Warning with debug atomic sleep:
>>> [   16.047517] BUG: sleeping function called from invalid context at
>>> ../kernel/locking/mutex.c:617
>>> [   16.054866] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
>>> [   16.061402] INFO: lockdep is turned off.
>>> [   16.065281] Preemption disabled at:[<  (null)>]   (null)
>>> [   16.070524]
>>> [   16.072002] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
>>> 4.4.0-rc1-00040-g28c429565d4f #290
>>> [   16.080150] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>> [   16.086215] [<c0016780>] (unwind_backtrace) from [<c00132f0>]
>>> (show_stack+0x10/0x14)
>>> [   16.093938] [<c00132f0>] (show_stack) from [<c0223ba4>]
>>> (dump_stack+0x70/0xbc)
>>> [   16.101122] [<c0223ba4>] (dump_stack) from [<c05ed8e0>]
>>> (mutex_lock_nested+0x24/0x474)
>>> [   16.109009] [<c05ed8e0>] (mutex_lock_nested) from [<c0259154>]
>>> (pwm_enable+0x20/0x7c)
>>> [   16.116799] [<c0259154>] (pwm_enable) from [<c04400bc>]
>>> (led_heartbeat_function+0xdc/0x140)
>>> [   16.125119] [<c04400bc>] (led_heartbeat_function) from [<c00864c8>]
>>> (call_timer_fn+0x6c/0xf4)
>>> [   16.133606] [<c00864c8>] (call_timer_fn) from [<c00866f8>]
>>> (run_timer_softirq+0x1a8/0x230)
>>> [   16.141846] [<c00866f8>] (run_timer_softirq) from [<c0028e68>]
>>> (__do_softirq+0x134/0x2c0)
>>> [   16.149982] [<c0028e68>] (__do_softirq) from [<c0029334>]
>>> (irq_exit+0xd0/0x114)
>>> [   16.157255] [<c0029334>] (irq_exit) from [<c0076610>]
>>> (__handle_domain_irq+0x70/0xe4)
>>> [   16.165056] [<c0076610>] (__handle_domain_irq) from [<c00094e8>]
>>> (gic_handle_irq+0x4c/0x94)
>>> [   16.173376] [<c00094e8>] (gic_handle_irq) from [<c0013db8>]
>>> (__irq_svc+0x58/0x98)
>>> [   16.180817] Exception stack(0xc08cdf58 to 0xc08cdfa0)
>>> [   16.185823] df40:
>>>    c0010dcc 00000000
>>> [   16.193997] df60: c08cdfa8 00000000 c05f57c4 00000000 c08ca520
>>> c08ce4bc c08c7364 c08ce4b4
>>> [   16.202141] df80: c09121ca 00000000 00000001 c08cdfa8 c0010dcc
>>> c0010dd0 600f0013 ffffffff
>>> [   16.210291] [<c0013db8>] (__irq_svc) from [<c0010dd0>]
>>> (arch_cpu_idle+0x20/0x3c)
>>> [   16.217661] [<c0010dd0>] (arch_cpu_idle) from [<c0063174>]
>>> (cpu_startup_entry+0x17c/0x26c)
>>> [   16.225899] [<c0063174>] (cpu_startup_entry) from [<c0860c40>]
>>> (start_kernel+0x368/0x3d0)
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>>
>>> > [    2.701737] Modules linked in:
>>> > [    2.701748] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.4.0-rc1-xu4f
>>> > #24
>>> > [    2.701753] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>> > [    2.701787] [<c0015f48>] (unwind_backtrace) from [<c0012d04>]
>>> > (show_stack+0x10/0x14)
>>> > [    2.701808] [<c0012d04>] (show_stack) from [<c01f83fc>]
>>> > (dump_stack+0x84/0xc4)
>>> > [    2.701824] [<c01f83fc>] (dump_stack) from [<c0023494>]
>>> > (warn_slowpath_common+0x80/0xb0)
>>> > [    2.701836] [<c0023494>] (warn_slowpath_common) from [<c00234f4>]
>>> > (warn_slowpath_fmt+0x30/0x40)
>>> > [    2.701849] [<c00234f4>] (warn_slowpath_fmt) from [<c056e6b8>]
>>> > (__mutex_lock_slowpath+0x3bc/0x404)
>>> > [    2.701864] [<c056e6b8>] (__mutex_lock_slowpath) from [<c056e70c>]
>>> > (mutex_lock+0xc/0x24)
>>> > [    2.701884] [<c056e70c>] (mutex_lock) from [<c0228984>]
>>> > (pwm_enable+0x20/0x7c)
>>> > [    2.701903] [<c0228984>] (pwm_enable) from [<c03f0000>]
>>> > (led_heartbeat_function+0x74/0x144)
>>> > [    2.701919] [<c03f0000>] (led_heartbeat_function) from [<c0074368>]
>>> > (call_timer_fn+0x24/0x98)
>>> > [    2.701932] [<c0074368>] (call_timer_fn) from [<c007453c>]
>>> > (run_timer_softirq+0x160/0x21c)
>>> > [    2.701946] [<c007453c>] (run_timer_softirq) from [<c0026e10>]
>>> > (__do_softirq+0x110/0x228)
>>> > [    2.701959] [<c0026e10>] (__do_softirq) from [<c00271c8>]
>>> > (irq_exit+0xc0/0xfc)
>>> > [    2.701976] [<c00271c8>] (irq_exit) from [<c0065180>]
>>> > (__handle_domain_irq+0x80/0xec)
>>> > [    2.701990] [<c0065180>] (__handle_domain_irq) from [<c0009494>]
>>> > (gic_handle_irq+0x54/0x94)
>>> > [    2.702001] [<c0009494>] (gic_handle_irq) from [<c0013794>]
>>> > (__irq_svc+0x54/0x90)
>>> > [    2.702008] Exception stack(0xee8bdf88 to 0xee8bdfd0)
>>> > [    2.702019] df80:                   00000001 00000000 00000000
>>> > c001b720 ee8bc000 c0573354
>>> > [    2.702031] dfa0: 00000000 00000000 ee8bdfe0 c07f92e4 c08004b4
>>> > c08004bc f0806640 ee8bdfd8
>>> > [    2.702039] dfc0: c0010180 c0010184 60000013 ffffffff
>>> > [    2.702053] [<c0013794>] (__irq_svc) from [<c0010184>]
>>> > (arch_cpu_idle+0x38/0x3c)
>>> > [    2.702073] [<c0010184>] (arch_cpu_idle) from [<c0058ed4>]
>>> > (cpu_startup_entry+0x1ec/0x270)
>>> > [    2.702086] [<c0058ed4>] (cpu_startup_entry) from [<4000956c>]
>>> > (0x4000956c)
>>> > [    2.702093] ---[ end trace 539af70491f4f1a9 ]---
>>> >
>>> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>> > ---
>>> >  drivers/pwm/core.c | 4 ----
>>> >  1 file changed, 4 deletions(-)
>>> >
>>> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>>> > index d24ca5f..b8f035a 100644
>>> > --- a/drivers/pwm/core.c
>>> > +++ b/drivers/pwm/core.c
>>> > @@ -506,16 +506,12 @@ int pwm_enable(struct pwm_device *pwm)
>>> >       if (!pwm)
>>> >               return -EINVAL;
>>> >
>>> > -     mutex_lock(&pwm->lock);
>>> > -
>>> >       if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
>>> >               err = pwm->chip->ops->enable(pwm->chip, pwm);
>>> >               if (err)
>>> >                       clear_bit(PWMF_ENABLED, &pwm->flags);
>>> >       }
>>> >
>>> > -     mutex_unlock(&pwm->lock);
>>> > -
>>> >       return err;
>>> >  }
>>> >  EXPORT_SYMBOL_GPL(pwm_enable);
>>> >
>>
>>
>> Adding Jonathan Richardson.
>>
>> Yes I am
>>
>> aware I am reverting some part of the d1cd21427747 ("pwm: Set enable state
>> properly on failed call to enable")
>>
>> Please take a look at this below commit.
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pwm/core.c?id=d1cd21427747f15920cd726f5f67a07880e7dee4
>>
>> Actually reverting this change it work fine.
>>
>> This changes introduce the new mutex lock pwm->lock to protect enabled bit
>> by drivers while setting polarity.
>>
>> Well pwm_set_polarity already acquires the pwm->lock and calls
>> pwm_is_enabled function.
>> Again within pwm_is_enabled we are trying to acquire the same mutex lock.
>
> You are describing a lockdown by trying to acquire the same mutex twice.
>
> However pwm_is_enabled() does not acquire mutex.
>
> Again, please look at generated warnings:
> 1. BUG: sleeping function called from invalid context
> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
>
> They are not related anyhow to what you described (acquiring already
> locked mutex).
>
> Best regards,
> Krzysztof

My last reply mail went in HTML format so resend this.

First it was a typo on my part.
It not pwm_is_enabled function its pwm_enabled.

Please take a look at the snip of the commit
d1cd21427747f15920cd726f5f67a07880e7dee4

@@ -494,10 +501,22 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
*/
int pwm_enable(struct pwm_device *pwm)
{
- if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags))
- return pwm->chip->ops->enable(pwm->chip, pwm);
+ int err = 0;
+
+ if (!pwm)
+ return -EINVAL;
+
+ mutex_lock(&pwm->lock);
+
+ if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
+ err = pwm->chip->ops->enable(pwm->chip, pwm);
+ if (err)
+ clear_bit(PWMF_ENABLED, &pwm->flags);
+ }
+
+ mutex_unlock(&pwm->lock);
- return pwm ? 0 : -EINVAL;
+ return err;
}
EXPORT_SYMBOL_GPL(pwm_enable);

Yes it a lockdown of mutex, trying to acquire same mutex

I don't know how the backtrace will get generate.
1 (BUG: sleeping function called from invalid context)
2 DEBUG_LOCKS_WARN_ON(in_interrupt())

-Anand Moon

Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable

[Krzysztof Kozlowski]

2015-11-21 21:11 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
> hi Krzysztof,
>
> On 21 November 2015 at 15:22, Krzysztof Kozlowski
> <k.kozlowski@samsung.com> wrote:
>> 2015-11-21 18:40 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>> hi Krzysztof,
>>>
>>> On 21 November 2015 at 09:56, Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>> wrote:
>>>>
>>>> W dniu 21.11.2015 o 01:59, Anand Moon pisze:
>>>> > Commit d1cd21427747f15920cd726f5f67a07880e7dee4
>>>> > (pwm: Set enable state properly on failed call to enable)
>>>> > introduce double lock of mutex on pwm_enable
>>>> >
>>>> > Changes fix the following debug lock warning.
>>>> >
>>>> > [    2.701720] WARNING: CPU: 3 PID: 0 at kernel/locking/mutex.c:526
>>>> > __mutex_lock_slowpath+0x3bc/0x404()
>>>> > [    2.701731] DEBUG_LOCKS_WARN_ON(in_interrupt())
>>>>
>>>> Hi Anand!
>>>>
>>>> Yeah, I am hitting this as well. Annoying. However your description is
>>>> inaccurate. Double lock of mutex does not cause warnings of sleeping or
>>>> locking in atomic context (if you build with debug sleep atomic you will
>>>> see different warning). More over you are partially reverting the commit
>>>> d1cd21427747 ("pwm: Set enable state properly on failed call to enable")
>>>> without proper explanation:
>>>> 1. why this locking is inappropriate;
>>>> 2. why it is safe to remove the mutex_lock().
>>>>
>>>> Please find the real problem, fix the real problem and throughly explain
>>>> the real issue.
>>>>
>>>> I was suspecting some weird changes in timers. For !irqsafe timer I
>>>> think it is safe to use mutexes... but apparently not. Maybe a work
>>>> should be scheduled from function called by timer?
>>>>
>>>> Warning with debug atomic sleep:
>>>> [   16.047517] BUG: sleeping function called from invalid context at
>>>> ../kernel/locking/mutex.c:617
>>>> [   16.054866] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
>>>> [   16.061402] INFO: lockdep is turned off.
>>>> [   16.065281] Preemption disabled at:[<  (null)>]   (null)
>>>> [   16.070524]
>>>> [   16.072002] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
>>>> 4.4.0-rc1-00040-g28c429565d4f #290
>>>> [   16.080150] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>> [   16.086215] [<c0016780>] (unwind_backtrace) from [<c00132f0>]
>>>> (show_stack+0x10/0x14)
>>>> [   16.093938] [<c00132f0>] (show_stack) from [<c0223ba4>]
>>>> (dump_stack+0x70/0xbc)
>>>> [   16.101122] [<c0223ba4>] (dump_stack) from [<c05ed8e0>]
>>>> (mutex_lock_nested+0x24/0x474)
>>>> [   16.109009] [<c05ed8e0>] (mutex_lock_nested) from [<c0259154>]
>>>> (pwm_enable+0x20/0x7c)
>>>> [   16.116799] [<c0259154>] (pwm_enable) from [<c04400bc>]
>>>> (led_heartbeat_function+0xdc/0x140)
>>>> [   16.125119] [<c04400bc>] (led_heartbeat_function) from [<c00864c8>]
>>>> (call_timer_fn+0x6c/0xf4)
>>>> [   16.133606] [<c00864c8>] (call_timer_fn) from [<c00866f8>]
>>>> (run_timer_softirq+0x1a8/0x230)
>>>> [   16.141846] [<c00866f8>] (run_timer_softirq) from [<c0028e68>]
>>>> (__do_softirq+0x134/0x2c0)
>>>> [   16.149982] [<c0028e68>] (__do_softirq) from [<c0029334>]
>>>> (irq_exit+0xd0/0x114)
>>>> [   16.157255] [<c0029334>] (irq_exit) from [<c0076610>]
>>>> (__handle_domain_irq+0x70/0xe4)
>>>> [   16.165056] [<c0076610>] (__handle_domain_irq) from [<c00094e8>]
>>>> (gic_handle_irq+0x4c/0x94)
>>>> [   16.173376] [<c00094e8>] (gic_handle_irq) from [<c0013db8>]
>>>> (__irq_svc+0x58/0x98)
>>>> [   16.180817] Exception stack(0xc08cdf58 to 0xc08cdfa0)
>>>> [   16.185823] df40:
>>>>    c0010dcc 00000000
>>>> [   16.193997] df60: c08cdfa8 00000000 c05f57c4 00000000 c08ca520
>>>> c08ce4bc c08c7364 c08ce4b4
>>>> [   16.202141] df80: c09121ca 00000000 00000001 c08cdfa8 c0010dcc
>>>> c0010dd0 600f0013 ffffffff
>>>> [   16.210291] [<c0013db8>] (__irq_svc) from [<c0010dd0>]
>>>> (arch_cpu_idle+0x20/0x3c)
>>>> [   16.217661] [<c0010dd0>] (arch_cpu_idle) from [<c0063174>]
>>>> (cpu_startup_entry+0x17c/0x26c)
>>>> [   16.225899] [<c0063174>] (cpu_startup_entry) from [<c0860c40>]
>>>> (start_kernel+0x368/0x3d0)
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>>
>>>> > [    2.701737] Modules linked in:
>>>> > [    2.701748] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.4.0-rc1-xu4f
>>>> > #24
>>>> > [    2.701753] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>> > [    2.701787] [<c0015f48>] (unwind_backtrace) from [<c0012d04>]
>>>> > (show_stack+0x10/0x14)
>>>> > [    2.701808] [<c0012d04>] (show_stack) from [<c01f83fc>]
>>>> > (dump_stack+0x84/0xc4)
>>>> > [    2.701824] [<c01f83fc>] (dump_stack) from [<c0023494>]
>>>> > (warn_slowpath_common+0x80/0xb0)
>>>> > [    2.701836] [<c0023494>] (warn_slowpath_common) from [<c00234f4>]
>>>> > (warn_slowpath_fmt+0x30/0x40)
>>>> > [    2.701849] [<c00234f4>] (warn_slowpath_fmt) from [<c056e6b8>]
>>>> > (__mutex_lock_slowpath+0x3bc/0x404)
>>>> > [    2.701864] [<c056e6b8>] (__mutex_lock_slowpath) from [<c056e70c>]
>>>> > (mutex_lock+0xc/0x24)
>>>> > [    2.701884] [<c056e70c>] (mutex_lock) from [<c0228984>]
>>>> > (pwm_enable+0x20/0x7c)
>>>> > [    2.701903] [<c0228984>] (pwm_enable) from [<c03f0000>]
>>>> > (led_heartbeat_function+0x74/0x144)
>>>> > [    2.701919] [<c03f0000>] (led_heartbeat_function) from [<c0074368>]
>>>> > (call_timer_fn+0x24/0x98)
>>>> > [    2.701932] [<c0074368>] (call_timer_fn) from [<c007453c>]
>>>> > (run_timer_softirq+0x160/0x21c)
>>>> > [    2.701946] [<c007453c>] (run_timer_softirq) from [<c0026e10>]
>>>> > (__do_softirq+0x110/0x228)
>>>> > [    2.701959] [<c0026e10>] (__do_softirq) from [<c00271c8>]
>>>> > (irq_exit+0xc0/0xfc)
>>>> > [    2.701976] [<c00271c8>] (irq_exit) from [<c0065180>]
>>>> > (__handle_domain_irq+0x80/0xec)
>>>> > [    2.701990] [<c0065180>] (__handle_domain_irq) from [<c0009494>]
>>>> > (gic_handle_irq+0x54/0x94)
>>>> > [    2.702001] [<c0009494>] (gic_handle_irq) from [<c0013794>]
>>>> > (__irq_svc+0x54/0x90)
>>>> > [    2.702008] Exception stack(0xee8bdf88 to 0xee8bdfd0)
>>>> > [    2.702019] df80:                   00000001 00000000 00000000
>>>> > c001b720 ee8bc000 c0573354
>>>> > [    2.702031] dfa0: 00000000 00000000 ee8bdfe0 c07f92e4 c08004b4
>>>> > c08004bc f0806640 ee8bdfd8
>>>> > [    2.702039] dfc0: c0010180 c0010184 60000013 ffffffff
>>>> > [    2.702053] [<c0013794>] (__irq_svc) from [<c0010184>]
>>>> > (arch_cpu_idle+0x38/0x3c)
>>>> > [    2.702073] [<c0010184>] (arch_cpu_idle) from [<c0058ed4>]
>>>> > (cpu_startup_entry+0x1ec/0x270)
>>>> > [    2.702086] [<c0058ed4>] (cpu_startup_entry) from [<4000956c>]
>>>> > (0x4000956c)
>>>> > [    2.702093] ---[ end trace 539af70491f4f1a9 ]---
>>>> >
>>>> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>> > ---
>>>> >  drivers/pwm/core.c | 4 ----
>>>> >  1 file changed, 4 deletions(-)
>>>> >
>>>> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>>>> > index d24ca5f..b8f035a 100644
>>>> > --- a/drivers/pwm/core.c
>>>> > +++ b/drivers/pwm/core.c
>>>> > @@ -506,16 +506,12 @@ int pwm_enable(struct pwm_device *pwm)
>>>> >       if (!pwm)
>>>> >               return -EINVAL;
>>>> >
>>>> > -     mutex_lock(&pwm->lock);
>>>> > -
>>>> >       if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
>>>> >               err = pwm->chip->ops->enable(pwm->chip, pwm);
>>>> >               if (err)
>>>> >                       clear_bit(PWMF_ENABLED, &pwm->flags);
>>>> >       }
>>>> >
>>>> > -     mutex_unlock(&pwm->lock);
>>>> > -
>>>> >       return err;
>>>> >  }
>>>> >  EXPORT_SYMBOL_GPL(pwm_enable);
>>>> >
>>>
>>>
>>> Adding Jonathan Richardson.
>>>
>>> Yes I am
>>>
>>> aware I am reverting some part of the d1cd21427747 ("pwm: Set enable state
>>> properly on failed call to enable")
>>>
>>> Please take a look at this below commit.
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pwm/core.c?id=d1cd21427747f15920cd726f5f67a07880e7dee4
>>>
>>> Actually reverting this change it work fine.
>>>
>>> This changes introduce the new mutex lock pwm->lock to protect enabled bit
>>> by drivers while setting polarity.
>>>
>>> Well pwm_set_polarity already acquires the pwm->lock and calls
>>> pwm_is_enabled function.
>>> Again within pwm_is_enabled we are trying to acquire the same mutex lock.
>>
>> You are describing a lockdown by trying to acquire the same mutex twice.
>>
>> However pwm_is_enabled() does not acquire mutex.
>>
>> Again, please look at generated warnings:
>> 1. BUG: sleeping function called from invalid context
>> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
>>
>> They are not related anyhow to what you described (acquiring already
>> locked mutex).
>>
>> Best regards,
>> Krzysztof
>
> My last reply mail went in HTML format so resend this.
>
> First it was a typo on my part.
> It not pwm_is_enabled function its pwm_enabled.

There is no such function as "pwm_enabled".

Sorry, I don't get your point.

Instead of pasting some commit use a descriptive way to show the calls
leading lockdown. But please use real function names.

Best regards,
Krzysztof

Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable

[Anand Moon]

Hi Krzysztof,

On 21 November 2015 at 18:37, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> 2015-11-21 21:11 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>> hi Krzysztof,
>>
>> On 21 November 2015 at 15:22, Krzysztof Kozlowski
>> <k.kozlowski@samsung.com> wrote:
>>> 2015-11-21 18:40 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>>> hi Krzysztof,
>>>>
>>>> On 21 November 2015 at 09:56, Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>>> wrote:
>>>>>
>>>>> W dniu 21.11.2015 o 01:59, Anand Moon pisze:
>>>>> > Commit d1cd21427747f15920cd726f5f67a07880e7dee4
>>>>> > (pwm: Set enable state properly on failed call to enable)
>>>>> > introduce double lock of mutex on pwm_enable
>>>>> >
>>>>> > Changes fix the following debug lock warning.
>>>>> >
>>>>> > [    2.701720] WARNING: CPU: 3 PID: 0 at kernel/locking/mutex.c:526
>>>>> > __mutex_lock_slowpath+0x3bc/0x404()
>>>>> > [    2.701731] DEBUG_LOCKS_WARN_ON(in_interrupt())
>>>>>
>>>>> Hi Anand!
>>>>>
>>>>> Yeah, I am hitting this as well. Annoying. However your description is
>>>>> inaccurate. Double lock of mutex does not cause warnings of sleeping or
>>>>> locking in atomic context (if you build with debug sleep atomic you will
>>>>> see different warning). More over you are partially reverting the commit
>>>>> d1cd21427747 ("pwm: Set enable state properly on failed call to enable")
>>>>> without proper explanation:
>>>>> 1. why this locking is inappropriate;
>>>>> 2. why it is safe to remove the mutex_lock().
>>>>>
>>>>> Please find the real problem, fix the real problem and throughly explain
>>>>> the real issue.
>>>>>
>>>>> I was suspecting some weird changes in timers. For !irqsafe timer I
>>>>> think it is safe to use mutexes... but apparently not. Maybe a work
>>>>> should be scheduled from function called by timer?
>>>>>
>>>>> Warning with debug atomic sleep:
>>>>> [   16.047517] BUG: sleeping function called from invalid context at
>>>>> ../kernel/locking/mutex.c:617
>>>>> [   16.054866] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
>>>>> [   16.061402] INFO: lockdep is turned off.
>>>>> [   16.065281] Preemption disabled at:[<  (null)>]   (null)
>>>>> [   16.070524]
>>>>> [   16.072002] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
>>>>> 4.4.0-rc1-00040-g28c429565d4f #290
>>>>> [   16.080150] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>>> [   16.086215] [<c0016780>] (unwind_backtrace) from [<c00132f0>]
>>>>> (show_stack+0x10/0x14)
>>>>> [   16.093938] [<c00132f0>] (show_stack) from [<c0223ba4>]
>>>>> (dump_stack+0x70/0xbc)
>>>>> [   16.101122] [<c0223ba4>] (dump_stack) from [<c05ed8e0>]
>>>>> (mutex_lock_nested+0x24/0x474)
>>>>> [   16.109009] [<c05ed8e0>] (mutex_lock_nested) from [<c0259154>]
>>>>> (pwm_enable+0x20/0x7c)
>>>>> [   16.116799] [<c0259154>] (pwm_enable) from [<c04400bc>]
>>>>> (led_heartbeat_function+0xdc/0x140)
>>>>> [   16.125119] [<c04400bc>] (led_heartbeat_function) from [<c00864c8>]
>>>>> (call_timer_fn+0x6c/0xf4)
>>>>> [   16.133606] [<c00864c8>] (call_timer_fn) from [<c00866f8>]
>>>>> (run_timer_softirq+0x1a8/0x230)
>>>>> [   16.141846] [<c00866f8>] (run_timer_softirq) from [<c0028e68>]
>>>>> (__do_softirq+0x134/0x2c0)
>>>>> [   16.149982] [<c0028e68>] (__do_softirq) from [<c0029334>]
>>>>> (irq_exit+0xd0/0x114)
>>>>> [   16.157255] [<c0029334>] (irq_exit) from [<c0076610>]
>>>>> (__handle_domain_irq+0x70/0xe4)
>>>>> [   16.165056] [<c0076610>] (__handle_domain_irq) from [<c00094e8>]
>>>>> (gic_handle_irq+0x4c/0x94)
>>>>> [   16.173376] [<c00094e8>] (gic_handle_irq) from [<c0013db8>]
>>>>> (__irq_svc+0x58/0x98)
>>>>> [   16.180817] Exception stack(0xc08cdf58 to 0xc08cdfa0)
>>>>> [   16.185823] df40:
>>>>>    c0010dcc 00000000
>>>>> [   16.193997] df60: c08cdfa8 00000000 c05f57c4 00000000 c08ca520
>>>>> c08ce4bc c08c7364 c08ce4b4
>>>>> [   16.202141] df80: c09121ca 00000000 00000001 c08cdfa8 c0010dcc
>>>>> c0010dd0 600f0013 ffffffff
>>>>> [   16.210291] [<c0013db8>] (__irq_svc) from [<c0010dd0>]
>>>>> (arch_cpu_idle+0x20/0x3c)
>>>>> [   16.217661] [<c0010dd0>] (arch_cpu_idle) from [<c0063174>]
>>>>> (cpu_startup_entry+0x17c/0x26c)
>>>>> [   16.225899] [<c0063174>] (cpu_startup_entry) from [<c0860c40>]
>>>>> (start_kernel+0x368/0x3d0)
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>>
>>>>>
>>>>> > [    2.701737] Modules linked in:
>>>>> > [    2.701748] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.4.0-rc1-xu4f
>>>>> > #24
>>>>> > [    2.701753] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>>> > [    2.701787] [<c0015f48>] (unwind_backtrace) from [<c0012d04>]
>>>>> > (show_stack+0x10/0x14)
>>>>> > [    2.701808] [<c0012d04>] (show_stack) from [<c01f83fc>]
>>>>> > (dump_stack+0x84/0xc4)
>>>>> > [    2.701824] [<c01f83fc>] (dump_stack) from [<c0023494>]
>>>>> > (warn_slowpath_common+0x80/0xb0)
>>>>> > [    2.701836] [<c0023494>] (warn_slowpath_common) from [<c00234f4>]
>>>>> > (warn_slowpath_fmt+0x30/0x40)
>>>>> > [    2.701849] [<c00234f4>] (warn_slowpath_fmt) from [<c056e6b8>]
>>>>> > (__mutex_lock_slowpath+0x3bc/0x404)
>>>>> > [    2.701864] [<c056e6b8>] (__mutex_lock_slowpath) from [<c056e70c>]
>>>>> > (mutex_lock+0xc/0x24)
>>>>> > [    2.701884] [<c056e70c>] (mutex_lock) from [<c0228984>]
>>>>> > (pwm_enable+0x20/0x7c)
>>>>> > [    2.701903] [<c0228984>] (pwm_enable) from [<c03f0000>]
>>>>> > (led_heartbeat_function+0x74/0x144)
>>>>> > [    2.701919] [<c03f0000>] (led_heartbeat_function) from [<c0074368>]
>>>>> > (call_timer_fn+0x24/0x98)
>>>>> > [    2.701932] [<c0074368>] (call_timer_fn) from [<c007453c>]
>>>>> > (run_timer_softirq+0x160/0x21c)
>>>>> > [    2.701946] [<c007453c>] (run_timer_softirq) from [<c0026e10>]
>>>>> > (__do_softirq+0x110/0x228)
>>>>> > [    2.701959] [<c0026e10>] (__do_softirq) from [<c00271c8>]
>>>>> > (irq_exit+0xc0/0xfc)
>>>>> > [    2.701976] [<c00271c8>] (irq_exit) from [<c0065180>]
>>>>> > (__handle_domain_irq+0x80/0xec)
>>>>> > [    2.701990] [<c0065180>] (__handle_domain_irq) from [<c0009494>]
>>>>> > (gic_handle_irq+0x54/0x94)
>>>>> > [    2.702001] [<c0009494>] (gic_handle_irq) from [<c0013794>]
>>>>> > (__irq_svc+0x54/0x90)
>>>>> > [    2.702008] Exception stack(0xee8bdf88 to 0xee8bdfd0)
>>>>> > [    2.702019] df80:                   00000001 00000000 00000000
>>>>> > c001b720 ee8bc000 c0573354
>>>>> > [    2.702031] dfa0: 00000000 00000000 ee8bdfe0 c07f92e4 c08004b4
>>>>> > c08004bc f0806640 ee8bdfd8
>>>>> > [    2.702039] dfc0: c0010180 c0010184 60000013 ffffffff
>>>>> > [    2.702053] [<c0013794>] (__irq_svc) from [<c0010184>]
>>>>> > (arch_cpu_idle+0x38/0x3c)
>>>>> > [    2.702073] [<c0010184>] (arch_cpu_idle) from [<c0058ed4>]
>>>>> > (cpu_startup_entry+0x1ec/0x270)
>>>>> > [    2.702086] [<c0058ed4>] (cpu_startup_entry) from [<4000956c>]
>>>>> > (0x4000956c)
>>>>> > [    2.702093] ---[ end trace 539af70491f4f1a9 ]---
>>>>> >
>>>>> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>>> > ---
>>>>> >  drivers/pwm/core.c | 4 ----
>>>>> >  1 file changed, 4 deletions(-)
>>>>> >
>>>>> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>>>>> > index d24ca5f..b8f035a 100644
>>>>> > --- a/drivers/pwm/core.c
>>>>> > +++ b/drivers/pwm/core.c
>>>>> > @@ -506,16 +506,12 @@ int pwm_enable(struct pwm_device *pwm)
>>>>> >       if (!pwm)
>>>>> >               return -EINVAL;
>>>>> >
>>>>> > -     mutex_lock(&pwm->lock);
>>>>> > -
>>>>> >       if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
>>>>> >               err = pwm->chip->ops->enable(pwm->chip, pwm);
>>>>> >               if (err)
>>>>> >                       clear_bit(PWMF_ENABLED, &pwm->flags);
>>>>> >       }
>>>>> >
>>>>> > -     mutex_unlock(&pwm->lock);
>>>>> > -
>>>>> >       return err;
>>>>> >  }
>>>>> >  EXPORT_SYMBOL_GPL(pwm_enable);
>>>>> >
>>>>
>>>>
>>>> Adding Jonathan Richardson.
>>>>
>>>> Yes I am
>>>>
>>>> aware I am reverting some part of the d1cd21427747 ("pwm: Set enable state
>>>> properly on failed call to enable")
>>>>
>>>> Please take a look at this below commit.
>>>>
>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pwm/core.c?id=d1cd21427747f15920cd726f5f67a07880e7dee4
>>>>
>>>> Actually reverting this change it work fine.
>>>>
>>>> This changes introduce the new mutex lock pwm->lock to protect enabled bit
>>>> by drivers while setting polarity.
>>>>
>>>> Well pwm_set_polarity already acquires the pwm->lock and calls
>>>> pwm_is_enabled function.
>>>> Again within pwm_is_enabled we are trying to acquire the same mutex lock.
>>>
>>> You are describing a lockdown by trying to acquire the same mutex twice.
>>>
>>> However pwm_is_enabled() does not acquire mutex.
>>>
>>> Again, please look at generated warnings:
>>> 1. BUG: sleeping function called from invalid context
>>> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
>>>
>>> They are not related anyhow to what you described (acquiring already
>>> locked mutex).
>>>
>>> Best regards,
>>> Krzysztof
>>
>> My last reply mail went in HTML format so resend this.
>>
>> First it was a typo on my part.
>> It not pwm_is_enabled function its pwm_enabled.
>
> There is no such function as "pwm_enabled".
>
> Sorry, I don't get your point.
>
> Instead of pasting some commit use a descriptive way to show the calls
> leading lockdown. But please use real function names.
>
> Best regards,
> Krzysztof

Opps Another type, sorry for this silly mistake.

It think so I am wrong on this patch.
Sorry for the noise.

-Anand Moon

Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable

[Anand Moon]

Hi Krzysztof,

On 21 November 2015 at 18:37, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> 2015-11-21 21:11 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>> hi Krzysztof,
>>
>> On 21 November 2015 at 15:22, Krzysztof Kozlowski
>> <k.kozlowski@samsung.com> wrote:
>>> 2015-11-21 18:40 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>>> hi Krzysztof,
>>>>
>>>> On 21 November 2015 at 09:56, Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>>> wrote:
>>>>>
>>>>> W dniu 21.11.2015 o 01:59, Anand Moon pisze:
>>>>> > Commit d1cd21427747f15920cd726f5f67a07880e7dee4
>>>>> > (pwm: Set enable state properly on failed call to enable)
>>>>> > introduce double lock of mutex on pwm_enable
>>>>> >
>>>>> > Changes fix the following debug lock warning.
>>>>> >
>>>>> > [    2.701720] WARNING: CPU: 3 PID: 0 at kernel/locking/mutex.c:526
>>>>> > __mutex_lock_slowpath+0x3bc/0x404()
>>>>> > [    2.701731] DEBUG_LOCKS_WARN_ON(in_interrupt())
>>>>>
>>>>> Hi Anand!
>>>>>
>>>>> Yeah, I am hitting this as well. Annoying. However your description is
>>>>> inaccurate. Double lock of mutex does not cause warnings of sleeping or
>>>>> locking in atomic context (if you build with debug sleep atomic you will
>>>>> see different warning). More over you are partially reverting the commit
>>>>> d1cd21427747 ("pwm: Set enable state properly on failed call to enable")
>>>>> without proper explanation:
>>>>> 1. why this locking is inappropriate;
>>>>> 2. why it is safe to remove the mutex_lock().
>>>>>
>>>>> Please find the real problem, fix the real problem and throughly explain
>>>>> the real issue.
>>>>>
>>>>> I was suspecting some weird changes in timers. For !irqsafe timer I
>>>>> think it is safe to use mutexes... but apparently not. Maybe a work
>>>>> should be scheduled from function called by timer?
>>>>>
>>>>> Warning with debug atomic sleep:
>>>>> [   16.047517] BUG: sleeping function called from invalid context at
>>>>> ../kernel/locking/mutex.c:617
>>>>> [   16.054866] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
>>>>> [   16.061402] INFO: lockdep is turned off.
>>>>> [   16.065281] Preemption disabled at:[<  (null)>]   (null)
>>>>> [   16.070524]
>>>>> [   16.072002] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
>>>>> 4.4.0-rc1-00040-g28c429565d4f #290
>>>>> [   16.080150] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>>> [   16.086215] [<c0016780>] (unwind_backtrace) from [<c00132f0>]
>>>>> (show_stack+0x10/0x14)
>>>>> [   16.093938] [<c00132f0>] (show_stack) from [<c0223ba4>]
>>>>> (dump_stack+0x70/0xbc)
>>>>> [   16.101122] [<c0223ba4>] (dump_stack) from [<c05ed8e0>]
>>>>> (mutex_lock_nested+0x24/0x474)
>>>>> [   16.109009] [<c05ed8e0>] (mutex_lock_nested) from [<c0259154>]
>>>>> (pwm_enable+0x20/0x7c)
>>>>> [   16.116799] [<c0259154>] (pwm_enable) from [<c04400bc>]
>>>>> (led_heartbeat_function+0xdc/0x140)
>>>>> [   16.125119] [<c04400bc>] (led_heartbeat_function) from [<c00864c8>]
>>>>> (call_timer_fn+0x6c/0xf4)
>>>>> [   16.133606] [<c00864c8>] (call_timer_fn) from [<c00866f8>]
>>>>> (run_timer_softirq+0x1a8/0x230)
>>>>> [   16.141846] [<c00866f8>] (run_timer_softirq) from [<c0028e68>]
>>>>> (__do_softirq+0x134/0x2c0)
>>>>> [   16.149982] [<c0028e68>] (__do_softirq) from [<c0029334>]
>>>>> (irq_exit+0xd0/0x114)
>>>>> [   16.157255] [<c0029334>] (irq_exit) from [<c0076610>]
>>>>> (__handle_domain_irq+0x70/0xe4)
>>>>> [   16.165056] [<c0076610>] (__handle_domain_irq) from [<c00094e8>]
>>>>> (gic_handle_irq+0x4c/0x94)
>>>>> [   16.173376] [<c00094e8>] (gic_handle_irq) from [<c0013db8>]
>>>>> (__irq_svc+0x58/0x98)
>>>>> [   16.180817] Exception stack(0xc08cdf58 to 0xc08cdfa0)
>>>>> [   16.185823] df40:
>>>>>    c0010dcc 00000000
>>>>> [   16.193997] df60: c08cdfa8 00000000 c05f57c4 00000000 c08ca520
>>>>> c08ce4bc c08c7364 c08ce4b4
>>>>> [   16.202141] df80: c09121ca 00000000 00000001 c08cdfa8 c0010dcc
>>>>> c0010dd0 600f0013 ffffffff
>>>>> [   16.210291] [<c0013db8>] (__irq_svc) from [<c0010dd0>]
>>>>> (arch_cpu_idle+0x20/0x3c)
>>>>> [   16.217661] [<c0010dd0>] (arch_cpu_idle) from [<c0063174>]
>>>>> (cpu_startup_entry+0x17c/0x26c)
>>>>> [   16.225899] [<c0063174>] (cpu_startup_entry) from [<c0860c40>]
>>>>> (start_kernel+0x368/0x3d0)
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>>
>>>>>
>>>>> > [    2.701737] Modules linked in:
>>>>> > [    2.701748] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.4.0-rc1-xu4f
>>>>> > #24
>>>>> > [    2.701753] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>>> > [    2.701787] [<c0015f48>] (unwind_backtrace) from [<c0012d04>]
>>>>> > (show_stack+0x10/0x14)
>>>>> > [    2.701808] [<c0012d04>] (show_stack) from [<c01f83fc>]
>>>>> > (dump_stack+0x84/0xc4)
>>>>> > [    2.701824] [<c01f83fc>] (dump_stack) from [<c0023494>]
>>>>> > (warn_slowpath_common+0x80/0xb0)
>>>>> > [    2.701836] [<c0023494>] (warn_slowpath_common) from [<c00234f4>]
>>>>> > (warn_slowpath_fmt+0x30/0x40)
>>>>> > [    2.701849] [<c00234f4>] (warn_slowpath_fmt) from [<c056e6b8>]
>>>>> > (__mutex_lock_slowpath+0x3bc/0x404)
>>>>> > [    2.701864] [<c056e6b8>] (__mutex_lock_slowpath) from [<c056e70c>]
>>>>> > (mutex_lock+0xc/0x24)
>>>>> > [    2.701884] [<c056e70c>] (mutex_lock) from [<c0228984>]
>>>>> > (pwm_enable+0x20/0x7c)
>>>>> > [    2.701903] [<c0228984>] (pwm_enable) from [<c03f0000>]
>>>>> > (led_heartbeat_function+0x74/0x144)
>>>>> > [    2.701919] [<c03f0000>] (led_heartbeat_function) from [<c0074368>]
>>>>> > (call_timer_fn+0x24/0x98)
>>>>> > [    2.701932] [<c0074368>] (call_timer_fn) from [<c007453c>]
>>>>> > (run_timer_softirq+0x160/0x21c)
>>>>> > [    2.701946] [<c007453c>] (run_timer_softirq) from [<c0026e10>]
>>>>> > (__do_softirq+0x110/0x228)
>>>>> > [    2.701959] [<c0026e10>] (__do_softirq) from [<c00271c8>]
>>>>> > (irq_exit+0xc0/0xfc)
>>>>> > [    2.701976] [<c00271c8>] (irq_exit) from [<c0065180>]
>>>>> > (__handle_domain_irq+0x80/0xec)
>>>>> > [    2.701990] [<c0065180>] (__handle_domain_irq) from [<c0009494>]
>>>>> > (gic_handle_irq+0x54/0x94)
>>>>> > [    2.702001] [<c0009494>] (gic_handle_irq) from [<c0013794>]
>>>>> > (__irq_svc+0x54/0x90)
>>>>> > [    2.702008] Exception stack(0xee8bdf88 to 0xee8bdfd0)
>>>>> > [    2.702019] df80:                   00000001 00000000 00000000
>>>>> > c001b720 ee8bc000 c0573354
>>>>> > [    2.702031] dfa0: 00000000 00000000 ee8bdfe0 c07f92e4 c08004b4
>>>>> > c08004bc f0806640 ee8bdfd8
>>>>> > [    2.702039] dfc0: c0010180 c0010184 60000013 ffffffff
>>>>> > [    2.702053] [<c0013794>] (__irq_svc) from [<c0010184>]
>>>>> > (arch_cpu_idle+0x38/0x3c)
>>>>> > [    2.702073] [<c0010184>] (arch_cpu_idle) from [<c0058ed4>]
>>>>> > (cpu_startup_entry+0x1ec/0x270)
>>>>> > [    2.702086] [<c0058ed4>] (cpu_startup_entry) from [<4000956c>]
>>>>> > (0x4000956c)
>>>>> > [    2.702093] ---[ end trace 539af70491f4f1a9 ]---
>>>>> >
>>>>> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>>> > ---
>>>>> >  drivers/pwm/core.c | 4 ----
>>>>> >  1 file changed, 4 deletions(-)
>>>>> >
>>>>> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>>>>> > index d24ca5f..b8f035a 100644
>>>>> > --- a/drivers/pwm/core.c
>>>>> > +++ b/drivers/pwm/core.c
>>>>> > @@ -506,16 +506,12 @@ int pwm_enable(struct pwm_device *pwm)
>>>>> >       if (!pwm)
>>>>> >               return -EINVAL;
>>>>> >
>>>>> > -     mutex_lock(&pwm->lock);
>>>>> > -
>>>>> >       if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
>>>>> >               err = pwm->chip->ops->enable(pwm->chip, pwm);
>>>>> >               if (err)
>>>>> >                       clear_bit(PWMF_ENABLED, &pwm->flags);
>>>>> >       }
>>>>> >
>>>>> > -     mutex_unlock(&pwm->lock);
>>>>> > -
>>>>> >       return err;
>>>>> >  }
>>>>> >  EXPORT_SYMBOL_GPL(pwm_enable);
>>>>> >
>>>>
>>>>
>>>> Adding Jonathan Richardson.
>>>>
>>>> Yes I am
>>>>
>>>> aware I am reverting some part of the d1cd21427747 ("pwm: Set enable state
>>>> properly on failed call to enable")
>>>>
>>>> Please take a look at this below commit.
>>>>
>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pwm/core.c?id=d1cd21427747f15920cd726f5f67a07880e7dee4
>>>>
>>>> Actually reverting this change it work fine.
>>>>
>>>> This changes introduce the new mutex lock pwm->lock to protect enabled bit
>>>> by drivers while setting polarity.
>>>>
>>>> Well pwm_set_polarity already acquires the pwm->lock and calls
>>>> pwm_is_enabled function.
>>>> Again within pwm_is_enabled we are trying to acquire the same mutex lock.
>>>
>>> You are describing a lockdown by trying to acquire the same mutex twice.
>>>
>>> However pwm_is_enabled() does not acquire mutex.
>>>
>>> Again, please look at generated warnings:
>>> 1. BUG: sleeping function called from invalid context
>>> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
>>>
>>> They are not related anyhow to what you described (acquiring already
>>> locked mutex).
>>>
>>> Best regards,
>>> Krzysztof
>>
>> My last reply mail went in HTML format so resend this.
>>
>> First it was a typo on my part.
>> It not pwm_is_enabled function its pwm_enabled.
>
> There is no such function as "pwm_enabled".
>
> Sorry, I don't get your point.
>
> Instead of pasting some commit use a descriptive way to show the calls
> leading lockdown. But please use real function names.
>
> Best regards,
> Krzysztof

Earlier my assumption of double mutex lock up totally rubbish.

After reverting my changes and building image with CONFIG_DEBUG_ATOMIC_SLEEP.

[  390.415370] BUG: sleeping function called from invalid context at
kernel/locking/mutex.c:97
[  390.422274] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
[  390.428831] Preemption disabled at:[<  (null)>]   (null)
[  391.970352] BUG: sleeping function called from invalid context at
kernel/locking/mutex.c:97
[  391.977251] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
[  391.983814] Preemption disabled at:[<  (null)>]   (null)
[  393.520376] BUG: sleeping function called from invalid context at
kernel/locking/mutex.c:97
[  393.527312] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
[  393.533925] Preemption disabled at:[<  (null)>]   (null)

I don't understand why their is requirement for mutex lock in
pwm_enable function.
mutexes are sleeping locks which behave similarly to binary semaphores.

Please correct me if I am wrong.

-Anand Moon

Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable

[Krzysztof Kozlowski]

2015-11-22 3:14 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
> Hi Krzysztof,
>
> On 21 November 2015 at 18:37, Krzysztof Kozlowski
> <k.kozlowski@samsung.com> wrote:
>> 2015-11-21 21:11 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>> hi Krzysztof,
>>>
>>> On 21 November 2015 at 15:22, Krzysztof Kozlowski
>>> <k.kozlowski@samsung.com> wrote:
>>>> 2015-11-21 18:40 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>>>> hi Krzysztof,
>>>>>
>>>>> On 21 November 2015 at 09:56, Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>>>> wrote:
>>>>>>
>>>>>> W dniu 21.11.2015 o 01:59, Anand Moon pisze:
>>>>>> > Commit d1cd21427747f15920cd726f5f67a07880e7dee4
>>>>>> > (pwm: Set enable state properly on failed call to enable)
>>>>>> > introduce double lock of mutex on pwm_enable
>>>>>> >
>>>>>> > Changes fix the following debug lock warning.
>>>>>> >
>>>>>> > [    2.701720] WARNING: CPU: 3 PID: 0 at kernel/locking/mutex.c:526
>>>>>> > __mutex_lock_slowpath+0x3bc/0x404()
>>>>>> > [    2.701731] DEBUG_LOCKS_WARN_ON(in_interrupt())
>>>>>>
>>>>>> Hi Anand!
>>>>>>
>>>>>> Yeah, I am hitting this as well. Annoying. However your description is
>>>>>> inaccurate. Double lock of mutex does not cause warnings of sleeping or
>>>>>> locking in atomic context (if you build with debug sleep atomic you will
>>>>>> see different warning). More over you are partially reverting the commit
>>>>>> d1cd21427747 ("pwm: Set enable state properly on failed call to enable")
>>>>>> without proper explanation:
>>>>>> 1. why this locking is inappropriate;
>>>>>> 2. why it is safe to remove the mutex_lock().
>>>>>>
>>>>>> Please find the real problem, fix the real problem and throughly explain
>>>>>> the real issue.
>>>>>>
>>>>>> I was suspecting some weird changes in timers. For !irqsafe timer I
>>>>>> think it is safe to use mutexes... but apparently not. Maybe a work
>>>>>> should be scheduled from function called by timer?
>>>>>>
>>>>>> Warning with debug atomic sleep:
>>>>>> [   16.047517] BUG: sleeping function called from invalid context at
>>>>>> ../kernel/locking/mutex.c:617
>>>>>> [   16.054866] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
>>>>>> [   16.061402] INFO: lockdep is turned off.
>>>>>> [   16.065281] Preemption disabled at:[<  (null)>]   (null)
>>>>>> [   16.070524]
>>>>>> [   16.072002] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
>>>>>> 4.4.0-rc1-00040-g28c429565d4f #290
>>>>>> [   16.080150] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>>>> [   16.086215] [<c0016780>] (unwind_backtrace) from [<c00132f0>]
>>>>>> (show_stack+0x10/0x14)
>>>>>> [   16.093938] [<c00132f0>] (show_stack) from [<c0223ba4>]
>>>>>> (dump_stack+0x70/0xbc)
>>>>>> [   16.101122] [<c0223ba4>] (dump_stack) from [<c05ed8e0>]
>>>>>> (mutex_lock_nested+0x24/0x474)
>>>>>> [   16.109009] [<c05ed8e0>] (mutex_lock_nested) from [<c0259154>]
>>>>>> (pwm_enable+0x20/0x7c)
>>>>>> [   16.116799] [<c0259154>] (pwm_enable) from [<c04400bc>]
>>>>>> (led_heartbeat_function+0xdc/0x140)
>>>>>> [   16.125119] [<c04400bc>] (led_heartbeat_function) from [<c00864c8>]
>>>>>> (call_timer_fn+0x6c/0xf4)
>>>>>> [   16.133606] [<c00864c8>] (call_timer_fn) from [<c00866f8>]
>>>>>> (run_timer_softirq+0x1a8/0x230)
>>>>>> [   16.141846] [<c00866f8>] (run_timer_softirq) from [<c0028e68>]
>>>>>> (__do_softirq+0x134/0x2c0)
>>>>>> [   16.149982] [<c0028e68>] (__do_softirq) from [<c0029334>]
>>>>>> (irq_exit+0xd0/0x114)
>>>>>> [   16.157255] [<c0029334>] (irq_exit) from [<c0076610>]
>>>>>> (__handle_domain_irq+0x70/0xe4)
>>>>>> [   16.165056] [<c0076610>] (__handle_domain_irq) from [<c00094e8>]
>>>>>> (gic_handle_irq+0x4c/0x94)
>>>>>> [   16.173376] [<c00094e8>] (gic_handle_irq) from [<c0013db8>]
>>>>>> (__irq_svc+0x58/0x98)
>>>>>> [   16.180817] Exception stack(0xc08cdf58 to 0xc08cdfa0)
>>>>>> [   16.185823] df40:
>>>>>>    c0010dcc 00000000
>>>>>> [   16.193997] df60: c08cdfa8 00000000 c05f57c4 00000000 c08ca520
>>>>>> c08ce4bc c08c7364 c08ce4b4
>>>>>> [   16.202141] df80: c09121ca 00000000 00000001 c08cdfa8 c0010dcc
>>>>>> c0010dd0 600f0013 ffffffff
>>>>>> [   16.210291] [<c0013db8>] (__irq_svc) from [<c0010dd0>]
>>>>>> (arch_cpu_idle+0x20/0x3c)
>>>>>> [   16.217661] [<c0010dd0>] (arch_cpu_idle) from [<c0063174>]
>>>>>> (cpu_startup_entry+0x17c/0x26c)
>>>>>> [   16.225899] [<c0063174>] (cpu_startup_entry) from [<c0860c40>]
>>>>>> (start_kernel+0x368/0x3d0)
>>>>>>
>>>>>> Best regards,
>>>>>> Krzysztof
>>>>>>
>>>>>>
>>>>>> > [    2.701737] Modules linked in:
>>>>>> > [    2.701748] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.4.0-rc1-xu4f
>>>>>> > #24
>>>>>> > [    2.701753] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>>>> > [    2.701787] [<c0015f48>] (unwind_backtrace) from [<c0012d04>]
>>>>>> > (show_stack+0x10/0x14)
>>>>>> > [    2.701808] [<c0012d04>] (show_stack) from [<c01f83fc>]
>>>>>> > (dump_stack+0x84/0xc4)
>>>>>> > [    2.701824] [<c01f83fc>] (dump_stack) from [<c0023494>]
>>>>>> > (warn_slowpath_common+0x80/0xb0)
>>>>>> > [    2.701836] [<c0023494>] (warn_slowpath_common) from [<c00234f4>]
>>>>>> > (warn_slowpath_fmt+0x30/0x40)
>>>>>> > [    2.701849] [<c00234f4>] (warn_slowpath_fmt) from [<c056e6b8>]
>>>>>> > (__mutex_lock_slowpath+0x3bc/0x404)
>>>>>> > [    2.701864] [<c056e6b8>] (__mutex_lock_slowpath) from [<c056e70c>]
>>>>>> > (mutex_lock+0xc/0x24)
>>>>>> > [    2.701884] [<c056e70c>] (mutex_lock) from [<c0228984>]
>>>>>> > (pwm_enable+0x20/0x7c)
>>>>>> > [    2.701903] [<c0228984>] (pwm_enable) from [<c03f0000>]
>>>>>> > (led_heartbeat_function+0x74/0x144)
>>>>>> > [    2.701919] [<c03f0000>] (led_heartbeat_function) from [<c0074368>]
>>>>>> > (call_timer_fn+0x24/0x98)
>>>>>> > [    2.701932] [<c0074368>] (call_timer_fn) from [<c007453c>]
>>>>>> > (run_timer_softirq+0x160/0x21c)
>>>>>> > [    2.701946] [<c007453c>] (run_timer_softirq) from [<c0026e10>]
>>>>>> > (__do_softirq+0x110/0x228)
>>>>>> > [    2.701959] [<c0026e10>] (__do_softirq) from [<c00271c8>]
>>>>>> > (irq_exit+0xc0/0xfc)
>>>>>> > [    2.701976] [<c00271c8>] (irq_exit) from [<c0065180>]
>>>>>> > (__handle_domain_irq+0x80/0xec)
>>>>>> > [    2.701990] [<c0065180>] (__handle_domain_irq) from [<c0009494>]
>>>>>> > (gic_handle_irq+0x54/0x94)
>>>>>> > [    2.702001] [<c0009494>] (gic_handle_irq) from [<c0013794>]
>>>>>> > (__irq_svc+0x54/0x90)
>>>>>> > [    2.702008] Exception stack(0xee8bdf88 to 0xee8bdfd0)
>>>>>> > [    2.702019] df80:                   00000001 00000000 00000000
>>>>>> > c001b720 ee8bc000 c0573354
>>>>>> > [    2.702031] dfa0: 00000000 00000000 ee8bdfe0 c07f92e4 c08004b4
>>>>>> > c08004bc f0806640 ee8bdfd8
>>>>>> > [    2.702039] dfc0: c0010180 c0010184 60000013 ffffffff
>>>>>> > [    2.702053] [<c0013794>] (__irq_svc) from [<c0010184>]
>>>>>> > (arch_cpu_idle+0x38/0x3c)
>>>>>> > [    2.702073] [<c0010184>] (arch_cpu_idle) from [<c0058ed4>]
>>>>>> > (cpu_startup_entry+0x1ec/0x270)
>>>>>> > [    2.702086] [<c0058ed4>] (cpu_startup_entry) from [<4000956c>]
>>>>>> > (0x4000956c)
>>>>>> > [    2.702093] ---[ end trace 539af70491f4f1a9 ]---
>>>>>> >
>>>>>> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>>>> > ---
>>>>>> >  drivers/pwm/core.c | 4 ----
>>>>>> >  1 file changed, 4 deletions(-)
>>>>>> >
>>>>>> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>>>>>> > index d24ca5f..b8f035a 100644
>>>>>> > --- a/drivers/pwm/core.c
>>>>>> > +++ b/drivers/pwm/core.c
>>>>>> > @@ -506,16 +506,12 @@ int pwm_enable(struct pwm_device *pwm)
>>>>>> >       if (!pwm)
>>>>>> >               return -EINVAL;
>>>>>> >
>>>>>> > -     mutex_lock(&pwm->lock);
>>>>>> > -
>>>>>> >       if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
>>>>>> >               err = pwm->chip->ops->enable(pwm->chip, pwm);
>>>>>> >               if (err)
>>>>>> >                       clear_bit(PWMF_ENABLED, &pwm->flags);
>>>>>> >       }
>>>>>> >
>>>>>> > -     mutex_unlock(&pwm->lock);
>>>>>> > -
>>>>>> >       return err;
>>>>>> >  }
>>>>>> >  EXPORT_SYMBOL_GPL(pwm_enable);
>>>>>> >
>>>>>
>>>>>
>>>>> Adding Jonathan Richardson.
>>>>>
>>>>> Yes I am
>>>>>
>>>>> aware I am reverting some part of the d1cd21427747 ("pwm: Set enable state
>>>>> properly on failed call to enable")
>>>>>
>>>>> Please take a look at this below commit.
>>>>>
>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pwm/core.c?id=d1cd21427747f15920cd726f5f67a07880e7dee4
>>>>>
>>>>> Actually reverting this change it work fine.
>>>>>
>>>>> This changes introduce the new mutex lock pwm->lock to protect enabled bit
>>>>> by drivers while setting polarity.
>>>>>
>>>>> Well pwm_set_polarity already acquires the pwm->lock and calls
>>>>> pwm_is_enabled function.
>>>>> Again within pwm_is_enabled we are trying to acquire the same mutex lock.
>>>>
>>>> You are describing a lockdown by trying to acquire the same mutex twice.
>>>>
>>>> However pwm_is_enabled() does not acquire mutex.
>>>>
>>>> Again, please look at generated warnings:
>>>> 1. BUG: sleeping function called from invalid context
>>>> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
>>>>
>>>> They are not related anyhow to what you described (acquiring already
>>>> locked mutex).
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>
>>> My last reply mail went in HTML format so resend this.
>>>
>>> First it was a typo on my part.
>>> It not pwm_is_enabled function its pwm_enabled.
>>
>> There is no such function as "pwm_enabled".
>>
>> Sorry, I don't get your point.
>>
>> Instead of pasting some commit use a descriptive way to show the calls
>> leading lockdown. But please use real function names.
>>
>> Best regards,
>> Krzysztof
>
> Earlier my assumption of double mutex lock up totally rubbish.
>
> After reverting my changes and building image with CONFIG_DEBUG_ATOMIC_SLEEP.
>
> [  390.415370] BUG: sleeping function called from invalid context at
> kernel/locking/mutex.c:97
> [  390.422274] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
> [  390.428831] Preemption disabled at:[<  (null)>]   (null)
> [  391.970352] BUG: sleeping function called from invalid context at
> kernel/locking/mutex.c:97
> [  391.977251] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
> [  391.983814] Preemption disabled at:[<  (null)>]   (null)
> [  393.520376] BUG: sleeping function called from invalid context at
> kernel/locking/mutex.c:97
> [  393.527312] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
> [  393.533925] Preemption disabled at:[<  (null)>]   (null)

Yes, now you pasted the same warning I did...

This is still the same issue. I already wrote it:
> 1. BUG: sleeping function called from invalid context
> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())

We can repeat it many times but that won't change anything...

Best regards,
Krzysztof

Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 12043 bytes --]

On Sun, Nov 22, 2015 at 09:13:17AM +0900, Krzysztof Kozlowski wrote:
> 2015-11-22 3:14 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
> > Hi Krzysztof,
> >
> > On 21 November 2015 at 18:37, Krzysztof Kozlowski
> > <k.kozlowski@samsung.com> wrote:
> >> 2015-11-21 21:11 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
> >>> hi Krzysztof,
> >>>
> >>> On 21 November 2015 at 15:22, Krzysztof Kozlowski
> >>> <k.kozlowski@samsung.com> wrote:
> >>>> 2015-11-21 18:40 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
> >>>>> hi Krzysztof,
> >>>>>
> >>>>> On 21 November 2015 at 09:56, Krzysztof Kozlowski <k.kozlowski@samsung.com>
> >>>>> wrote:
> >>>>>>
> >>>>>> W dniu 21.11.2015 o 01:59, Anand Moon pisze:
> >>>>>> > Commit d1cd21427747f15920cd726f5f67a07880e7dee4
> >>>>>> > (pwm: Set enable state properly on failed call to enable)
> >>>>>> > introduce double lock of mutex on pwm_enable
> >>>>>> >
> >>>>>> > Changes fix the following debug lock warning.
> >>>>>> >
> >>>>>> > [    2.701720] WARNING: CPU: 3 PID: 0 at kernel/locking/mutex.c:526
> >>>>>> > __mutex_lock_slowpath+0x3bc/0x404()
> >>>>>> > [    2.701731] DEBUG_LOCKS_WARN_ON(in_interrupt())
> >>>>>>
> >>>>>> Hi Anand!
> >>>>>>
> >>>>>> Yeah, I am hitting this as well. Annoying. However your description is
> >>>>>> inaccurate. Double lock of mutex does not cause warnings of sleeping or
> >>>>>> locking in atomic context (if you build with debug sleep atomic you will
> >>>>>> see different warning). More over you are partially reverting the commit
> >>>>>> d1cd21427747 ("pwm: Set enable state properly on failed call to enable")
> >>>>>> without proper explanation:
> >>>>>> 1. why this locking is inappropriate;
> >>>>>> 2. why it is safe to remove the mutex_lock().
> >>>>>>
> >>>>>> Please find the real problem, fix the real problem and throughly explain
> >>>>>> the real issue.
> >>>>>>
> >>>>>> I was suspecting some weird changes in timers. For !irqsafe timer I
> >>>>>> think it is safe to use mutexes... but apparently not. Maybe a work
> >>>>>> should be scheduled from function called by timer?
> >>>>>>
> >>>>>> Warning with debug atomic sleep:
> >>>>>> [   16.047517] BUG: sleeping function called from invalid context at
> >>>>>> ../kernel/locking/mutex.c:617
> >>>>>> [   16.054866] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
> >>>>>> [   16.061402] INFO: lockdep is turned off.
> >>>>>> [   16.065281] Preemption disabled at:[<  (null)>]   (null)
> >>>>>> [   16.070524]
> >>>>>> [   16.072002] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
> >>>>>> 4.4.0-rc1-00040-g28c429565d4f #290
> >>>>>> [   16.080150] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> >>>>>> [   16.086215] [<c0016780>] (unwind_backtrace) from [<c00132f0>]
> >>>>>> (show_stack+0x10/0x14)
> >>>>>> [   16.093938] [<c00132f0>] (show_stack) from [<c0223ba4>]
> >>>>>> (dump_stack+0x70/0xbc)
> >>>>>> [   16.101122] [<c0223ba4>] (dump_stack) from [<c05ed8e0>]
> >>>>>> (mutex_lock_nested+0x24/0x474)
> >>>>>> [   16.109009] [<c05ed8e0>] (mutex_lock_nested) from [<c0259154>]
> >>>>>> (pwm_enable+0x20/0x7c)
> >>>>>> [   16.116799] [<c0259154>] (pwm_enable) from [<c04400bc>]
> >>>>>> (led_heartbeat_function+0xdc/0x140)
> >>>>>> [   16.125119] [<c04400bc>] (led_heartbeat_function) from [<c00864c8>]
> >>>>>> (call_timer_fn+0x6c/0xf4)
> >>>>>> [   16.133606] [<c00864c8>] (call_timer_fn) from [<c00866f8>]
> >>>>>> (run_timer_softirq+0x1a8/0x230)
> >>>>>> [   16.141846] [<c00866f8>] (run_timer_softirq) from [<c0028e68>]
> >>>>>> (__do_softirq+0x134/0x2c0)
> >>>>>> [   16.149982] [<c0028e68>] (__do_softirq) from [<c0029334>]
> >>>>>> (irq_exit+0xd0/0x114)
> >>>>>> [   16.157255] [<c0029334>] (irq_exit) from [<c0076610>]
> >>>>>> (__handle_domain_irq+0x70/0xe4)
> >>>>>> [   16.165056] [<c0076610>] (__handle_domain_irq) from [<c00094e8>]
> >>>>>> (gic_handle_irq+0x4c/0x94)
> >>>>>> [   16.173376] [<c00094e8>] (gic_handle_irq) from [<c0013db8>]
> >>>>>> (__irq_svc+0x58/0x98)
> >>>>>> [   16.180817] Exception stack(0xc08cdf58 to 0xc08cdfa0)
> >>>>>> [   16.185823] df40:
> >>>>>>    c0010dcc 00000000
> >>>>>> [   16.193997] df60: c08cdfa8 00000000 c05f57c4 00000000 c08ca520
> >>>>>> c08ce4bc c08c7364 c08ce4b4
> >>>>>> [   16.202141] df80: c09121ca 00000000 00000001 c08cdfa8 c0010dcc
> >>>>>> c0010dd0 600f0013 ffffffff
> >>>>>> [   16.210291] [<c0013db8>] (__irq_svc) from [<c0010dd0>]
> >>>>>> (arch_cpu_idle+0x20/0x3c)
> >>>>>> [   16.217661] [<c0010dd0>] (arch_cpu_idle) from [<c0063174>]
> >>>>>> (cpu_startup_entry+0x17c/0x26c)
> >>>>>> [   16.225899] [<c0063174>] (cpu_startup_entry) from [<c0860c40>]
> >>>>>> (start_kernel+0x368/0x3d0)
> >>>>>>
> >>>>>> Best regards,
> >>>>>> Krzysztof
> >>>>>>
> >>>>>>
> >>>>>> > [    2.701737] Modules linked in:
> >>>>>> > [    2.701748] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.4.0-rc1-xu4f
> >>>>>> > #24
> >>>>>> > [    2.701753] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> >>>>>> > [    2.701787] [<c0015f48>] (unwind_backtrace) from [<c0012d04>]
> >>>>>> > (show_stack+0x10/0x14)
> >>>>>> > [    2.701808] [<c0012d04>] (show_stack) from [<c01f83fc>]
> >>>>>> > (dump_stack+0x84/0xc4)
> >>>>>> > [    2.701824] [<c01f83fc>] (dump_stack) from [<c0023494>]
> >>>>>> > (warn_slowpath_common+0x80/0xb0)
> >>>>>> > [    2.701836] [<c0023494>] (warn_slowpath_common) from [<c00234f4>]
> >>>>>> > (warn_slowpath_fmt+0x30/0x40)
> >>>>>> > [    2.701849] [<c00234f4>] (warn_slowpath_fmt) from [<c056e6b8>]
> >>>>>> > (__mutex_lock_slowpath+0x3bc/0x404)
> >>>>>> > [    2.701864] [<c056e6b8>] (__mutex_lock_slowpath) from [<c056e70c>]
> >>>>>> > (mutex_lock+0xc/0x24)
> >>>>>> > [    2.701884] [<c056e70c>] (mutex_lock) from [<c0228984>]
> >>>>>> > (pwm_enable+0x20/0x7c)
> >>>>>> > [    2.701903] [<c0228984>] (pwm_enable) from [<c03f0000>]
> >>>>>> > (led_heartbeat_function+0x74/0x144)
> >>>>>> > [    2.701919] [<c03f0000>] (led_heartbeat_function) from [<c0074368>]
> >>>>>> > (call_timer_fn+0x24/0x98)
> >>>>>> > [    2.701932] [<c0074368>] (call_timer_fn) from [<c007453c>]
> >>>>>> > (run_timer_softirq+0x160/0x21c)
> >>>>>> > [    2.701946] [<c007453c>] (run_timer_softirq) from [<c0026e10>]
> >>>>>> > (__do_softirq+0x110/0x228)
> >>>>>> > [    2.701959] [<c0026e10>] (__do_softirq) from [<c00271c8>]
> >>>>>> > (irq_exit+0xc0/0xfc)
> >>>>>> > [    2.701976] [<c00271c8>] (irq_exit) from [<c0065180>]
> >>>>>> > (__handle_domain_irq+0x80/0xec)
> >>>>>> > [    2.701990] [<c0065180>] (__handle_domain_irq) from [<c0009494>]
> >>>>>> > (gic_handle_irq+0x54/0x94)
> >>>>>> > [    2.702001] [<c0009494>] (gic_handle_irq) from [<c0013794>]
> >>>>>> > (__irq_svc+0x54/0x90)
> >>>>>> > [    2.702008] Exception stack(0xee8bdf88 to 0xee8bdfd0)
> >>>>>> > [    2.702019] df80:                   00000001 00000000 00000000
> >>>>>> > c001b720 ee8bc000 c0573354
> >>>>>> > [    2.702031] dfa0: 00000000 00000000 ee8bdfe0 c07f92e4 c08004b4
> >>>>>> > c08004bc f0806640 ee8bdfd8
> >>>>>> > [    2.702039] dfc0: c0010180 c0010184 60000013 ffffffff
> >>>>>> > [    2.702053] [<c0013794>] (__irq_svc) from [<c0010184>]
> >>>>>> > (arch_cpu_idle+0x38/0x3c)
> >>>>>> > [    2.702073] [<c0010184>] (arch_cpu_idle) from [<c0058ed4>]
> >>>>>> > (cpu_startup_entry+0x1ec/0x270)
> >>>>>> > [    2.702086] [<c0058ed4>] (cpu_startup_entry) from [<4000956c>]
> >>>>>> > (0x4000956c)
> >>>>>> > [    2.702093] ---[ end trace 539af70491f4f1a9 ]---
> >>>>>> >
> >>>>>> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> >>>>>> > ---
> >>>>>> >  drivers/pwm/core.c | 4 ----
> >>>>>> >  1 file changed, 4 deletions(-)
> >>>>>> >
> >>>>>> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> >>>>>> > index d24ca5f..b8f035a 100644
> >>>>>> > --- a/drivers/pwm/core.c
> >>>>>> > +++ b/drivers/pwm/core.c
> >>>>>> > @@ -506,16 +506,12 @@ int pwm_enable(struct pwm_device *pwm)
> >>>>>> >       if (!pwm)
> >>>>>> >               return -EINVAL;
> >>>>>> >
> >>>>>> > -     mutex_lock(&pwm->lock);
> >>>>>> > -
> >>>>>> >       if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
> >>>>>> >               err = pwm->chip->ops->enable(pwm->chip, pwm);
> >>>>>> >               if (err)
> >>>>>> >                       clear_bit(PWMF_ENABLED, &pwm->flags);
> >>>>>> >       }
> >>>>>> >
> >>>>>> > -     mutex_unlock(&pwm->lock);
> >>>>>> > -
> >>>>>> >       return err;
> >>>>>> >  }
> >>>>>> >  EXPORT_SYMBOL_GPL(pwm_enable);
> >>>>>> >
> >>>>>
> >>>>>
> >>>>> Adding Jonathan Richardson.
> >>>>>
> >>>>> Yes I am
> >>>>>
> >>>>> aware I am reverting some part of the d1cd21427747 ("pwm: Set enable state
> >>>>> properly on failed call to enable")
> >>>>>
> >>>>> Please take a look at this below commit.
> >>>>>
> >>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pwm/core.c?id=d1cd21427747f15920cd726f5f67a07880e7dee4
> >>>>>
> >>>>> Actually reverting this change it work fine.
> >>>>>
> >>>>> This changes introduce the new mutex lock pwm->lock to protect enabled bit
> >>>>> by drivers while setting polarity.
> >>>>>
> >>>>> Well pwm_set_polarity already acquires the pwm->lock and calls
> >>>>> pwm_is_enabled function.
> >>>>> Again within pwm_is_enabled we are trying to acquire the same mutex lock.
> >>>>
> >>>> You are describing a lockdown by trying to acquire the same mutex twice.
> >>>>
> >>>> However pwm_is_enabled() does not acquire mutex.
> >>>>
> >>>> Again, please look at generated warnings:
> >>>> 1. BUG: sleeping function called from invalid context
> >>>> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
> >>>>
> >>>> They are not related anyhow to what you described (acquiring already
> >>>> locked mutex).
> >>>>
> >>>> Best regards,
> >>>> Krzysztof
> >>>
> >>> My last reply mail went in HTML format so resend this.
> >>>
> >>> First it was a typo on my part.
> >>> It not pwm_is_enabled function its pwm_enabled.
> >>
> >> There is no such function as "pwm_enabled".
> >>
> >> Sorry, I don't get your point.
> >>
> >> Instead of pasting some commit use a descriptive way to show the calls
> >> leading lockdown. But please use real function names.
> >>
> >> Best regards,
> >> Krzysztof
> >
> > Earlier my assumption of double mutex lock up totally rubbish.
> >
> > After reverting my changes and building image with CONFIG_DEBUG_ATOMIC_SLEEP.
> >
> > [  390.415370] BUG: sleeping function called from invalid context at
> > kernel/locking/mutex.c:97
> > [  390.422274] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
> > [  390.428831] Preemption disabled at:[<  (null)>]   (null)
> > [  391.970352] BUG: sleeping function called from invalid context at
> > kernel/locking/mutex.c:97
> > [  391.977251] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
> > [  391.983814] Preemption disabled at:[<  (null)>]   (null)
> > [  393.520376] BUG: sleeping function called from invalid context at
> > kernel/locking/mutex.c:97
> > [  393.527312] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
> > [  393.533925] Preemption disabled at:[<  (null)>]   (null)
> 
> Yes, now you pasted the same warning I did...
> 
> This is still the same issue. I already wrote it:
> > 1. BUG: sleeping function called from invalid context
> > 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
> 
> We can repeat it many times but that won't change anything...

This looks like you're simply running the leds-pwm driver with a PWM
that isn't properly marked as potentially sleeping. Unfortunately the
introduction of the mutex in d1cd21427747 ("pwm: Set enable state
properly on failed call to enable") effectively makes all PWM drivers
potentially sleeping. That in turn makes the .can_sleep field obsolete
since all drivers can now sleep.

Any objections to simply removing it and make all users use a workqueue
or some such if they need to control a PWM as a result of an interrupt
trigger?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable

[Jonathan Richardson]

On 15-11-23 02:07 AM, Thierry Reding wrote:
> On Sun, Nov 22, 2015 at 09:13:17AM +0900, Krzysztof Kozlowski wrote:
>> 2015-11-22 3:14 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>> Hi Krzysztof,
>>>
>>> On 21 November 2015 at 18:37, Krzysztof Kozlowski
>>> <k.kozlowski@samsung.com> wrote:
>>>> 2015-11-21 21:11 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>>>> hi Krzysztof,
>>>>>
>>>>> On 21 November 2015 at 15:22, Krzysztof Kozlowski
>>>>> <k.kozlowski@samsung.com> wrote:
>>>>>> 2015-11-21 18:40 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>>>>>> hi Krzysztof,
>>>>>>>
>>>>>>> On 21 November 2015 at 09:56, Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> W dniu 21.11.2015 o 01:59, Anand Moon pisze:
>>>>>>>>> Commit d1cd21427747f15920cd726f5f67a07880e7dee4
>>>>>>>>> (pwm: Set enable state properly on failed call to enable)
>>>>>>>>> introduce double lock of mutex on pwm_enable
>>>>>>>>>
>>>>>>>>> Changes fix the following debug lock warning.
>>>>>>>>>
>>>>>>>>> [    2.701720] WARNING: CPU: 3 PID: 0 at kernel/locking/mutex.c:526
>>>>>>>>> __mutex_lock_slowpath+0x3bc/0x404()
>>>>>>>>> [    2.701731] DEBUG_LOCKS_WARN_ON(in_interrupt())
>>>>>>>>
>>>>>>>> Hi Anand!
>>>>>>>>
>>>>>>>> Yeah, I am hitting this as well. Annoying. However your description is
>>>>>>>> inaccurate. Double lock of mutex does not cause warnings of sleeping or
>>>>>>>> locking in atomic context (if you build with debug sleep atomic you will
>>>>>>>> see different warning). More over you are partially reverting the commit
>>>>>>>> d1cd21427747 ("pwm: Set enable state properly on failed call to enable")
>>>>>>>> without proper explanation:
>>>>>>>> 1. why this locking is inappropriate;
>>>>>>>> 2. why it is safe to remove the mutex_lock().
>>>>>>>>
>>>>>>>> Please find the real problem, fix the real problem and throughly explain
>>>>>>>> the real issue.
>>>>>>>>
>>>>>>>> I was suspecting some weird changes in timers. For !irqsafe timer I
>>>>>>>> think it is safe to use mutexes... but apparently not. Maybe a work
>>>>>>>> should be scheduled from function called by timer?
>>>>>>>>
>>>>>>>> Warning with debug atomic sleep:
>>>>>>>> [   16.047517] BUG: sleeping function called from invalid context at
>>>>>>>> ../kernel/locking/mutex.c:617
>>>>>>>> [   16.054866] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
>>>>>>>> [   16.061402] INFO: lockdep is turned off.
>>>>>>>> [   16.065281] Preemption disabled at:[<  (null)>]   (null)
>>>>>>>> [   16.070524]
>>>>>>>> [   16.072002] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
>>>>>>>> 4.4.0-rc1-00040-g28c429565d4f #290
>>>>>>>> [   16.080150] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>>>>>> [   16.086215] [<c0016780>] (unwind_backtrace) from [<c00132f0>]
>>>>>>>> (show_stack+0x10/0x14)
>>>>>>>> [   16.093938] [<c00132f0>] (show_stack) from [<c0223ba4>]
>>>>>>>> (dump_stack+0x70/0xbc)
>>>>>>>> [   16.101122] [<c0223ba4>] (dump_stack) from [<c05ed8e0>]
>>>>>>>> (mutex_lock_nested+0x24/0x474)
>>>>>>>> [   16.109009] [<c05ed8e0>] (mutex_lock_nested) from [<c0259154>]
>>>>>>>> (pwm_enable+0x20/0x7c)
>>>>>>>> [   16.116799] [<c0259154>] (pwm_enable) from [<c04400bc>]
>>>>>>>> (led_heartbeat_function+0xdc/0x140)
>>>>>>>> [   16.125119] [<c04400bc>] (led_heartbeat_function) from [<c00864c8>]
>>>>>>>> (call_timer_fn+0x6c/0xf4)
>>>>>>>> [   16.133606] [<c00864c8>] (call_timer_fn) from [<c00866f8>]
>>>>>>>> (run_timer_softirq+0x1a8/0x230)
>>>>>>>> [   16.141846] [<c00866f8>] (run_timer_softirq) from [<c0028e68>]
>>>>>>>> (__do_softirq+0x134/0x2c0)
>>>>>>>> [   16.149982] [<c0028e68>] (__do_softirq) from [<c0029334>]
>>>>>>>> (irq_exit+0xd0/0x114)
>>>>>>>> [   16.157255] [<c0029334>] (irq_exit) from [<c0076610>]
>>>>>>>> (__handle_domain_irq+0x70/0xe4)
>>>>>>>> [   16.165056] [<c0076610>] (__handle_domain_irq) from [<c00094e8>]
>>>>>>>> (gic_handle_irq+0x4c/0x94)
>>>>>>>> [   16.173376] [<c00094e8>] (gic_handle_irq) from [<c0013db8>]
>>>>>>>> (__irq_svc+0x58/0x98)
>>>>>>>> [   16.180817] Exception stack(0xc08cdf58 to 0xc08cdfa0)
>>>>>>>> [   16.185823] df40:
>>>>>>>>    c0010dcc 00000000
>>>>>>>> [   16.193997] df60: c08cdfa8 00000000 c05f57c4 00000000 c08ca520
>>>>>>>> c08ce4bc c08c7364 c08ce4b4
>>>>>>>> [   16.202141] df80: c09121ca 00000000 00000001 c08cdfa8 c0010dcc
>>>>>>>> c0010dd0 600f0013 ffffffff
>>>>>>>> [   16.210291] [<c0013db8>] (__irq_svc) from [<c0010dd0>]
>>>>>>>> (arch_cpu_idle+0x20/0x3c)
>>>>>>>> [   16.217661] [<c0010dd0>] (arch_cpu_idle) from [<c0063174>]
>>>>>>>> (cpu_startup_entry+0x17c/0x26c)
>>>>>>>> [   16.225899] [<c0063174>] (cpu_startup_entry) from [<c0860c40>]
>>>>>>>> (start_kernel+0x368/0x3d0)
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Krzysztof
>>>>>>>>
>>>>>>>>
>>>>>>>>> [    2.701737] Modules linked in:
>>>>>>>>> [    2.701748] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.4.0-rc1-xu4f
>>>>>>>>> #24
>>>>>>>>> [    2.701753] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>>>>>>> [    2.701787] [<c0015f48>] (unwind_backtrace) from [<c0012d04>]
>>>>>>>>> (show_stack+0x10/0x14)
>>>>>>>>> [    2.701808] [<c0012d04>] (show_stack) from [<c01f83fc>]
>>>>>>>>> (dump_stack+0x84/0xc4)
>>>>>>>>> [    2.701824] [<c01f83fc>] (dump_stack) from [<c0023494>]
>>>>>>>>> (warn_slowpath_common+0x80/0xb0)
>>>>>>>>> [    2.701836] [<c0023494>] (warn_slowpath_common) from [<c00234f4>]
>>>>>>>>> (warn_slowpath_fmt+0x30/0x40)
>>>>>>>>> [    2.701849] [<c00234f4>] (warn_slowpath_fmt) from [<c056e6b8>]
>>>>>>>>> (__mutex_lock_slowpath+0x3bc/0x404)
>>>>>>>>> [    2.701864] [<c056e6b8>] (__mutex_lock_slowpath) from [<c056e70c>]
>>>>>>>>> (mutex_lock+0xc/0x24)
>>>>>>>>> [    2.701884] [<c056e70c>] (mutex_lock) from [<c0228984>]
>>>>>>>>> (pwm_enable+0x20/0x7c)
>>>>>>>>> [    2.701903] [<c0228984>] (pwm_enable) from [<c03f0000>]
>>>>>>>>> (led_heartbeat_function+0x74/0x144)
>>>>>>>>> [    2.701919] [<c03f0000>] (led_heartbeat_function) from [<c0074368>]
>>>>>>>>> (call_timer_fn+0x24/0x98)
>>>>>>>>> [    2.701932] [<c0074368>] (call_timer_fn) from [<c007453c>]
>>>>>>>>> (run_timer_softirq+0x160/0x21c)
>>>>>>>>> [    2.701946] [<c007453c>] (run_timer_softirq) from [<c0026e10>]
>>>>>>>>> (__do_softirq+0x110/0x228)
>>>>>>>>> [    2.701959] [<c0026e10>] (__do_softirq) from [<c00271c8>]
>>>>>>>>> (irq_exit+0xc0/0xfc)
>>>>>>>>> [    2.701976] [<c00271c8>] (irq_exit) from [<c0065180>]
>>>>>>>>> (__handle_domain_irq+0x80/0xec)
>>>>>>>>> [    2.701990] [<c0065180>] (__handle_domain_irq) from [<c0009494>]
>>>>>>>>> (gic_handle_irq+0x54/0x94)
>>>>>>>>> [    2.702001] [<c0009494>] (gic_handle_irq) from [<c0013794>]
>>>>>>>>> (__irq_svc+0x54/0x90)
>>>>>>>>> [    2.702008] Exception stack(0xee8bdf88 to 0xee8bdfd0)
>>>>>>>>> [    2.702019] df80:                   00000001 00000000 00000000
>>>>>>>>> c001b720 ee8bc000 c0573354
>>>>>>>>> [    2.702031] dfa0: 00000000 00000000 ee8bdfe0 c07f92e4 c08004b4
>>>>>>>>> c08004bc f0806640 ee8bdfd8
>>>>>>>>> [    2.702039] dfc0: c0010180 c0010184 60000013 ffffffff
>>>>>>>>> [    2.702053] [<c0013794>] (__irq_svc) from [<c0010184>]
>>>>>>>>> (arch_cpu_idle+0x38/0x3c)
>>>>>>>>> [    2.702073] [<c0010184>] (arch_cpu_idle) from [<c0058ed4>]
>>>>>>>>> (cpu_startup_entry+0x1ec/0x270)
>>>>>>>>> [    2.702086] [<c0058ed4>] (cpu_startup_entry) from [<4000956c>]
>>>>>>>>> (0x4000956c)
>>>>>>>>> [    2.702093] ---[ end trace 539af70491f4f1a9 ]---
>>>>>>>>>
>>>>>>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>>>>>>> ---
>>>>>>>>>  drivers/pwm/core.c | 4 ----
>>>>>>>>>  1 file changed, 4 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>>>>>>>>> index d24ca5f..b8f035a 100644
>>>>>>>>> --- a/drivers/pwm/core.c
>>>>>>>>> +++ b/drivers/pwm/core.c
>>>>>>>>> @@ -506,16 +506,12 @@ int pwm_enable(struct pwm_device *pwm)
>>>>>>>>>       if (!pwm)
>>>>>>>>>               return -EINVAL;
>>>>>>>>>
>>>>>>>>> -     mutex_lock(&pwm->lock);
>>>>>>>>> -
>>>>>>>>>       if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
>>>>>>>>>               err = pwm->chip->ops->enable(pwm->chip, pwm);
>>>>>>>>>               if (err)
>>>>>>>>>                       clear_bit(PWMF_ENABLED, &pwm->flags);
>>>>>>>>>       }
>>>>>>>>>
>>>>>>>>> -     mutex_unlock(&pwm->lock);
>>>>>>>>> -
>>>>>>>>>       return err;
>>>>>>>>>  }
>>>>>>>>>  EXPORT_SYMBOL_GPL(pwm_enable);
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Adding Jonathan Richardson.
>>>>>>>
>>>>>>> Yes I am
>>>>>>>
>>>>>>> aware I am reverting some part of the d1cd21427747 ("pwm: Set enable state
>>>>>>> properly on failed call to enable")
>>>>>>>
>>>>>>> Please take a look at this below commit.
>>>>>>>
>>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pwm/core.c?id=d1cd21427747f15920cd726f5f67a07880e7dee4
>>>>>>>
>>>>>>> Actually reverting this change it work fine.
>>>>>>>
>>>>>>> This changes introduce the new mutex lock pwm->lock to protect enabled bit
>>>>>>> by drivers while setting polarity.
>>>>>>>
>>>>>>> Well pwm_set_polarity already acquires the pwm->lock and calls
>>>>>>> pwm_is_enabled function.
>>>>>>> Again within pwm_is_enabled we are trying to acquire the same mutex lock.
>>>>>>
>>>>>> You are describing a lockdown by trying to acquire the same mutex twice.
>>>>>>
>>>>>> However pwm_is_enabled() does not acquire mutex.
>>>>>>
>>>>>> Again, please look at generated warnings:
>>>>>> 1. BUG: sleeping function called from invalid context
>>>>>> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
>>>>>>
>>>>>> They are not related anyhow to what you described (acquiring already
>>>>>> locked mutex).
>>>>>>
>>>>>> Best regards,
>>>>>> Krzysztof
>>>>>
>>>>> My last reply mail went in HTML format so resend this.
>>>>>
>>>>> First it was a typo on my part.
>>>>> It not pwm_is_enabled function its pwm_enabled.
>>>>
>>>> There is no such function as "pwm_enabled".
>>>>
>>>> Sorry, I don't get your point.
>>>>
>>>> Instead of pasting some commit use a descriptive way to show the calls
>>>> leading lockdown. But please use real function names.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>
>>> Earlier my assumption of double mutex lock up totally rubbish.
>>>
>>> After reverting my changes and building image with CONFIG_DEBUG_ATOMIC_SLEEP.
>>>
>>> [  390.415370] BUG: sleeping function called from invalid context at
>>> kernel/locking/mutex.c:97
>>> [  390.422274] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
>>> [  390.428831] Preemption disabled at:[<  (null)>]   (null)
>>> [  391.970352] BUG: sleeping function called from invalid context at
>>> kernel/locking/mutex.c:97
>>> [  391.977251] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
>>> [  391.983814] Preemption disabled at:[<  (null)>]   (null)
>>> [  393.520376] BUG: sleeping function called from invalid context at
>>> kernel/locking/mutex.c:97
>>> [  393.527312] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
>>> [  393.533925] Preemption disabled at:[<  (null)>]   (null)
>>
>> Yes, now you pasted the same warning I did...
>>
>> This is still the same issue. I already wrote it:
>>> 1. BUG: sleeping function called from invalid context
>>> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
>>
>> We can repeat it many times but that won't change anything...
> 
> This looks like you're simply running the leds-pwm driver with a PWM
> that isn't properly marked as potentially sleeping. Unfortunately the
> introduction of the mutex in d1cd21427747 ("pwm: Set enable state
> properly on failed call to enable") effectively makes all PWM drivers
> potentially sleeping. That in turn makes the .can_sleep field obsolete
> since all drivers can now sleep.
> 
> Any objections to simply removing it and make all users use a workqueue
> or some such if they need to control a PWM as a result of an interrupt
> trigger?
> 
> Thierry
> 

No objections here. The function is somewhat new and I didn't notice it.
It should have been removed in my last patch set.

Thanks,
Jon

Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable

[Anand Moon]

Hi Krzysztof,

On 22 November 2015 at 05:43, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> 2015-11-22 3:14 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>> Hi Krzysztof,
>>
>> On 21 November 2015 at 18:37, Krzysztof Kozlowski
>> <k.kozlowski@samsung.com> wrote:
>>> 2015-11-21 21:11 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>>> hi Krzysztof,
>>>>
>>>> On 21 November 2015 at 15:22, Krzysztof Kozlowski
>>>> <k.kozlowski@samsung.com> wrote:
>>>>> 2015-11-21 18:40 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>>>>> hi Krzysztof,
>>>>>>
>>>>>> On 21 November 2015 at 09:56, Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> W dniu 21.11.2015 o 01:59, Anand Moon pisze:
>>>>>>> > Commit d1cd21427747f15920cd726f5f67a07880e7dee4
>>>>>>> > (pwm: Set enable state properly on failed call to enable)
>>>>>>> > introduce double lock of mutex on pwm_enable
>>>>>>> >
>>>>>>> > Changes fix the following debug lock warning.
>>>>>>> >
>>>>>>> > [    2.701720] WARNING: CPU: 3 PID: 0 at kernel/locking/mutex.c:526
>>>>>>> > __mutex_lock_slowpath+0x3bc/0x404()
>>>>>>> > [    2.701731] DEBUG_LOCKS_WARN_ON(in_interrupt())
>>>>>>>
>>>>>>> Hi Anand!
>>>>>>>
>>>>>>> Yeah, I am hitting this as well. Annoying. However your description is
>>>>>>> inaccurate. Double lock of mutex does not cause warnings of sleeping or
>>>>>>> locking in atomic context (if you build with debug sleep atomic you will
>>>>>>> see different warning). More over you are partially reverting the commit
>>>>>>> d1cd21427747 ("pwm: Set enable state properly on failed call to enable")
>>>>>>> without proper explanation:
>>>>>>> 1. why this locking is inappropriate;
>>>>>>> 2. why it is safe to remove the mutex_lock().
>>>>>>>
>>>>>>> Please find the real problem, fix the real problem and throughly explain
>>>>>>> the real issue.
>>>>>>>
>>>>>>> I was suspecting some weird changes in timers. For !irqsafe timer I
>>>>>>> think it is safe to use mutexes... but apparently not. Maybe a work
>>>>>>> should be scheduled from function called by timer?
>>>>>>>
>>>>>>> Warning with debug atomic sleep:
>>>>>>> [   16.047517] BUG: sleeping function called from invalid context at
>>>>>>> ../kernel/locking/mutex.c:617
>>>>>>> [   16.054866] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
>>>>>>> [   16.061402] INFO: lockdep is turned off.
>>>>>>> [   16.065281] Preemption disabled at:[<  (null)>]   (null)
>>>>>>> [   16.070524]
>>>>>>> [   16.072002] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
>>>>>>> 4.4.0-rc1-00040-g28c429565d4f #290
>>>>>>> [   16.080150] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>>>>> [   16.086215] [<c0016780>] (unwind_backtrace) from [<c00132f0>]
>>>>>>> (show_stack+0x10/0x14)
>>>>>>> [   16.093938] [<c00132f0>] (show_stack) from [<c0223ba4>]
>>>>>>> (dump_stack+0x70/0xbc)
>>>>>>> [   16.101122] [<c0223ba4>] (dump_stack) from [<c05ed8e0>]
>>>>>>> (mutex_lock_nested+0x24/0x474)
>>>>>>> [   16.109009] [<c05ed8e0>] (mutex_lock_nested) from [<c0259154>]
>>>>>>> (pwm_enable+0x20/0x7c)
>>>>>>> [   16.116799] [<c0259154>] (pwm_enable) from [<c04400bc>]
>>>>>>> (led_heartbeat_function+0xdc/0x140)
>>>>>>> [   16.125119] [<c04400bc>] (led_heartbeat_function) from [<c00864c8>]
>>>>>>> (call_timer_fn+0x6c/0xf4)
>>>>>>> [   16.133606] [<c00864c8>] (call_timer_fn) from [<c00866f8>]
>>>>>>> (run_timer_softirq+0x1a8/0x230)
>>>>>>> [   16.141846] [<c00866f8>] (run_timer_softirq) from [<c0028e68>]
>>>>>>> (__do_softirq+0x134/0x2c0)
>>>>>>> [   16.149982] [<c0028e68>] (__do_softirq) from [<c0029334>]
>>>>>>> (irq_exit+0xd0/0x114)
>>>>>>> [   16.157255] [<c0029334>] (irq_exit) from [<c0076610>]
>>>>>>> (__handle_domain_irq+0x70/0xe4)
>>>>>>> [   16.165056] [<c0076610>] (__handle_domain_irq) from [<c00094e8>]
>>>>>>> (gic_handle_irq+0x4c/0x94)
>>>>>>> [   16.173376] [<c00094e8>] (gic_handle_irq) from [<c0013db8>]
>>>>>>> (__irq_svc+0x58/0x98)
>>>>>>> [   16.180817] Exception stack(0xc08cdf58 to 0xc08cdfa0)
>>>>>>> [   16.185823] df40:
>>>>>>>    c0010dcc 00000000
>>>>>>> [   16.193997] df60: c08cdfa8 00000000 c05f57c4 00000000 c08ca520
>>>>>>> c08ce4bc c08c7364 c08ce4b4
>>>>>>> [   16.202141] df80: c09121ca 00000000 00000001 c08cdfa8 c0010dcc
>>>>>>> c0010dd0 600f0013 ffffffff
>>>>>>> [   16.210291] [<c0013db8>] (__irq_svc) from [<c0010dd0>]
>>>>>>> (arch_cpu_idle+0x20/0x3c)
>>>>>>> [   16.217661] [<c0010dd0>] (arch_cpu_idle) from [<c0063174>]
>>>>>>> (cpu_startup_entry+0x17c/0x26c)
>>>>>>> [   16.225899] [<c0063174>] (cpu_startup_entry) from [<c0860c40>]
>>>>>>> (start_kernel+0x368/0x3d0)
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Krzysztof
>>>>>>>
>>>>>>>
>>>>>>> > [    2.701737] Modules linked in:
>>>>>>> > [    2.701748] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.4.0-rc1-xu4f
>>>>>>> > #24
>>>>>>> > [    2.701753] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>>>>> > [    2.701787] [<c0015f48>] (unwind_backtrace) from [<c0012d04>]
>>>>>>> > (show_stack+0x10/0x14)
>>>>>>> > [    2.701808] [<c0012d04>] (show_stack) from [<c01f83fc>]
>>>>>>> > (dump_stack+0x84/0xc4)
>>>>>>> > [    2.701824] [<c01f83fc>] (dump_stack) from [<c0023494>]
>>>>>>> > (warn_slowpath_common+0x80/0xb0)
>>>>>>> > [    2.701836] [<c0023494>] (warn_slowpath_common) from [<c00234f4>]
>>>>>>> > (warn_slowpath_fmt+0x30/0x40)
>>>>>>> > [    2.701849] [<c00234f4>] (warn_slowpath_fmt) from [<c056e6b8>]
>>>>>>> > (__mutex_lock_slowpath+0x3bc/0x404)
>>>>>>> > [    2.701864] [<c056e6b8>] (__mutex_lock_slowpath) from [<c056e70c>]
>>>>>>> > (mutex_lock+0xc/0x24)
>>>>>>> > [    2.701884] [<c056e70c>] (mutex_lock) from [<c0228984>]
>>>>>>> > (pwm_enable+0x20/0x7c)
>>>>>>> > [    2.701903] [<c0228984>] (pwm_enable) from [<c03f0000>]
>>>>>>> > (led_heartbeat_function+0x74/0x144)
>>>>>>> > [    2.701919] [<c03f0000>] (led_heartbeat_function) from [<c0074368>]
>>>>>>> > (call_timer_fn+0x24/0x98)
>>>>>>> > [    2.701932] [<c0074368>] (call_timer_fn) from [<c007453c>]
>>>>>>> > (run_timer_softirq+0x160/0x21c)
>>>>>>> > [    2.701946] [<c007453c>] (run_timer_softirq) from [<c0026e10>]
>>>>>>> > (__do_softirq+0x110/0x228)
>>>>>>> > [    2.701959] [<c0026e10>] (__do_softirq) from [<c00271c8>]
>>>>>>> > (irq_exit+0xc0/0xfc)
>>>>>>> > [    2.701976] [<c00271c8>] (irq_exit) from [<c0065180>]
>>>>>>> > (__handle_domain_irq+0x80/0xec)
>>>>>>> > [    2.701990] [<c0065180>] (__handle_domain_irq) from [<c0009494>]
>>>>>>> > (gic_handle_irq+0x54/0x94)
>>>>>>> > [    2.702001] [<c0009494>] (gic_handle_irq) from [<c0013794>]
>>>>>>> > (__irq_svc+0x54/0x90)
>>>>>>> > [    2.702008] Exception stack(0xee8bdf88 to 0xee8bdfd0)
>>>>>>> > [    2.702019] df80:                   00000001 00000000 00000000
>>>>>>> > c001b720 ee8bc000 c0573354
>>>>>>> > [    2.702031] dfa0: 00000000 00000000 ee8bdfe0 c07f92e4 c08004b4
>>>>>>> > c08004bc f0806640 ee8bdfd8
>>>>>>> > [    2.702039] dfc0: c0010180 c0010184 60000013 ffffffff
>>>>>>> > [    2.702053] [<c0013794>] (__irq_svc) from [<c0010184>]
>>>>>>> > (arch_cpu_idle+0x38/0x3c)
>>>>>>> > [    2.702073] [<c0010184>] (arch_cpu_idle) from [<c0058ed4>]
>>>>>>> > (cpu_startup_entry+0x1ec/0x270)
>>>>>>> > [    2.702086] [<c0058ed4>] (cpu_startup_entry) from [<4000956c>]
>>>>>>> > (0x4000956c)
>>>>>>> > [    2.702093] ---[ end trace 539af70491f4f1a9 ]---
>>>>>>> >
>>>>>>> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>>>>> > ---
>>>>>>> >  drivers/pwm/core.c | 4 ----
>>>>>>> >  1 file changed, 4 deletions(-)
>>>>>>> >
>>>>>>> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>>>>>>> > index d24ca5f..b8f035a 100644
>>>>>>> > --- a/drivers/pwm/core.c
>>>>>>> > +++ b/drivers/pwm/core.c
>>>>>>> > @@ -506,16 +506,12 @@ int pwm_enable(struct pwm_device *pwm)
>>>>>>> >       if (!pwm)
>>>>>>> >               return -EINVAL;
>>>>>>> >
>>>>>>> > -     mutex_lock(&pwm->lock);
>>>>>>> > -
>>>>>>> >       if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
>>>>>>> >               err = pwm->chip->ops->enable(pwm->chip, pwm);
>>>>>>> >               if (err)
>>>>>>> >                       clear_bit(PWMF_ENABLED, &pwm->flags);
>>>>>>> >       }
>>>>>>> >
>>>>>>> > -     mutex_unlock(&pwm->lock);
>>>>>>> > -
>>>>>>> >       return err;
>>>>>>> >  }
>>>>>>> >  EXPORT_SYMBOL_GPL(pwm_enable);
>>>>>>> >
>>>>>>
>>>>>>
>>>>>> Adding Jonathan Richardson.
>>>>>>
>>>>>> Yes I am
>>>>>>
>>>>>> aware I am reverting some part of the d1cd21427747 ("pwm: Set enable state
>>>>>> properly on failed call to enable")
>>>>>>
>>>>>> Please take a look at this below commit.
>>>>>>
>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pwm/core.c?id=d1cd21427747f15920cd726f5f67a07880e7dee4
>>>>>>
>>>>>> Actually reverting this change it work fine.
>>>>>>
>>>>>> This changes introduce the new mutex lock pwm->lock to protect enabled bit
>>>>>> by drivers while setting polarity.
>>>>>>
>>>>>> Well pwm_set_polarity already acquires the pwm->lock and calls
>>>>>> pwm_is_enabled function.
>>>>>> Again within pwm_is_enabled we are trying to acquire the same mutex lock.
>>>>>
>>>>> You are describing a lockdown by trying to acquire the same mutex twice.
>>>>>
>>>>> However pwm_is_enabled() does not acquire mutex.
>>>>>
>>>>> Again, please look at generated warnings:
>>>>> 1. BUG: sleeping function called from invalid context
>>>>> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
>>>>>
>>>>> They are not related anyhow to what you described (acquiring already
>>>>> locked mutex).
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>
>>>> My last reply mail went in HTML format so resend this.
>>>>
>>>> First it was a typo on my part.
>>>> It not pwm_is_enabled function its pwm_enabled.
>>>
>>> There is no such function as "pwm_enabled".
>>>
>>> Sorry, I don't get your point.
>>>
>>> Instead of pasting some commit use a descriptive way to show the calls
>>> leading lockdown. But please use real function names.
>>>
>>> Best regards,
>>> Krzysztof
>>
>> Earlier my assumption of double mutex lock up totally rubbish.
>>
>> After reverting my changes and building image with CONFIG_DEBUG_ATOMIC_SLEEP.
>>
>> [  390.415370] BUG: sleeping function called from invalid context at
>> kernel/locking/mutex.c:97
>> [  390.422274] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
>> [  390.428831] Preemption disabled at:[<  (null)>]   (null)
>> [  391.970352] BUG: sleeping function called from invalid context at
>> kernel/locking/mutex.c:97
>> [  391.977251] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
>> [  391.983814] Preemption disabled at:[<  (null)>]   (null)
>> [  393.520376] BUG: sleeping function called from invalid context at
>> kernel/locking/mutex.c:97
>> [  393.527312] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
>> [  393.533925] Preemption disabled at:[<  (null)>]   (null)
>
> Yes, now you pasted the same warning I did...
>
> This is still the same issue. I already wrote it:
>> 1. BUG: sleeping function called from invalid context
>> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
>
> We can repeat it many times but that won't change anything...
>
> Best regards,
> Krzysztof

Would you consider below changes to fix the above issue.
I have tested this change by enabling CONFIG_DEBUG_ATOMIC_SLEEP
And I don't observed issue.

1. BUG: sleeping function called from invalid context
2. DEBUG_LOCKS_WARN_ON(in_interrupt())

Please share your thought on this changes.

root@odroidxu4:/usr/src/odroidxu3-4.y-devel# git diff drivers/pwm/core.c
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index d24ca5f..f3f6cf9 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -506,6 +506,9 @@ int pwm_enable(struct pwm_device *pwm)
        if (!pwm)
                return -EINVAL;

+       if (!mutex_is_locked(&pwm->lock))
+               return -EINVAL;
+
        mutex_lock(&pwm->lock);

        if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {

-Anand Moon

Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable

[Anand Moon]

Hi Krzysztof,

On 11 December 2015 at 09:37, Anand Moon <linux.amoon@gmail.com> wrote:
> Hi Krzysztof,
>
> On 22 November 2015 at 05:43, Krzysztof Kozlowski
> <k.kozlowski@samsung.com> wrote:
>> 2015-11-22 3:14 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>> Hi Krzysztof,
>>>
>>> On 21 November 2015 at 18:37, Krzysztof Kozlowski
>>> <k.kozlowski@samsung.com> wrote:
>>>> 2015-11-21 21:11 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>>>> hi Krzysztof,
>>>>>
>>>>> On 21 November 2015 at 15:22, Krzysztof Kozlowski
>>>>> <k.kozlowski@samsung.com> wrote:
>>>>>> 2015-11-21 18:40 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>>>>>> hi Krzysztof,
>>>>>>>
>>>>>>> On 21 November 2015 at 09:56, Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> W dniu 21.11.2015 o 01:59, Anand Moon pisze:
>>>>>>>> > Commit d1cd21427747f15920cd726f5f67a07880e7dee4
>>>>>>>> > (pwm: Set enable state properly on failed call to enable)
>>>>>>>> > introduce double lock of mutex on pwm_enable
>>>>>>>> >
>>>>>>>> > Changes fix the following debug lock warning.
>>>>>>>> >
>>>>>>>> > [    2.701720] WARNING: CPU: 3 PID: 0 at kernel/locking/mutex.c:526
>>>>>>>> > __mutex_lock_slowpath+0x3bc/0x404()
>>>>>>>> > [    2.701731] DEBUG_LOCKS_WARN_ON(in_interrupt())
>>>>>>>>
>>>>>>>> Hi Anand!
>>>>>>>>
>>>>>>>> Yeah, I am hitting this as well. Annoying. However your description is
>>>>>>>> inaccurate. Double lock of mutex does not cause warnings of sleeping or
>>>>>>>> locking in atomic context (if you build with debug sleep atomic you will
>>>>>>>> see different warning). More over you are partially reverting the commit
>>>>>>>> d1cd21427747 ("pwm: Set enable state properly on failed call to enable")
>>>>>>>> without proper explanation:
>>>>>>>> 1. why this locking is inappropriate;
>>>>>>>> 2. why it is safe to remove the mutex_lock().
>>>>>>>>
>>>>>>>> Please find the real problem, fix the real problem and throughly explain
>>>>>>>> the real issue.
>>>>>>>>
>>>>>>>> I was suspecting some weird changes in timers. For !irqsafe timer I
>>>>>>>> think it is safe to use mutexes... but apparently not. Maybe a work
>>>>>>>> should be scheduled from function called by timer?
>>>>>>>>
>>>>>>>> Warning with debug atomic sleep:
>>>>>>>> [   16.047517] BUG: sleeping function called from invalid context at
>>>>>>>> ../kernel/locking/mutex.c:617
>>>>>>>> [   16.054866] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
>>>>>>>> [   16.061402] INFO: lockdep is turned off.
>>>>>>>> [   16.065281] Preemption disabled at:[<  (null)>]   (null)
>>>>>>>> [   16.070524]
>>>>>>>> [   16.072002] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
>>>>>>>> 4.4.0-rc1-00040-g28c429565d4f #290
>>>>>>>> [   16.080150] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>>>>>> [   16.086215] [<c0016780>] (unwind_backtrace) from [<c00132f0>]
>>>>>>>> (show_stack+0x10/0x14)
>>>>>>>> [   16.093938] [<c00132f0>] (show_stack) from [<c0223ba4>]
>>>>>>>> (dump_stack+0x70/0xbc)
>>>>>>>> [   16.101122] [<c0223ba4>] (dump_stack) from [<c05ed8e0>]
>>>>>>>> (mutex_lock_nested+0x24/0x474)
>>>>>>>> [   16.109009] [<c05ed8e0>] (mutex_lock_nested) from [<c0259154>]
>>>>>>>> (pwm_enable+0x20/0x7c)
>>>>>>>> [   16.116799] [<c0259154>] (pwm_enable) from [<c04400bc>]
>>>>>>>> (led_heartbeat_function+0xdc/0x140)
>>>>>>>> [   16.125119] [<c04400bc>] (led_heartbeat_function) from [<c00864c8>]
>>>>>>>> (call_timer_fn+0x6c/0xf4)
>>>>>>>> [   16.133606] [<c00864c8>] (call_timer_fn) from [<c00866f8>]
>>>>>>>> (run_timer_softirq+0x1a8/0x230)
>>>>>>>> [   16.141846] [<c00866f8>] (run_timer_softirq) from [<c0028e68>]
>>>>>>>> (__do_softirq+0x134/0x2c0)
>>>>>>>> [   16.149982] [<c0028e68>] (__do_softirq) from [<c0029334>]
>>>>>>>> (irq_exit+0xd0/0x114)
>>>>>>>> [   16.157255] [<c0029334>] (irq_exit) from [<c0076610>]
>>>>>>>> (__handle_domain_irq+0x70/0xe4)
>>>>>>>> [   16.165056] [<c0076610>] (__handle_domain_irq) from [<c00094e8>]
>>>>>>>> (gic_handle_irq+0x4c/0x94)
>>>>>>>> [   16.173376] [<c00094e8>] (gic_handle_irq) from [<c0013db8>]
>>>>>>>> (__irq_svc+0x58/0x98)
>>>>>>>> [   16.180817] Exception stack(0xc08cdf58 to 0xc08cdfa0)
>>>>>>>> [   16.185823] df40:
>>>>>>>>    c0010dcc 00000000
>>>>>>>> [   16.193997] df60: c08cdfa8 00000000 c05f57c4 00000000 c08ca520
>>>>>>>> c08ce4bc c08c7364 c08ce4b4
>>>>>>>> [   16.202141] df80: c09121ca 00000000 00000001 c08cdfa8 c0010dcc
>>>>>>>> c0010dd0 600f0013 ffffffff
>>>>>>>> [   16.210291] [<c0013db8>] (__irq_svc) from [<c0010dd0>]
>>>>>>>> (arch_cpu_idle+0x20/0x3c)
>>>>>>>> [   16.217661] [<c0010dd0>] (arch_cpu_idle) from [<c0063174>]
>>>>>>>> (cpu_startup_entry+0x17c/0x26c)
>>>>>>>> [   16.225899] [<c0063174>] (cpu_startup_entry) from [<c0860c40>]
>>>>>>>> (start_kernel+0x368/0x3d0)
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Krzysztof
>>>>>>>>
>>>>>>>>
>>>>>>>> > [    2.701737] Modules linked in:
>>>>>>>> > [    2.701748] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.4.0-rc1-xu4f
>>>>>>>> > #24
>>>>>>>> > [    2.701753] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>>>>>>>> > [    2.701787] [<c0015f48>] (unwind_backtrace) from [<c0012d04>]
>>>>>>>> > (show_stack+0x10/0x14)
>>>>>>>> > [    2.701808] [<c0012d04>] (show_stack) from [<c01f83fc>]
>>>>>>>> > (dump_stack+0x84/0xc4)
>>>>>>>> > [    2.701824] [<c01f83fc>] (dump_stack) from [<c0023494>]
>>>>>>>> > (warn_slowpath_common+0x80/0xb0)
>>>>>>>> > [    2.701836] [<c0023494>] (warn_slowpath_common) from [<c00234f4>]
>>>>>>>> > (warn_slowpath_fmt+0x30/0x40)
>>>>>>>> > [    2.701849] [<c00234f4>] (warn_slowpath_fmt) from [<c056e6b8>]
>>>>>>>> > (__mutex_lock_slowpath+0x3bc/0x404)
>>>>>>>> > [    2.701864] [<c056e6b8>] (__mutex_lock_slowpath) from [<c056e70c>]
>>>>>>>> > (mutex_lock+0xc/0x24)
>>>>>>>> > [    2.701884] [<c056e70c>] (mutex_lock) from [<c0228984>]
>>>>>>>> > (pwm_enable+0x20/0x7c)
>>>>>>>> > [    2.701903] [<c0228984>] (pwm_enable) from [<c03f0000>]
>>>>>>>> > (led_heartbeat_function+0x74/0x144)
>>>>>>>> > [    2.701919] [<c03f0000>] (led_heartbeat_function) from [<c0074368>]
>>>>>>>> > (call_timer_fn+0x24/0x98)
>>>>>>>> > [    2.701932] [<c0074368>] (call_timer_fn) from [<c007453c>]
>>>>>>>> > (run_timer_softirq+0x160/0x21c)
>>>>>>>> > [    2.701946] [<c007453c>] (run_timer_softirq) from [<c0026e10>]
>>>>>>>> > (__do_softirq+0x110/0x228)
>>>>>>>> > [    2.701959] [<c0026e10>] (__do_softirq) from [<c00271c8>]
>>>>>>>> > (irq_exit+0xc0/0xfc)
>>>>>>>> > [    2.701976] [<c00271c8>] (irq_exit) from [<c0065180>]
>>>>>>>> > (__handle_domain_irq+0x80/0xec)
>>>>>>>> > [    2.701990] [<c0065180>] (__handle_domain_irq) from [<c0009494>]
>>>>>>>> > (gic_handle_irq+0x54/0x94)
>>>>>>>> > [    2.702001] [<c0009494>] (gic_handle_irq) from [<c0013794>]
>>>>>>>> > (__irq_svc+0x54/0x90)
>>>>>>>> > [    2.702008] Exception stack(0xee8bdf88 to 0xee8bdfd0)
>>>>>>>> > [    2.702019] df80:                   00000001 00000000 00000000
>>>>>>>> > c001b720 ee8bc000 c0573354
>>>>>>>> > [    2.702031] dfa0: 00000000 00000000 ee8bdfe0 c07f92e4 c08004b4
>>>>>>>> > c08004bc f0806640 ee8bdfd8
>>>>>>>> > [    2.702039] dfc0: c0010180 c0010184 60000013 ffffffff
>>>>>>>> > [    2.702053] [<c0013794>] (__irq_svc) from [<c0010184>]
>>>>>>>> > (arch_cpu_idle+0x38/0x3c)
>>>>>>>> > [    2.702073] [<c0010184>] (arch_cpu_idle) from [<c0058ed4>]
>>>>>>>> > (cpu_startup_entry+0x1ec/0x270)
>>>>>>>> > [    2.702086] [<c0058ed4>] (cpu_startup_entry) from [<4000956c>]
>>>>>>>> > (0x4000956c)
>>>>>>>> > [    2.702093] ---[ end trace 539af70491f4f1a9 ]---
>>>>>>>> >
>>>>>>>> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>>>>>> > ---
>>>>>>>> >  drivers/pwm/core.c | 4 ----
>>>>>>>> >  1 file changed, 4 deletions(-)
>>>>>>>> >
>>>>>>>> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>>>>>>>> > index d24ca5f..b8f035a 100644
>>>>>>>> > --- a/drivers/pwm/core.c
>>>>>>>> > +++ b/drivers/pwm/core.c
>>>>>>>> > @@ -506,16 +506,12 @@ int pwm_enable(struct pwm_device *pwm)
>>>>>>>> >       if (!pwm)
>>>>>>>> >               return -EINVAL;
>>>>>>>> >
>>>>>>>> > -     mutex_lock(&pwm->lock);
>>>>>>>> > -
>>>>>>>> >       if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
>>>>>>>> >               err = pwm->chip->ops->enable(pwm->chip, pwm);
>>>>>>>> >               if (err)
>>>>>>>> >                       clear_bit(PWMF_ENABLED, &pwm->flags);
>>>>>>>> >       }
>>>>>>>> >
>>>>>>>> > -     mutex_unlock(&pwm->lock);
>>>>>>>> > -
>>>>>>>> >       return err;
>>>>>>>> >  }
>>>>>>>> >  EXPORT_SYMBOL_GPL(pwm_enable);
>>>>>>>> >
>>>>>>>
>>>>>>>
>>>>>>> Adding Jonathan Richardson.
>>>>>>>
>>>>>>> Yes I am
>>>>>>>
>>>>>>> aware I am reverting some part of the d1cd21427747 ("pwm: Set enable state
>>>>>>> properly on failed call to enable")
>>>>>>>
>>>>>>> Please take a look at this below commit.
>>>>>>>
>>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pwm/core.c?id=d1cd21427747f15920cd726f5f67a07880e7dee4
>>>>>>>
>>>>>>> Actually reverting this change it work fine.
>>>>>>>
>>>>>>> This changes introduce the new mutex lock pwm->lock to protect enabled bit
>>>>>>> by drivers while setting polarity.
>>>>>>>
>>>>>>> Well pwm_set_polarity already acquires the pwm->lock and calls
>>>>>>> pwm_is_enabled function.
>>>>>>> Again within pwm_is_enabled we are trying to acquire the same mutex lock.
>>>>>>
>>>>>> You are describing a lockdown by trying to acquire the same mutex twice.
>>>>>>
>>>>>> However pwm_is_enabled() does not acquire mutex.
>>>>>>
>>>>>> Again, please look at generated warnings:
>>>>>> 1. BUG: sleeping function called from invalid context
>>>>>> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
>>>>>>
>>>>>> They are not related anyhow to what you described (acquiring already
>>>>>> locked mutex).
>>>>>>
>>>>>> Best regards,
>>>>>> Krzysztof
>>>>>
>>>>> My last reply mail went in HTML format so resend this.
>>>>>
>>>>> First it was a typo on my part.
>>>>> It not pwm_is_enabled function its pwm_enabled.
>>>>
>>>> There is no such function as "pwm_enabled".
>>>>
>>>> Sorry, I don't get your point.
>>>>
>>>> Instead of pasting some commit use a descriptive way to show the calls
>>>> leading lockdown. But please use real function names.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>
>>> Earlier my assumption of double mutex lock up totally rubbish.
>>>
>>> After reverting my changes and building image with CONFIG_DEBUG_ATOMIC_SLEEP.
>>>
>>> [  390.415370] BUG: sleeping function called from invalid context at
>>> kernel/locking/mutex.c:97
>>> [  390.422274] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
>>> [  390.428831] Preemption disabled at:[<  (null)>]   (null)
>>> [  391.970352] BUG: sleeping function called from invalid context at
>>> kernel/locking/mutex.c:97
>>> [  391.977251] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
>>> [  391.983814] Preemption disabled at:[<  (null)>]   (null)
>>> [  393.520376] BUG: sleeping function called from invalid context at
>>> kernel/locking/mutex.c:97
>>> [  393.527312] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
>>> [  393.533925] Preemption disabled at:[<  (null)>]   (null)
>>
>> Yes, now you pasted the same warning I did...
>>
>> This is still the same issue. I already wrote it:
>>> 1. BUG: sleeping function called from invalid context
>>> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
>>
>> We can repeat it many times but that won't change anything...
>>
>> Best regards,
>> Krzysztof
>
> Would you consider below changes to fix the above issue.
> I have tested this change by enabling CONFIG_DEBUG_ATOMIC_SLEEP
> And I don't observed issue.
>
> 1. BUG: sleeping function called from invalid context
> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
>
> Please share your thought on this changes.
>
> root@odroidxu4:/usr/src/odroidxu3-4.y-devel# git diff drivers/pwm/core.c
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index d24ca5f..f3f6cf9 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -506,6 +506,9 @@ int pwm_enable(struct pwm_device *pwm)
>         if (!pwm)
>                 return -EINVAL;
>
> +       if (!mutex_is_locked(&pwm->lock))
> +               return -EINVAL;
> +
>         mutex_lock(&pwm->lock);
>
>         if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
>
> -Anand Moon

Please ignore this changes.

-Anand Moon

Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable

[Krzysztof Kozlowski]

On 11.12.2015 13:07, Anand Moon wrote:
> Hi Krzysztof,
> 
> On 22 November 2015 at 05:43, Krzysztof Kozlowski
> <k.kozlowski@samsung.com> wrote:
>> 2015-11-22 3:14 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>> Hi Krzysztof,

[...]

>> Yes, now you pasted the same warning I did...
>>
>> This is still the same issue. I already wrote it:
>>> 1. BUG: sleeping function called from invalid context
>>> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
>>
>> We can repeat it many times but that won't change anything...
>>
>> Best regards,
>> Krzysztof
> 
> Would you consider below changes to fix the above issue.
> I have tested this change by enabling CONFIG_DEBUG_ATOMIC_SLEEP
> And I don't observed issue.
> 
> 1. BUG: sleeping function called from invalid context
> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
> 
> Please share your thought on this changes.
> 
> root@odroidxu4:/usr/src/odroidxu3-4.y-devel# git diff drivers/pwm/core.c
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index d24ca5f..f3f6cf9 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -506,6 +506,9 @@ int pwm_enable(struct pwm_device *pwm)
>         if (!pwm)
>                 return -EINVAL;
> 
> +       if (!mutex_is_locked(&pwm->lock))
> +               return -EINVAL;
> +
>         mutex_lock(&pwm->lock);
> 
>         if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {

First of all, Thierry suggested way of fixing this:
"Any objections to simply removing it and make all users use a workqueue
or some such if they need to control a PWM as a result of an interrupt
trigger?"
what is wrong with his approach?

Second, you are writing something that looks like mutex-try-lock...
which will fail the pwm_enable(). IMHO this *hides* the real issue and
does not solve anything (except hiding also the warning).

Best regards,
Krzysztof

Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable

[Anand Moon]

Hi Krzysztof

On 11 December 2015 at 09:53, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> On 11.12.2015 13:07, Anand Moon wrote:
>> Hi Krzysztof,
>>
>> On 22 November 2015 at 05:43, Krzysztof Kozlowski
>> <k.kozlowski@samsung.com> wrote:
>>> 2015-11-22 3:14 GMT+09:00 Anand Moon <linux.amoon@gmail.com>:
>>>> Hi Krzysztof,
>
> [...]
>
>>> Yes, now you pasted the same warning I did...
>>>
>>> This is still the same issue. I already wrote it:
>>>> 1. BUG: sleeping function called from invalid context
>>>> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
>>>
>>> We can repeat it many times but that won't change anything...
>>>
>>> Best regards,
>>> Krzysztof
>>
>> Would you consider below changes to fix the above issue.
>> I have tested this change by enabling CONFIG_DEBUG_ATOMIC_SLEEP
>> And I don't observed issue.
>>
>> 1. BUG: sleeping function called from invalid context
>> 2. DEBUG_LOCKS_WARN_ON(in_interrupt())
>>
>> Please share your thought on this changes.
>>
>> root@odroidxu4:/usr/src/odroidxu3-4.y-devel# git diff drivers/pwm/core.c
>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>> index d24ca5f..f3f6cf9 100644
>> --- a/drivers/pwm/core.c
>> +++ b/drivers/pwm/core.c
>> @@ -506,6 +506,9 @@ int pwm_enable(struct pwm_device *pwm)
>>         if (!pwm)
>>                 return -EINVAL;
>>
>> +       if (!mutex_is_locked(&pwm->lock))
>> +               return -EINVAL;
>> +
>>         mutex_lock(&pwm->lock);
>>
>>         if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
>
> First of all, Thierry suggested way of fixing this:
> "Any objections to simply removing it and make all users use a workqueue
> or some such if they need to control a PWM as a result of an interrupt
> trigger?"

You are correct, current design need to be changes.

> what is wrong with his approach?
>

pwm->lock is locked but it never get unlocked, although the code looks correct.

> Second, you are writing something that looks like mutex-try-lock...
> which will fail the pwm_enable(). IMHO this *hides* the real issue and
> does not solve anything (except hiding also the warning).

Thanks for the inputs. I have realized my mistake.

>
> Best regards,
> Krzysztof

-Anand Moon