From patchwork Wed Jan 20 14:32:20 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Thierry Reding X-Patchwork-Id: 8072361 Return-Path: X-Original-To: patchwork-linux-samsung-soc@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id CEFF4BEEE5 for ; Wed, 20 Jan 2016 14:32:29 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id AD239204AF for ; Wed, 20 Jan 2016 14:32:28 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 9E110202E6 for ; Wed, 20 Jan 2016 14:32:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933818AbcATOcZ (ORCPT ); Wed, 20 Jan 2016 09:32:25 -0500 Received: from mail-wm0-f54.google.com ([74.125.82.54]:38174 "EHLO mail-wm0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932744AbcATOcY (ORCPT ); Wed, 20 Jan 2016 09:32:24 -0500 Received: by mail-wm0-f54.google.com with SMTP id b14so32348459wmb.1; Wed, 20 Jan 2016 06:32:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=Lul7rLd8Xs0lDZlg+aHMILZh6ji/cqH4ibCl7It4OFE=; b=FMw8zS85L2vvRJ9J2K7wrYkihmJg56HM8tWlgOx9+oFMYNrOARonTA77TnB/K5vF1X V0zdKH99JEPfoeRFo2fNdJ6bVzgrR1RTm/FmQJMqC6jwnQU4206PJ+m/CdTodJ1O1zg2 3Pj1AW3EIpN8NjJoeWtkCaiJuXw69XtG4C30q3NH2+gr8Gwl9Dt6YzgTGIeYDElBCa7x riUS8UwcxIEYCPYnpZ+jVdY0onZf1KBl5vUaOxfYK4cJ88V4Ld+s8hETwH2JObPdX/+I IRKBvGhXF7RrOwQWHmz3uoeJ+kdUcgP6fx+ALNSjG2XjT4HWm8S3iS3VB0EJuqbM1Y2g Na3Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=Lul7rLd8Xs0lDZlg+aHMILZh6ji/cqH4ibCl7It4OFE=; b=H0kWF1L14BMV6m7bWb9fHRebqTz244jfc8FdEpTTy2cc2aEkVJJCAqVg6Yh4E1Sb/T gOP+bfC6TjLLXJGhYuRcMolN5jiNpsWG/IC0KKvkrNkqKqyH5jaxJRPH7/b1SNwNA0vA gtDqik5y9pjS6LFnBUW9Am9BMIGLWYHnWZw6oxMDbXAKfZZg1acbmExn5KpYPgMBGURq +m4LFW3W+08wNpMCW99w73PLIQsZyr11umzDnGwgDFHB58HZuLKx96TBAI/3eD4+C6Sj 45rslTftdoEWTGofWvagZhUbE65cYiR/CqHdcC3kkcN2v6Izng97Hu5sPMu5tda6gmro 0r8g== X-Gm-Message-State: ALoCoQniwXDTLO5j3yK1ELCTKhOO6vkRZzyd0oQQ55DJC8xs8oXN22hxz2MNwNcPMpV6HXOvs5ePiRIIPvifi+FnVU4AKNEDLA== X-Received: by 10.194.60.231 with SMTP id k7mr36050899wjr.61.1453300342369; Wed, 20 Jan 2016 06:32:22 -0800 (PST) Received: from localhost (port-9824.pppoe.wtnet.de. [84.46.38.134]) by smtp.gmail.com with ESMTPSA id z65sm25431799wmg.10.2016.01.20.06.32.21 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 20 Jan 2016 06:32:21 -0800 (PST) Date: Wed, 20 Jan 2016 15:32:20 +0100 From: Thierry Reding To: Krzysztof Kozlowski Cc: Anand Moon , Guenter Roeck , Javier Martinez Canillas , linux-pwm@vger.kernel.org, Linux Kernel , "linux-samsung-soc@vger.kernel.org" Subject: Re: [PATCHv2] pwm: avoid holding mutex in interrupt context Message-ID: <20160120143220.GA31427@ulmo> References: <1453064517-12315-1-git-send-email-linux.amoon@gmail.com> <569C2AE8.6070100@samsung.com> <569C69F4.7000906@samsung.com> <569EC6F6.1020505@samsung.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <569EC6F6.1020505@samsung.com> User-Agent: Mutt/1.5.24 (2015-08-30) 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, RP_MATCHES_RCVD, T_DKIM_INVALID, T_TVD_MIME_EPI,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 On Wed, Jan 20, 2016 at 08:29:58AM +0900, Krzysztof Kozlowski wrote: > On 20.01.2016 00:04, Anand Moon wrote: > > Hi Krzysztof, > > > > On 18 January 2016 at 09:58, Krzysztof Kozlowski > >>> 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 > >> > >> > > > > Below is the mapping of calls from pwm driver. > > I have tried to map the functionality and I am trying to understand > > the flow of the driver. > > > > Also looking in document > > > > https://www.kernel.org/doc/Documentation/pwm.txt > > > > pwm-samsung driver controls the LEDS, fans...etc > > > > Form the dts modes pwmleds > > > > pwmleds { > > compatible = "pwm-leds"; > > > > blueled { > > label = "blue:heartbeat"; > > pwms = <&pwm 2 2000000 0>; > > pwm-names = "pwm2"; > > max_brightness = <255>; > > linux,default-trigger = "heartbeat"; > > }; > > }; > > > > Following is the map out from the device tree. > > > > pwms = <&pwm 2 2000000 0>; > > > > &pwm -> pwm: pwm@12dd0000 --->samsung,exynos4210-pwm > > 2 -> period > > 2000000 -> duty_cycle > > 0 -> polarity > > I do not see any relations between DTS and the problem. > > > > > And here is the mapping of the call of function > > Note: This function call are as per my understanding of the flow in > > the driver. I might be wrong. > > > > pwm_samsung_set_polarity(struct pwm_chip *chip, struct pwm_device > > *pwm, enum pwm_polarity polarity) > > \ > > pwm_samsung_set_invert(our_chip, pwm->hwpwm, invert); > > \ > > pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity) > > No, pwm_samsung_set_invert does not call pwm_set_polarity(). This would > result in a circular call - back to pwm_samsung_set_polarity(). > > > \ > > pwm->chip->ops->set_polarity(pwm->chip, pwm, polarity); > > \ > > pwm_enable(struct pwm_device *pwm) or pwm_disable(struct pwm_device *pwm) > > > > pwm_enable or pwm_disable will be triggered on change in pwm->flags by > > the pwm core. > > before pwm_set_polarity is called form the Samsung driver it hold with > > following locks > > > > Here is the locking > > > > pwm_samsung_set_polarity(struct pwm_chip *chip, struct pwm_device > > *pwm, enum pwm_polarity polarity) > > \ > > pwm_samsung_set_invert(struct samsung_pwm_chip *chip, unsigned int > > channel, bool invert) > > \ > > spin_lock_irqsave(&samsung_pwm_lock, flags); > > \ > > pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity) > > \ > > mutex_lock(&pwm->lock) > > > > pwm_enable(struct pwm_device *pwm) or pwm_disable(struct > > pwm_device *pwm) > > \ > > mutex_lock(&pwm->lock); > > > > Problem I see that we are holding the lock in interrupt context. > > I don't know how the this triggers this bug. > > > > BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97 > > So leave it. If your flow of calls was correct, you would spot the > problem. But actually it does not matter - I think the flow is not correct. The reason for the BUG that you're seeing is that the leds-pwm driver differentiates between PWMs that can sleep and those that can't. This used to be limited to some PWMs that were attached to a slow bus like I2C, or that called functions which might sleep (like clk_prepare()). With commit d1cd21427747 ("pwm: Set enable state properly on failed call to enable"), effectively all PWM drivers may sleep. The lock introduced in that commit must also be a mutex because it protects sections which may sleep themselves (->enable() and ->set_polarity()) so turning it into a spinlock won't work for the general case. Given that this is currently broken and we're quite close to -rc1 I suggest the following fix for now: --- >8 --- diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index d24ca5f281b4..7831bc6b51dd 100644 For v4.6 I can remove all usage of the ->can_sleep and pwm_can_sleep() because they're effectively useless now. Does that sound reasonable to everyone? Anand, the above should fix the issue for you. Can you give it a try and report if it doesn't? Thanks, Thierry --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -889,7 +889,7 @@ EXPORT_SYMBOL_GPL(devm_pwm_put); */ bool pwm_can_sleep(struct pwm_device *pwm) { - return pwm->chip->can_sleep; + return true; } EXPORT_SYMBOL_GPL(pwm_can_sleep); --- >8 ---