lkml.org 
[lkml]   [2016]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCHv2] pwm: avoid holding mutex in interrupt context
From
Date
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

\
 
 \ /
  Last update: 2016-01-20 01:01    [W:0.106 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site