From patchwork Fri Dec 11 04:07:48 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Anand Moon X-Patchwork-Id: 7824211 Return-Path: X-Original-To: patchwork-linux-samsung-soc@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id D8A5F9F349 for ; Fri, 11 Dec 2015 04:08:15 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 75FDF20570 for ; Fri, 11 Dec 2015 04:08:14 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 059BF20547 for ; Fri, 11 Dec 2015 04:08:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752889AbbLKEIM (ORCPT ); Thu, 10 Dec 2015 23:08:12 -0500 Received: from mail-wm0-f54.google.com ([74.125.82.54]:34636 "EHLO mail-wm0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751498AbbLKEIK (ORCPT ); Thu, 10 Dec 2015 23:08:10 -0500 Received: by mail-wm0-f54.google.com with SMTP id v187so60624770wmv.1; Thu, 10 Dec 2015 20:08:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type; bh=wQzrE1AaW72SfpvcEe8yLFtjFyC9/InMuVCalD4tQg8=; b=KdT6UeCFOYzI6LKKaR7FU6ia1Dvpm2CBKBiDKKfqJJRka0DXOjM1h9muBJBOB5nyTj PZyMi3OqMU7dXyeDLvRqHMHwAPCY4gZbj+ZNRsZ5yXLuORdG5II408uPVXBs8pi6cKI+ HHRqdpqco1yh4lbUY84i6Pvr/NmZwyIAB6ucKvkkFPD9OFl7StSH5Hr9Ubv9QNlydMI6 n02aDxznUi9cbfI3HdhMIepUk8qsGxz7FYFfUND10Txt4aE8iTmSn9IfRLqcJ9iRzmG5 JavOuKT8vjSDXsxftulZMAmG64pMC/7sxXcPRS3YKotoNnyts0TuhMWNe+YSffA0GIfB JFAg== X-Received: by 10.194.104.5 with SMTP id ga5mr17077811wjb.155.1449806888308; Thu, 10 Dec 2015 20:08:08 -0800 (PST) MIME-Version: 1.0 Received: by 10.28.211.208 with HTTP; Thu, 10 Dec 2015 20:07:48 -0800 (PST) In-Reply-To: References: <1448038740-2811-1-git-send-email-linux.amoon@gmail.com> <564FF287.8000201@samsung.com> From: Anand Moon Date: Fri, 11 Dec 2015 09:37:48 +0530 Message-ID: Subject: Re: [PATCH] pwm: Avoid double mutex lock on pwm_enable To: Krzysztof Kozlowski Cc: Guenter Roeck , Thierry Reding , Javier Martinez Canillas , linux-pwm@vger.kernel.org, Linux Kernel , "linux-samsung-soc@vger.kernel.org" , Jonathan Richardson Sender: linux-samsung-soc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-samsung-soc@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, T_DKIM_INVALID, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Krzysztof, On 22 November 2015 at 05:43, Krzysztof Kozlowski wrote: > 2015-11-22 3:14 GMT+09:00 Anand Moon : >> Hi Krzysztof, >> >> On 21 November 2015 at 18:37, Krzysztof Kozlowski >> wrote: >>> 2015-11-21 21:11 GMT+09:00 Anand Moon : >>>> hi Krzysztof, >>>> >>>> On 21 November 2015 at 15:22, Krzysztof Kozlowski >>>> wrote: >>>>> 2015-11-21 18:40 GMT+09:00 Anand Moon : >>>>>> hi Krzysztof, >>>>>> >>>>>> On 21 November 2015 at 09:56, Krzysztof Kozlowski >>>>>> 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] [] (unwind_backtrace) from [] >>>>>>> (show_stack+0x10/0x14) >>>>>>> [ 16.093938] [] (show_stack) from [] >>>>>>> (dump_stack+0x70/0xbc) >>>>>>> [ 16.101122] [] (dump_stack) from [] >>>>>>> (mutex_lock_nested+0x24/0x474) >>>>>>> [ 16.109009] [] (mutex_lock_nested) from [] >>>>>>> (pwm_enable+0x20/0x7c) >>>>>>> [ 16.116799] [] (pwm_enable) from [] >>>>>>> (led_heartbeat_function+0xdc/0x140) >>>>>>> [ 16.125119] [] (led_heartbeat_function) from [] >>>>>>> (call_timer_fn+0x6c/0xf4) >>>>>>> [ 16.133606] [] (call_timer_fn) from [] >>>>>>> (run_timer_softirq+0x1a8/0x230) >>>>>>> [ 16.141846] [] (run_timer_softirq) from [] >>>>>>> (__do_softirq+0x134/0x2c0) >>>>>>> [ 16.149982] [] (__do_softirq) from [] >>>>>>> (irq_exit+0xd0/0x114) >>>>>>> [ 16.157255] [] (irq_exit) from [] >>>>>>> (__handle_domain_irq+0x70/0xe4) >>>>>>> [ 16.165056] [] (__handle_domain_irq) from [] >>>>>>> (gic_handle_irq+0x4c/0x94) >>>>>>> [ 16.173376] [] (gic_handle_irq) from [] >>>>>>> (__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] [] (__irq_svc) from [] >>>>>>> (arch_cpu_idle+0x20/0x3c) >>>>>>> [ 16.217661] [] (arch_cpu_idle) from [] >>>>>>> (cpu_startup_entry+0x17c/0x26c) >>>>>>> [ 16.225899] [] (cpu_startup_entry) from [] >>>>>>> (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] [] (unwind_backtrace) from [] >>>>>>> > (show_stack+0x10/0x14) >>>>>>> > [ 2.701808] [] (show_stack) from [] >>>>>>> > (dump_stack+0x84/0xc4) >>>>>>> > [ 2.701824] [] (dump_stack) from [] >>>>>>> > (warn_slowpath_common+0x80/0xb0) >>>>>>> > [ 2.701836] [] (warn_slowpath_common) from [] >>>>>>> > (warn_slowpath_fmt+0x30/0x40) >>>>>>> > [ 2.701849] [] (warn_slowpath_fmt) from [] >>>>>>> > (__mutex_lock_slowpath+0x3bc/0x404) >>>>>>> > [ 2.701864] [] (__mutex_lock_slowpath) from [] >>>>>>> > (mutex_lock+0xc/0x24) >>>>>>> > [ 2.701884] [] (mutex_lock) from [] >>>>>>> > (pwm_enable+0x20/0x7c) >>>>>>> > [ 2.701903] [] (pwm_enable) from [] >>>>>>> > (led_heartbeat_function+0x74/0x144) >>>>>>> > [ 2.701919] [] (led_heartbeat_function) from [] >>>>>>> > (call_timer_fn+0x24/0x98) >>>>>>> > [ 2.701932] [] (call_timer_fn) from [] >>>>>>> > (run_timer_softirq+0x160/0x21c) >>>>>>> > [ 2.701946] [] (run_timer_softirq) from [] >>>>>>> > (__do_softirq+0x110/0x228) >>>>>>> > [ 2.701959] [] (__do_softirq) from [] >>>>>>> > (irq_exit+0xc0/0xfc) >>>>>>> > [ 2.701976] [] (irq_exit) from [] >>>>>>> > (__handle_domain_irq+0x80/0xec) >>>>>>> > [ 2.701990] [] (__handle_domain_irq) from [] >>>>>>> > (gic_handle_irq+0x54/0x94) >>>>>>> > [ 2.702001] [] (gic_handle_irq) from [] >>>>>>> > (__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] [] (__irq_svc) from [] >>>>>>> > (arch_cpu_idle+0x38/0x3c) >>>>>>> > [ 2.702073] [] (arch_cpu_idle) from [] >>>>>>> > (cpu_startup_entry+0x1ec/0x270) >>>>>>> > [ 2.702086] [] (cpu_startup_entry) from [<4000956c>] >>>>>>> > (0x4000956c) >>>>>>> > [ 2.702093] ---[ end trace 539af70491f4f1a9 ]--- >>>>>>> > >>>>>>> > Signed-off-by: Anand Moon >>>>>>> > --- >>>>>>> > 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 -Anand Moon --- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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)) {