From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753764AbcARE2p (ORCPT ); Sun, 17 Jan 2016 23:28:45 -0500 Received: from mailout3.w1.samsung.com ([210.118.77.13]:10414 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753097AbcARE2n (ORCPT ); Sun, 17 Jan 2016 23:28:43 -0500 X-AuditID: cbfec7f4-f79026d00000418a-fa-569c69f79223 Subject: Re: [PATCHv2] pwm: avoid holding mutex in interrupt context To: Anand Moon References: <1453064517-12315-1-git-send-email-linux.amoon@gmail.com> <569C2AE8.6070100@samsung.com> Cc: Guenter Roeck , Thierry Reding , Javier Martinez Canillas , linux-pwm@vger.kernel.org, Linux Kernel , "linux-samsung-soc@vger.kernel.org" From: Krzysztof Kozlowski Message-id: <569C69F4.7000906@samsung.com> Date: Mon, 18 Jan 2016 13:28:36 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-version: 1.0 In-reply-to: Content-type: text/plain; charset=utf-8 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrPLMWRmVeSWpSXmKPExsVy+t/xq7rfM+eEGfQ/1LZ483YNk8XrF4YW l3fNYbO4e3cVo8WM8/uYLNZtvMVu8WThGSaLn7vmsThweOycdZfdY0s/kNj5vYHdo2/LKkaP z5vkAlijuGxSUnMyy1KL9O0SuDK+9P5hKmhWqFi1bgtrA+N5yS5GTg4JAROJnXfesUHYYhIX 7q0Hsrk4hASWMkqsWTMVynnKKDFj13amLkYODmEBF4n7GxNAGkQE1CSuPF3BClGzllFi3e89 TCAOs8BcJolZbZuYQarYBIwlNi9fAraCV0BLYs+8e4wgNouAqsTcZ3uZQGxRgQiJw51d7BA1 ghI/Jt9jAbE5BYIlZnV+ZgVZzCygLjFlSi5ImFlAXmLzmrfMExgFZiHpmIVQNQtJ1QJG5lWM oqmlyQXFSem5hnrFibnFpXnpesn5uZsYIcH+ZQfj4mNWhxgFOBiVeHhnnJ8dJsSaWFZcmXuI UYKDWUmEVzplTpgQb0piZVVqUX58UWlOavEhRmkOFiVx3rm73ocICaQnlqRmp6YWpBbBZJk4 OKUaGNUmXlnA+OyVRIVKarGQ1I5zcvxiB4o/LY5T2uBv49OVuY7HWu/6u87yQh2RHRLX494x xZ5YZraiSaGjTDf683r2Xb9+npCYs5XdRTrf3rJK5aTPrf66b5Zrpvgbh8Ql6hc/7pznIXJk 5WWpV8ULLfj3usqb/TeQdwhM7WWdIluZ4zPJ3bBdiaU4I9FQi7moOBEAntIOj3ICAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 18.01.2016 13:23, Anand Moon wrote: > Hi Krzysztof, > > On 18 January 2016 at 05:29, Krzysztof Kozlowski > wrote: >> On 18.01.2016 06:01, Anand Moon wrote: >>> The introduction of the mutex in commit 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. >>> >>> Changes fix the below bug by using spinlocks instead of mutex >>> >>> [ 22.300239] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97 >>> [ 22.307212] in_atomic(): 1, irqs_disabled(): 0, pid: 2257, name: sh >>> [ 22.313454] Preemption disabled at:[< (null)>] (null) >>> [ 23.655232] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97 >>> [ 23.662174] in_atomic(): 1, irqs_disabled(): 0, pid: 2404, name: upowerd >>> [ 23.668932] Preemption disabled at:[< (null)>] (null) >>> [ 25.010207] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97 >>> [ 25.017125] in_atomic(): 1, irqs_disabled(): 0, pid: 2262, name: indicator-keybo >>> [ 25.024491] Preemption disabled at:[< (null)>] (null) >>> [ 26.355237] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97 >>> [ 26.362141] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0 >>> [ 26.368728] Preemption disabled at:[< (null)>] (null) >>> [ 27.680220] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97 >>> [ 27.687119] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0 >>> [ 27.693698] Preemption disabled at:[< (null)>] (null) >>> [ 29.005199] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97 >>> [ 29.012124] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0 >>> >>> [thierry.reding@gmail.com: Fixed the commit message] >>> Signed-off-by: Anand Moon >>> --- >>> Changes logs: droped my prevoius approch. >>> --- >>> drivers/pwm/core.c | 10 +++++----- >>> include/linux/pwm.h | 4 ++-- >>> 2 files changed, 7 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c >>> index d24ca5f..58e7091 100644 >>> --- a/drivers/pwm/core.c >>> +++ b/drivers/pwm/core.c >>> @@ -269,7 +269,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip, >>> pwm->pwm = chip->base + i; >>> pwm->hwpwm = i; >>> pwm->polarity = polarity; >>> - mutex_init(&pwm->lock); >>> + spin_lock_init(&pwm->lock); >>> >>> radix_tree_insert(&pwm_tree, pwm->pwm, pwm); >>> } >>> @@ -474,7 +474,7 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity) >>> if (!pwm->chip->ops->set_polarity) >>> return -ENOSYS; >>> >>> - mutex_lock(&pwm->lock); >>> + spin_lock_irq(&pwm->lock); >> >> Anand, >> >> Thank you for the effort put into digging into this issue. Unfortunately >> this approach is bad. You cannot fix one issue without looking at the >> big picture of the given subsystem. This patch does exactly this - fixes >> your warning but probably introduces bugs all over the place. >> >> Although the set_polarity callback (called under the lock) is not >> described as sleeping-allowed but some implementations do it in a >> sleeping way. This is really easy to find, e.g.: >> pwm_omap_dmtimer_set_polarity. >> >> This means: no. >> >> Best regards, >> Krzysztof >> > > Already within function pwm_samsung_set_invert is protected by > spin_lock_irqsave(&samsung_pwm_lock, flags); > > So no need to introduce another lock to control pwm_samsung_set_polarity. > > Best Regards. > -Anand Moon I don't have any clue what is your point here. I don't get what pwm_samsung_set_polarity has to do with main pwm core... Sorry, you need to be more specific. Best regards, Krzysztof