[PATCHv2] pwm: avoid holding mutex in interrupt context

[PATCHv2] pwm: avoid holding mutex in interrupt context

[Anand Moon]

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@gmail.com: Fixed the commit message]
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
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);
 
 	if (pwm_is_enabled(pwm)) {
 		err = -EBUSY;
@@ -488,7 +488,7 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
 	pwm->polarity = polarity;
 
 unlock:
-	mutex_unlock(&pwm->lock);
+	spin_unlock_irq(&pwm->lock);
 	return err;
 }
 EXPORT_SYMBOL_GPL(pwm_set_polarity);
@@ -506,7 +506,7 @@ int pwm_enable(struct pwm_device *pwm)
 	if (!pwm)
 		return -EINVAL;
 
-	mutex_lock(&pwm->lock);
+	spin_lock_irq(&pwm->lock);
 
 	if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
 		err = pwm->chip->ops->enable(pwm->chip, pwm);
@@ -514,7 +514,7 @@ int pwm_enable(struct pwm_device *pwm)
 			clear_bit(PWMF_ENABLED, &pwm->flags);
 	}
 
-	mutex_unlock(&pwm->lock);
+	spin_unlock_irq(&pwm->lock);
 
 	return err;
 }
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index cfc3ed4..86ad4c2 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -2,7 +2,7 @@
 #define __LINUX_PWM_H
 
 #include <linux/err.h>
-#include <linux/mutex.h>
+#include <linux/spinlock.h>
 #include <linux/of.h>
 
 struct pwm_device;
@@ -100,7 +100,7 @@ struct pwm_device {
 	unsigned int pwm;
 	struct pwm_chip *chip;
 	void *chip_data;
-	struct mutex lock;
+	spinlock_t lock;
 
 	unsigned int period;
 	unsigned int duty_cycle;
-- 
2.1.4

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

[Krzysztof Kozlowski]

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@gmail.com: Fixed the commit message]
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> 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

>  
>  	if (pwm_is_enabled(pwm)) {
>  		err = -EBUSY;
> @@ -488,7 +488,7 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
>  	pwm->polarity = polarity;
>  
>  unlock:
> -	mutex_unlock(&pwm->lock);
> +	spin_unlock_irq(&pwm->lock);
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(pwm_set_polarity);
> @@ -506,7 +506,7 @@ int pwm_enable(struct pwm_device *pwm)
>  	if (!pwm)
>  		return -EINVAL;
>  
> -	mutex_lock(&pwm->lock);
> +	spin_lock_irq(&pwm->lock);
>  
>  	if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
>  		err = pwm->chip->ops->enable(pwm->chip, pwm);
> @@ -514,7 +514,7 @@ int pwm_enable(struct pwm_device *pwm)
>  			clear_bit(PWMF_ENABLED, &pwm->flags);
>  	}
>  
> -	mutex_unlock(&pwm->lock);
> +	spin_unlock_irq(&pwm->lock);
>  
>  	return err;
>  }
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index cfc3ed4..86ad4c2 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -2,7 +2,7 @@
>  #define __LINUX_PWM_H
>  
>  #include <linux/err.h>
> -#include <linux/mutex.h>
> +#include <linux/spinlock.h>
>  #include <linux/of.h>
>  
>  struct pwm_device;
> @@ -100,7 +100,7 @@ struct pwm_device {
>  	unsigned int pwm;
>  	struct pwm_chip *chip;
>  	void *chip_data;
> -	struct mutex lock;
> +	spinlock_t lock;
>  
>  	unsigned int period;
>  	unsigned int duty_cycle;
> 

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

[Anand Moon]

hi Krzysztof

On 18 January 2016 at 05:29, Krzysztof Kozlowski
<k.kozlowski@samsung.com> 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@gmail.com: Fixed the commit message]
>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>> ---
>> 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
>

Thanks for you input. I will look into this approach suggested by you.

Best regards,
-Anand Moon

>>
>>       if (pwm_is_enabled(pwm)) {
>>               err = -EBUSY;
>> @@ -488,7 +488,7 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
>>       pwm->polarity = polarity;
>>
>>  unlock:
>> -     mutex_unlock(&pwm->lock);
>> +     spin_unlock_irq(&pwm->lock);
>>       return err;
>>  }
>>  EXPORT_SYMBOL_GPL(pwm_set_polarity);
>> @@ -506,7 +506,7 @@ int pwm_enable(struct pwm_device *pwm)
>>       if (!pwm)
>>               return -EINVAL;
>>
>> -     mutex_lock(&pwm->lock);
>> +     spin_lock_irq(&pwm->lock);
>>
>>       if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
>>               err = pwm->chip->ops->enable(pwm->chip, pwm);
>> @@ -514,7 +514,7 @@ int pwm_enable(struct pwm_device *pwm)
>>                       clear_bit(PWMF_ENABLED, &pwm->flags);
>>       }
>>
>> -     mutex_unlock(&pwm->lock);
>> +     spin_unlock_irq(&pwm->lock);
>>
>>       return err;
>>  }
>> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
>> index cfc3ed4..86ad4c2 100644
>> --- a/include/linux/pwm.h
>> +++ b/include/linux/pwm.h
>> @@ -2,7 +2,7 @@
>>  #define __LINUX_PWM_H
>>
>>  #include <linux/err.h>
>> -#include <linux/mutex.h>
>> +#include <linux/spinlock.h>
>>  #include <linux/of.h>
>>
>>  struct pwm_device;
>> @@ -100,7 +100,7 @@ struct pwm_device {
>>       unsigned int pwm;
>>       struct pwm_chip *chip;
>>       void *chip_data;
>> -     struct mutex lock;
>> +     spinlock_t lock;
>>
>>       unsigned int period;
>>       unsigned int duty_cycle;
>>
>

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

[Anand Moon]

Hi Krzysztof,

On 18 January 2016 at 05:29, Krzysztof Kozlowski
<k.kozlowski@samsung.com> 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@gmail.com: Fixed the commit message]
>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>> ---
>> 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

>>
>>       if (pwm_is_enabled(pwm)) {
>>               err = -EBUSY;
>> @@ -488,7 +488,7 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
>>       pwm->polarity = polarity;
>>
>>  unlock:
>> -     mutex_unlock(&pwm->lock);
>> +     spin_unlock_irq(&pwm->lock);
>>       return err;
>>  }
>>  EXPORT_SYMBOL_GPL(pwm_set_polarity);
>> @@ -506,7 +506,7 @@ int pwm_enable(struct pwm_device *pwm)
>>       if (!pwm)
>>               return -EINVAL;
>>
>> -     mutex_lock(&pwm->lock);
>> +     spin_lock_irq(&pwm->lock);
>>
>>       if (!test_and_set_bit(PWMF_ENABLED, &pwm->flags)) {
>>               err = pwm->chip->ops->enable(pwm->chip, pwm);
>> @@ -514,7 +514,7 @@ int pwm_enable(struct pwm_device *pwm)
>>                       clear_bit(PWMF_ENABLED, &pwm->flags);
>>       }
>>
>> -     mutex_unlock(&pwm->lock);
>> +     spin_unlock_irq(&pwm->lock);
>>
>>       return err;
>>  }
>> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
>> index cfc3ed4..86ad4c2 100644
>> --- a/include/linux/pwm.h
>> +++ b/include/linux/pwm.h
>> @@ -2,7 +2,7 @@
>>  #define __LINUX_PWM_H
>>
>>  #include <linux/err.h>
>> -#include <linux/mutex.h>
>> +#include <linux/spinlock.h>
>>  #include <linux/of.h>
>>
>>  struct pwm_device;
>> @@ -100,7 +100,7 @@ struct pwm_device {
>>       unsigned int pwm;
>>       struct pwm_chip *chip;
>>       void *chip_data;
>> -     struct mutex lock;
>> +     spinlock_t lock;
>>
>>       unsigned int period;
>>       unsigned int duty_cycle;
>>
>

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@samsung.com> 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@gmail.com: Fixed the commit message]
>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>> ---
>>> 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

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

[Anand Moon]

Hi Krzysztof,

On 18 January 2016 at 09:58, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> On 18.01.2016 13:23, Anand Moon wrote:
>> Hi Krzysztof,
>>
>> On 18 January 2016 at 05:29, Krzysztof Kozlowski
>> <k.kozlowski@samsung.com> 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@gmail.com: Fixed the commit message]
>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>> ---
>>>> 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
>
>

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

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)
  \
   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

Please let me know if I am wrong.

-Anand Moon

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

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

[Anand Moon]

Hi Krzysztof,

On 20 January 2016 at 04:59, Krzysztof Kozlowski
<k.kozlowski@samsung.com> 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.
>
> Best regards,
> Krzysztof

Yep the flow might be wrong.
Ok thanks for your input.

Best Regards.
-Anand Moon

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5131 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Anand Moon]

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