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

[Krzysztof Kozlowski]

On 18.01.2016 13:23, Anand Moon wrote:
> Hi Krzysztof,
> 
> On 18 January 2016 at 05:29, Krzysztof Kozlowski
> <k.kozlowski@xxxxxxxxxxx> wrote:
>> On 18.01.2016 06:01, Anand Moon wrote:
>>> The introduction of the mutex in commit d1cd21427747 ("pwm: Set enable
>>> state properly on failed call to enable") effectively makes all PWM drivers
>>> potentially sleeping. That in turn makes the .can_sleep field obsolete
>>> since all drivers can now sleep.
>>>
>>> Changes fix the below bug by using spinlocks instead of mutex
>>>
>>> [   22.300239] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97
>>> [   22.307212] in_atomic(): 1, irqs_disabled(): 0, pid: 2257, name: sh
>>> [   22.313454] Preemption disabled at:[<  (null)>]   (null)
>>> [   23.655232] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97
>>> [   23.662174] in_atomic(): 1, irqs_disabled(): 0, pid: 2404, name: upowerd
>>> [   23.668932] Preemption disabled at:[<  (null)>]   (null)
>>> [   25.010207] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97
>>> [   25.017125] in_atomic(): 1, irqs_disabled(): 0, pid: 2262, name: indicator-keybo
>>> [   25.024491] Preemption disabled at:[<  (null)>]   (null)
>>> [   26.355237] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97
>>> [   26.362141] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
>>> [   26.368728] Preemption disabled at:[<  (null)>]   (null)
>>> [   27.680220] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97
>>> [   27.687119] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
>>> [   27.693698] Preemption disabled at:[<  (null)>]   (null)
>>> [   29.005199] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97
>>> [   29.012124] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
>>>
>>> [thierry.reding@xxxxxxxxx: Fixed the commit message]
>>> Signed-off-by: Anand Moon <linux.amoon@xxxxxxxxx>
>>> ---
>>> Changes logs: droped my prevoius approch.
>>> ---
>>>  drivers/pwm/core.c  | 10 +++++-----
>>>  include/linux/pwm.h |  4 ++--
>>>  2 files changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>>> index d24ca5f..58e7091 100644
>>> --- a/drivers/pwm/core.c
>>> +++ b/drivers/pwm/core.c
>>> @@ -269,7 +269,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>>>               pwm->pwm = chip->base + i;
>>>               pwm->hwpwm = i;
>>>               pwm->polarity = polarity;
>>> -             mutex_init(&pwm->lock);
>>> +             spin_lock_init(&pwm->lock);
>>>
>>>               radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
>>>       }
>>> @@ -474,7 +474,7 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
>>>       if (!pwm->chip->ops->set_polarity)
>>>               return -ENOSYS;
>>>
>>> -     mutex_lock(&pwm->lock);
>>> +     spin_lock_irq(&pwm->lock);
>>
>> Anand,
>>
>> Thank you for the effort put into digging into this issue. Unfortunately
>> this approach is bad. You cannot fix one issue without looking at the
>> big picture of the given subsystem. This patch does exactly this - fixes
>> your warning but probably introduces bugs all over the place.
>>
>> Although the set_polarity callback (called under the lock) is not
>> described as sleeping-allowed but some implementations do it in a
>> sleeping way. This is really easy to find, e.g.:
>> pwm_omap_dmtimer_set_polarity.
>>
>> This means: no.
>>
>> Best regards,
>> Krzysztof
>>
> 
> 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