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

[Krzysztof Kozlowski]

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.

Best regards,
Krzysztof