| Message ID | 20160120143220.GA31427@ulmo (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Thierry, On 20 January 2016 at 20:02, Thierry Reding <thierry.reding@gmail.com> wrote: > 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 Thanks for this fix. Best Regards -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
--- 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 ---