Message-ID: <CANAwSgRXZxV=JJD8hyMdi9_aHK88zeM5WxYw4pwSvTL3v5F22A@mail.gmail.com>
Date:	Fri, 11 Dec 2015 09:52:42 +0530
From:	Anand Moon <linux.amoon@...il.com>
To:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
Cc:	Guenter Roeck <linux@...ck-us.net>,
	Thierry Reding <thierry.reding@...il.com>,
	Javier Martinez Canillas <javier@....samsung.com>,
	linux-pwm@...r.kernel.org,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	"linux-samsung-soc@...r.kernel.org" 
	<linux-samsung-soc@...r.kernel.org>,
	Jonathan Richardson <jonathar@...adcom.com>
Subject: Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable

Hi Krzysztof,

On 11 December 2015 at 09:37, Anand Moon <linux.amoon@...il.com> wrote:
> Hi Krzysztof,
>
> On 22 November 2015 at 05:43, Krzysztof Kozlowski
> <k.kozlowski@...sung.com> wrote:
>> 2015-11-22 3:14 GMT+09:00 Anand Moon <linux.amoon@...il.com>:
>>> Hi Krzysztof,
>>>
>>> On 21 November 2015 at 18:37, Krzysztof Kozlowski
>>> <k.kozlowski@...sung.com> wrote:
>>>> 2015-11-21 21:11 GMT+09:00 Anand Moon <linux.amoon@...il.com>:
>>>>> hi Krzysztof,
>>>>>
>>>>> On 21 November 2015 at 15:22, Krzysztof Kozlowski
>>>>> <k.kozlowski@...sung.com> wrote:
>>>>>> 2015-11-21 18:40 GMT+09:00 Anand Moon <linux.amoon@...il.com>:
>>>>>>> hi Krzysztof,
>>>>>>>
>>>>>>> On 21 November 2015 at 09:56, Krzysztof Kozlowski <k.kozlowski@...sung.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@...il.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@...oidxu4:/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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists