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 --- 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 --- 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
Attachment:
signature.asc
Description: PGP signature
