Re: [PATCHv2] pwm: avoid holding mutex in interrupt context

[Anand Moon]

Hi Thierry,

On 20 January 2016 at 20:02, Thierry Reding  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(_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 = < 2 200 0>;
>> > pwm-names = "pwm2";
>> > max_brightness = <255>;
>> > linux,default-trigger = "heartbeat";
>> > };
>> > };
>> >
>> > Following is the map out from the device tree.
>> >
>> > pwms = < 2 200 0>;
>> >
>> >->  pwm: pwm@12dd --->samsung,exynos4210-pwm
>> > 2  ->  period
>> > 200->  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(_pwm_lock, flags);
>> >   \
>> >pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
>> >\
>> > mutex_lock(>lock)
>> >
>> >   pwm_enable(struct pwm_device *pwm) or pwm_disable(struct
>> > pwm_device *pwm)
>> >   \
>> >mutex_lock(>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 

Re: [PATCHv2] pwm: avoid holding mutex in interrupt context

[Anand Moon]

Hi Thierry,

On 20 January 2016 at 20:02, Thierry Reding  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(_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 = < 2 200 0>;
>> > pwm-names = "pwm2";
>> > max_brightness = <255>;
>> > linux,default-trigger = "heartbeat";
>> > };
>> > };
>> >
>> > Following is the map out from the device tree.
>> >
>> > pwms = < 2 200 0>;
>> >
>> >->  pwm: pwm@12dd --->samsung,exynos4210-pwm
>> > 2  ->  period
>> > 200->  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(_pwm_lock, flags);
>> >   \
>> >pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
>> >\
>> > mutex_lock(>lock)
>> >
>> >   pwm_enable(struct pwm_device *pwm) or pwm_disable(struct
>> > pwm_device *pwm)
>> >   \
>> >mutex_lock(>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