[PATCH] Input: pwm-beeper - defer pwm config if pwm can sleep

[PATCH] Input: pwm-beeper - defer pwm config if pwm can sleep

[Manfred Schlaegl]

If the pwm can sleep defer actions to it using a worker.
A similar approach was used in leds-pwm (c971ff185)

Trigger:
On a Freescale i.MX53 based board we ran into "BUG: scheduling while
atomic" because input_inject_event locks interrupts, but
imx_pwm_config_v2 sleeps.

Tested on Freescale i.MX53 SoC with 4.5-rc1 and 4.1.

Unmodified applicable to
 * 4.5-rc4
 * 4.4.1 (stable)
 * 4.3.5 (stable)
 * 4.1.18 (longterm)

Modified applicable to
 * 3.18.27 (longterm)

Signed-off-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>
---
 drivers/input/misc/pwm-beeper.c | 62 +++++++++++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 18 deletions(-)

diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index f2261ab..c160b5e 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -20,21 +20,42 @@
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 #include <linux/slab.h>
+#include <linux/workqueue.h>
 
 struct pwm_beeper {
 	struct input_dev *input;
 	struct pwm_device *pwm;
+	struct work_struct work;
 	unsigned long period;
+	bool can_sleep;
 };
 
 #define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
 
+static void __pwm_beeper_set(struct pwm_beeper *beeper)
+{
+	unsigned long period = beeper->period;
+
+	pwm_config(beeper->pwm, period / 2, period);
+
+	if (period == 0)
+		pwm_disable(beeper->pwm);
+	else
+		pwm_enable(beeper->pwm);
+}
+
+static void pwm_beeper_work(struct work_struct *work)
+{
+	struct pwm_beeper *beeper =
+		container_of(work, struct pwm_beeper, work);
+
+	__pwm_beeper_set(beeper);
+}
+
 static int pwm_beeper_event(struct input_dev *input,
 			    unsigned int type, unsigned int code, int value)
 {
-	int ret = 0;
 	struct pwm_beeper *beeper = input_get_drvdata(input);
-	unsigned long period;
 
 	if (type != EV_SND || value < 0)
 		return -EINVAL;
@@ -49,18 +70,15 @@ static int pwm_beeper_event(struct input_dev *input,
 		return -EINVAL;
 	}
 
-	if (value == 0) {
-		pwm_disable(beeper->pwm);
-	} else {
-		period = HZ_TO_NANOSECONDS(value);
-		ret = pwm_config(beeper->pwm, period / 2, period);
-		if (ret)
-			return ret;
-		ret = pwm_enable(beeper->pwm);
-		if (ret)
-			return ret;
-		beeper->period = period;
-	}
+	if (value == 0)
+		beeper->period = 0;
+	else
+		beeper->period = HZ_TO_NANOSECONDS(value);
+
+	if (beeper->can_sleep)
+		schedule_work(&beeper->work);
+	else
+		__pwm_beeper_set(beeper);
 
 	return 0;
 }
@@ -87,6 +105,10 @@ static int pwm_beeper_probe(struct platform_device *pdev)
 		goto err_free;
 	}
 
+	beeper->can_sleep = pwm_can_sleep(beeper->pwm);
+	if (beeper->can_sleep)
+		INIT_WORK(&beeper->work, pwm_beeper_work);
+
 	beeper->input = input_allocate_device();
 	if (!beeper->input) {
 		dev_err(&pdev->dev, "Failed to allocate input device\n");
@@ -133,6 +155,9 @@ static int pwm_beeper_remove(struct platform_device *pdev)
 {
 	struct pwm_beeper *beeper = platform_get_drvdata(pdev);
 
+	if (beeper->can_sleep)
+		cancel_work_sync(&beeper->work);
+
 	input_unregister_device(beeper->input);
 
 	pwm_disable(beeper->pwm);
@@ -147,6 +172,9 @@ static int __maybe_unused pwm_beeper_suspend(struct device *dev)
 {
 	struct pwm_beeper *beeper = dev_get_drvdata(dev);
 
+	if (beeper->can_sleep)
+		cancel_work_sync(&beeper->work);
+
 	if (beeper->period)
 		pwm_disable(beeper->pwm);
 
@@ -157,10 +185,8 @@ static int __maybe_unused pwm_beeper_resume(struct device *dev)
 {
 	struct pwm_beeper *beeper = dev_get_drvdata(dev);
 
-	if (beeper->period) {
-		pwm_config(beeper->pwm, beeper->period / 2, beeper->period);
-		pwm_enable(beeper->pwm);
-	}
+	if (beeper->period)
+		__pwm_beeper_set(beeper);
 
 	return 0;
 }
-- 
2.1.4

Re: [PATCH] Input: pwm-beeper - defer pwm config if pwm can sleep

[Dmitry Torokhov]

On Wed, Feb 17, 2016 at 02:19:26PM +0100, Manfred Schlaegl wrote:
> If the pwm can sleep defer actions to it using a worker.
> A similar approach was used in leds-pwm (c971ff185)
> 
> Trigger:
> On a Freescale i.MX53 based board we ran into "BUG: scheduling while
> atomic" because input_inject_event locks interrupts, but
> imx_pwm_config_v2 sleeps.
> 
> Tested on Freescale i.MX53 SoC with 4.5-rc1 and 4.1.
> 
> Unmodified applicable to
>  * 4.5-rc4
>  * 4.4.1 (stable)
>  * 4.3.5 (stable)
>  * 4.1.18 (longterm)
> 
> Modified applicable to
>  * 3.18.27 (longterm)
> 
> Signed-off-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>
> ---
>  drivers/input/misc/pwm-beeper.c | 62 +++++++++++++++++++++++++++++------------
>  1 file changed, 44 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
> index f2261ab..c160b5e 100644
> --- a/drivers/input/misc/pwm-beeper.c
> +++ b/drivers/input/misc/pwm-beeper.c
> @@ -20,21 +20,42 @@
>  #include <linux/platform_device.h>
>  #include <linux/pwm.h>
>  #include <linux/slab.h>
> +#include <linux/workqueue.h>
>  
>  struct pwm_beeper {
>  	struct input_dev *input;
>  	struct pwm_device *pwm;
> +	struct work_struct work;
>  	unsigned long period;
> +	bool can_sleep;

I wonder if it is not better to always schedule work, regardless of
whether PWM may sleep or not.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: pwm-beeper - defer pwm config if pwm can sleep

[Manfred Schlaegl]

On 2016-02-22 20:46, Dmitry Torokhov wrote:
> On Wed, Feb 17, 2016 at 02:19:26PM +0100, Manfred Schlaegl wrote:
>> If the pwm can sleep defer actions to it using a worker.
>> A similar approach was used in leds-pwm (c971ff185)
>>
>> Trigger:
>> On a Freescale i.MX53 based board we ran into "BUG: scheduling while
>> atomic" because input_inject_event locks interrupts, but
>> imx_pwm_config_v2 sleeps.
>>
>> Tested on Freescale i.MX53 SoC with 4.5-rc1 and 4.1.
>>
>> Unmodified applicable to
>>  * 4.5-rc4
>>  * 4.4.1 (stable)
>>  * 4.3.5 (stable)
>>  * 4.1.18 (longterm)
>>
>> Modified applicable to
>>  * 3.18.27 (longterm)
>>
>> Signed-off-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>
>> ---
>>  drivers/input/misc/pwm-beeper.c | 62 +++++++++++++++++++++++++++++------------
>>  1 file changed, 44 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
>> index f2261ab..c160b5e 100644
>> --- a/drivers/input/misc/pwm-beeper.c
>> +++ b/drivers/input/misc/pwm-beeper.c
>> @@ -20,21 +20,42 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/pwm.h>
>>  #include <linux/slab.h>
>> +#include <linux/workqueue.h>
>>  
>>  struct pwm_beeper {
>>  	struct input_dev *input;
>>  	struct pwm_device *pwm;
>> +	struct work_struct work;
>>  	unsigned long period;
>> +	bool can_sleep;
> 
> I wonder if it is not better to always schedule work, regardless of
> whether PWM may sleep or not.
> 
> Thanks.
> 

In my opinion there is no real strong argument to do it this or that way.
I decided to do it this way because of following weaker arguments:
 1. If pwm can not sleep the behavior stays exactly the same as before
 2. The introduced conditions do not really add much complexity to the code
 3. It was successfully done the same way in leds-pwm

Best regards,
Manfred

Re: [PATCH] Input: pwm-beeper - defer pwm config if pwm can sleep

[Manfred Schlaegl]

Hello,

Any more feedback on this?

Just wanted to remember - this patch is still pending for integration.

kindly regards,
manfred

Re: [PATCH] Input: pwm-beeper - defer pwm config if pwm can sleep

[Thierry Reding]

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

On Mon, Feb 22, 2016 at 11:46:39AM -0800, Dmitry Torokhov wrote:
> On Wed, Feb 17, 2016 at 02:19:26PM +0100, Manfred Schlaegl wrote:
> > If the pwm can sleep defer actions to it using a worker.
> > A similar approach was used in leds-pwm (c971ff185)
> > 
> > Trigger:
> > On a Freescale i.MX53 based board we ran into "BUG: scheduling while
> > atomic" because input_inject_event locks interrupts, but
> > imx_pwm_config_v2 sleeps.
> > 
> > Tested on Freescale i.MX53 SoC with 4.5-rc1 and 4.1.
> > 
> > Unmodified applicable to
> >  * 4.5-rc4
> >  * 4.4.1 (stable)
> >  * 4.3.5 (stable)
> >  * 4.1.18 (longterm)
> > 
> > Modified applicable to
> >  * 3.18.27 (longterm)
> > 
> > Signed-off-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>
> > ---
> >  drivers/input/misc/pwm-beeper.c | 62 +++++++++++++++++++++++++++++------------
> >  1 file changed, 44 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
> > index f2261ab..c160b5e 100644
> > --- a/drivers/input/misc/pwm-beeper.c
> > +++ b/drivers/input/misc/pwm-beeper.c
> > @@ -20,21 +20,42 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/pwm.h>
> >  #include <linux/slab.h>
> > +#include <linux/workqueue.h>
> >  
> >  struct pwm_beeper {
> >  	struct input_dev *input;
> >  	struct pwm_device *pwm;
> > +	struct work_struct work;
> >  	unsigned long period;
> > +	bool can_sleep;
> 
> I wonder if it is not better to always schedule work, regardless of
> whether PWM may sleep or not.

I agree with Dmitry. Users of the PWM API should always assume that
calls to the PWM API might sleep. Conditionalizing on pwm_can_sleep()
isn't a good idea, since that function is scheduled to be removed. In
fact it's been returning true unconditionally since v4.5, so the fast
path is dead code anyway.

Thierry

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

Re: [PATCH] Input: pwm-beeper - defer pwm config if pwm can sleep

[Manfred Schlaegl]

On 2016-05-12 14:18, Thierry Reding wrote:
> 
> I agree with Dmitry. Users of the PWM API should always assume that
> calls to the PWM API might sleep. Conditionalizing on pwm_can_sleep()
> isn't a good idea, since that function is scheduled to be removed. In
> fact it's been returning true unconditionally since v4.5, so the fast
> path is dead code anyway.
> 

In this case, the decision is clear ;-)
I'll rework and send the new patch in the next days.

best regards,
manfred

[PATCH] Input: pwm-beeper - fix: scheduling while atomic

[Manfred Schlaegl]

Pwm config may sleep so defer it using a worker.

Trigger:
On a Freescale i.MX53 based board we ran into "BUG: scheduling while
atomic" because input_inject_event locks interrupts, but
imx_pwm_config_v2 sleeps.

Tested on Freescale i.MX53 SoC with 4.6.0 and 4.1.24.

Unmodified applicable to
 * 4.6    (stable)
 * 4.5.4  (stable)
 * 4.4.10 (longterm)
 * 4.1.24 (longterm)

Modified applicable to
 * 3.18.33 (longterm)

Signed-off-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>
---
 drivers/input/misc/pwm-beeper.c | 54 +++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 18 deletions(-)

diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index f2261ab..7d783c8 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -20,21 +20,41 @@
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 #include <linux/slab.h>
+#include <linux/workqueue.h>
 
 struct pwm_beeper {
 	struct input_dev *input;
 	struct pwm_device *pwm;
+	struct work_struct work;
 	unsigned long period;
 };
 
 #define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
 
+static void __pwm_beeper_set(struct pwm_beeper *beeper)
+{
+	unsigned long period = beeper->period;
+
+	pwm_config(beeper->pwm, period / 2, period);
+
+	if (period == 0)
+		pwm_disable(beeper->pwm);
+	else
+		pwm_enable(beeper->pwm);
+}
+
+static void pwm_beeper_work(struct work_struct *work)
+{
+	struct pwm_beeper *beeper =
+		container_of(work, struct pwm_beeper, work);
+
+	__pwm_beeper_set(beeper);
+}
+
 static int pwm_beeper_event(struct input_dev *input,
 			    unsigned int type, unsigned int code, int value)
 {
-	int ret = 0;
 	struct pwm_beeper *beeper = input_get_drvdata(input);
-	unsigned long period;
 
 	if (type != EV_SND || value < 0)
 		return -EINVAL;
@@ -49,18 +69,12 @@ static int pwm_beeper_event(struct input_dev *input,
 		return -EINVAL;
 	}
 
-	if (value == 0) {
-		pwm_disable(beeper->pwm);
-	} else {
-		period = HZ_TO_NANOSECONDS(value);
-		ret = pwm_config(beeper->pwm, period / 2, period);
-		if (ret)
-			return ret;
-		ret = pwm_enable(beeper->pwm);
-		if (ret)
-			return ret;
-		beeper->period = period;
-	}
+	if (value == 0)
+		beeper->period = 0;
+	else
+		beeper->period = HZ_TO_NANOSECONDS(value);
+
+	schedule_work(&beeper->work);
 
 	return 0;
 }
@@ -87,6 +101,8 @@ static int pwm_beeper_probe(struct platform_device *pdev)
 		goto err_free;
 	}
 
+	INIT_WORK(&beeper->work, pwm_beeper_work);
+
 	beeper->input = input_allocate_device();
 	if (!beeper->input) {
 		dev_err(&pdev->dev, "Failed to allocate input device\n");
@@ -133,6 +149,8 @@ static int pwm_beeper_remove(struct platform_device *pdev)
 {
 	struct pwm_beeper *beeper = platform_get_drvdata(pdev);
 
+	cancel_work_sync(&beeper->work);
+
 	input_unregister_device(beeper->input);
 
 	pwm_disable(beeper->pwm);
@@ -147,6 +165,8 @@ static int __maybe_unused pwm_beeper_suspend(struct device *dev)
 {
 	struct pwm_beeper *beeper = dev_get_drvdata(dev);
 
+	cancel_work_sync(&beeper->work);
+
 	if (beeper->period)
 		pwm_disable(beeper->pwm);
 
@@ -157,10 +177,8 @@ static int __maybe_unused pwm_beeper_resume(struct device *dev)
 {
 	struct pwm_beeper *beeper = dev_get_drvdata(dev);
 
-	if (beeper->period) {
-		pwm_config(beeper->pwm, beeper->period / 2, beeper->period);
-		pwm_enable(beeper->pwm);
-	}
+	if (beeper->period)
+		__pwm_beeper_set(beeper);
 
 	return 0;
 }
-- 
2.1.4

Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic

[Greg Kroah-Hartman]

On Wed, May 18, 2016 at 05:16:49PM +0200, Manfred Schlaegl wrote:
> Pwm config may sleep so defer it using a worker.
> 
> Trigger:
> On a Freescale i.MX53 based board we ran into "BUG: scheduling while
> atomic" because input_inject_event locks interrupts, but
> imx_pwm_config_v2 sleeps.
> 
> Tested on Freescale i.MX53 SoC with 4.6.0 and 4.1.24.
> 
> Unmodified applicable to
>  * 4.6    (stable)
>  * 4.5.4  (stable)
>  * 4.4.10 (longterm)
>  * 4.1.24 (longterm)
> 
> Modified applicable to
>  * 3.18.33 (longterm)

What does this all mean?  Have you read
Documentation/stable_kernel_rules.txt for how to mark things for stable
inclusion?

thanks,

greg k-h

Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic

[Manfred Schlaegl]

On 2016-05-18 18:06, Greg Kroah-Hartman wrote:
> On Wed, May 18, 2016 at 05:16:49PM +0200, Manfred Schlaegl wrote:
>> Pwm config may sleep so defer it using a worker.
>>
>> Trigger:
>> On a Freescale i.MX53 based board we ran into "BUG: scheduling while
>> atomic" because input_inject_event locks interrupts, but
>> imx_pwm_config_v2 sleeps.
>>
>> Tested on Freescale i.MX53 SoC with 4.6.0 and 4.1.24.
>>
>> Unmodified applicable to
>>  * 4.6    (stable)
>>  * 4.5.4  (stable)
>>  * 4.4.10 (longterm)
>>  * 4.1.24 (longterm)
>>
>> Modified applicable to
>>  * 3.18.33 (longterm)
> 
> What does this all mean?  Have you read
> Documentation/stable_kernel_rules.txt for how to mark things for stable
> inclusion?
> 
> thanks,
> 
> greg k-h
> 

Sorry, I'm afraid I missed that. Thanks for the clarification. I will respect that in the future.

Should I resend the patch with a cleaned up message (without "Unmodified applicable to" and "Modified applicable to" stuff)?
Is the rest of message (formally) ok?

thanks,
manfred

Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic

[Dmitry Torokhov]

Hi Manfred,

On Wed, May 18, 2016 at 05:16:49PM +0200, Manfred Schlaegl wrote:
> @@ -133,6 +149,8 @@ static int pwm_beeper_remove(struct platform_device *pdev)
>  {
>  	struct pwm_beeper *beeper = platform_get_drvdata(pdev);
>  
> +	cancel_work_sync(&beeper->work);
> +
>  	input_unregister_device(beeper->input);

This is racy, request to play may come in after cancel_work_sync()
returns but before we unregistered input device. I think you want the
version below.

-- 
Dmitry


Input: pwm-beeper - fix 'scheduling while atomic'

From: Manfred Schlaegl <manfred.schlaegl@gmx.at>

Pwm config may sleep so defer it using a worker.

Trigger:
On a Freescale i.MX53 based board we ran into "BUG: scheduling while
atomic" because input_inject_event locks interrupts, but
imx_pwm_config_v2 sleeps.

Tested on Freescale i.MX53 SoC with 4.6.0 and 4.1.24.

Signed-off-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/pwm-beeper.c |   70 +++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 21 deletions(-)

diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index f2261ab..27de150 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -20,21 +20,41 @@
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 #include <linux/slab.h>
+#include <linux/workqueue.h>
 
 struct pwm_beeper {
 	struct input_dev *input;
 	struct pwm_device *pwm;
+	struct work_struct work;
 	unsigned long period;
 };
 
 #define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
 
+static void __pwm_beeper_set(struct pwm_beeper *beeper)
+{
+	unsigned long period = beeper->period;
+
+	pwm_config(beeper->pwm, period / 2, period);
+
+	if (period == 0)
+		pwm_disable(beeper->pwm);
+	else
+		pwm_enable(beeper->pwm);
+}
+
+static void pwm_beeper_work(struct work_struct *work)
+{
+	struct pwm_beeper *beeper =
+		container_of(work, struct pwm_beeper, work);
+
+	__pwm_beeper_set(beeper);
+}
+
 static int pwm_beeper_event(struct input_dev *input,
 			    unsigned int type, unsigned int code, int value)
 {
-	int ret = 0;
 	struct pwm_beeper *beeper = input_get_drvdata(input);
-	unsigned long period;
 
 	if (type != EV_SND || value < 0)
 		return -EINVAL;
@@ -49,22 +69,31 @@ static int pwm_beeper_event(struct input_dev *input,
 		return -EINVAL;
 	}
 
-	if (value == 0) {
-		pwm_disable(beeper->pwm);
-	} else {
-		period = HZ_TO_NANOSECONDS(value);
-		ret = pwm_config(beeper->pwm, period / 2, period);
-		if (ret)
-			return ret;
-		ret = pwm_enable(beeper->pwm);
-		if (ret)
-			return ret;
-		beeper->period = period;
-	}
+	if (value == 0)
+		beeper->period = 0;
+	else
+		beeper->period = HZ_TO_NANOSECONDS(value);
+
+	schedule_work(&beeper->work);
 
 	return 0;
 }
 
+static void pwm_beeper_stop(struct pwm_beeper *beeper)
+{
+	cancel_work_sync(&beeper->work);
+
+	if (beeper->period)
+		pwm_disable(beeper->pwm);
+}
+
+static void pwm_beeper_close(struct input_dev *input)
+{
+	struct pwm_beeper *beeper = input_get_drvdata(input);
+
+	pwm_beeper_stop(beeper);
+}
+
 static int pwm_beeper_probe(struct platform_device *pdev)
 {
 	unsigned long pwm_id = (unsigned long)dev_get_platdata(&pdev->dev);
@@ -87,6 +116,8 @@ static int pwm_beeper_probe(struct platform_device *pdev)
 		goto err_free;
 	}
 
+	INIT_WORK(&beeper->work, pwm_beeper_work);
+
 	beeper->input = input_allocate_device();
 	if (!beeper->input) {
 		dev_err(&pdev->dev, "Failed to allocate input device\n");
@@ -106,6 +137,7 @@ static int pwm_beeper_probe(struct platform_device *pdev)
 	beeper->input->sndbit[0] = BIT(SND_TONE) | BIT(SND_BELL);
 
 	beeper->input->event = pwm_beeper_event;
+	beeper->input->close = pwm_beeper_close;
 
 	input_set_drvdata(beeper->input, beeper);
 
@@ -135,7 +167,6 @@ static int pwm_beeper_remove(struct platform_device *pdev)
 
 	input_unregister_device(beeper->input);
 
-	pwm_disable(beeper->pwm);
 	pwm_free(beeper->pwm);
 
 	kfree(beeper);
@@ -147,8 +178,7 @@ static int __maybe_unused pwm_beeper_suspend(struct device *dev)
 {
 	struct pwm_beeper *beeper = dev_get_drvdata(dev);
 
-	if (beeper->period)
-		pwm_disable(beeper->pwm);
+	pwm_beeper_stop(beeper);
 
 	return 0;
 }
@@ -157,10 +187,8 @@ static int __maybe_unused pwm_beeper_resume(struct device *dev)
 {
 	struct pwm_beeper *beeper = dev_get_drvdata(dev);
 
-	if (beeper->period) {
-		pwm_config(beeper->pwm, beeper->period / 2, beeper->period);
-		pwm_enable(beeper->pwm);
-	}
+	if (beeper->period)
+		__pwm_beeper_set(beeper);
 
 	return 0;
 }

Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic

[Manfred Schlaegl]

On 2016-05-20 18:59, Dmitry Torokhov wrote:
> Hi Manfred,
> 
> On Wed, May 18, 2016 at 05:16:49PM +0200, Manfred Schlaegl wrote:
>> @@ -133,6 +149,8 @@ static int pwm_beeper_remove(struct platform_device *pdev)
>>  {
>>  	struct pwm_beeper *beeper = platform_get_drvdata(pdev);
>>  
>> +	cancel_work_sync(&beeper->work);
>> +
>>  	input_unregister_device(beeper->input);
> 
> This is racy, request to play may come in after cancel_work_sync()
> returns but before we unregistered input device. I think you want the
> version below.
> 

Hi Dmitry,

yes you are right. Thank you for your feedback.
I also see that point, but I think it would be a simpler change just
to cancel the worker after unregistering the device (to reorder 
cancel_work_sync and input_unregister_device).

Patch will follow shortly.

What do you think?

Sincerely,
Manfred

Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic

[Manfred Schlaegl]

Pwm config may sleep so defer it using a worker.

On a Freescale i.MX53 based board we ran into "BUG: scheduling while
atomic" because input_inject_event locks interrupts, but
imx_pwm_config_v2 sleeps.

Tested on Freescale i.MX53 SoC with 4.6.0 and 4.1.24.

Signed-off-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>
---
 drivers/input/misc/pwm-beeper.c | 54 +++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 18 deletions(-)

diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index f2261ab..014495d3 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -20,21 +20,41 @@
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 #include <linux/slab.h>
+#include <linux/workqueue.h>
 
 struct pwm_beeper {
 	struct input_dev *input;
 	struct pwm_device *pwm;
+	struct work_struct work;
 	unsigned long period;
 };
 
 #define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
 
+static void __pwm_beeper_set(struct pwm_beeper *beeper)
+{
+	unsigned long period = beeper->period;
+
+	pwm_config(beeper->pwm, period / 2, period);
+
+	if (period == 0)
+		pwm_disable(beeper->pwm);
+	else
+		pwm_enable(beeper->pwm);
+}
+
+static void pwm_beeper_work(struct work_struct *work)
+{
+	struct pwm_beeper *beeper =
+		container_of(work, struct pwm_beeper, work);
+
+	__pwm_beeper_set(beeper);
+}
+
 static int pwm_beeper_event(struct input_dev *input,
 			    unsigned int type, unsigned int code, int value)
 {
-	int ret = 0;
 	struct pwm_beeper *beeper = input_get_drvdata(input);
-	unsigned long period;
 
 	if (type != EV_SND || value < 0)
 		return -EINVAL;
@@ -49,18 +69,12 @@ static int pwm_beeper_event(struct input_dev *input,
 		return -EINVAL;
 	}
 
-	if (value == 0) {
-		pwm_disable(beeper->pwm);
-	} else {
-		period = HZ_TO_NANOSECONDS(value);
-		ret = pwm_config(beeper->pwm, period / 2, period);
-		if (ret)
-			return ret;
-		ret = pwm_enable(beeper->pwm);
-		if (ret)
-			return ret;
-		beeper->period = period;
-	}
+	if (value == 0)
+		beeper->period = 0;
+	else
+		beeper->period = HZ_TO_NANOSECONDS(value);
+
+	schedule_work(&beeper->work);
 
 	return 0;
 }
@@ -87,6 +101,8 @@ static int pwm_beeper_probe(struct platform_device *pdev)
 		goto err_free;
 	}
 
+	INIT_WORK(&beeper->work, pwm_beeper_work);
+
 	beeper->input = input_allocate_device();
 	if (!beeper->input) {
 		dev_err(&pdev->dev, "Failed to allocate input device\n");
@@ -135,6 +151,8 @@ static int pwm_beeper_remove(struct platform_device *pdev)
 
 	input_unregister_device(beeper->input);
 
+	cancel_work_sync(&beeper->work);
+
 	pwm_disable(beeper->pwm);
 	pwm_free(beeper->pwm);
 
@@ -147,6 +165,8 @@ static int __maybe_unused pwm_beeper_suspend(struct device *dev)
 {
 	struct pwm_beeper *beeper = dev_get_drvdata(dev);
 
+	cancel_work_sync(&beeper->work);
+
 	if (beeper->period)
 		pwm_disable(beeper->pwm);
 
@@ -157,10 +177,8 @@ static int __maybe_unused pwm_beeper_resume(struct device *dev)
 {
 	struct pwm_beeper *beeper = dev_get_drvdata(dev);
 
-	if (beeper->period) {
-		pwm_config(beeper->pwm, beeper->period / 2, beeper->period);
-		pwm_enable(beeper->pwm);
-	}
+	if (beeper->period)
+		__pwm_beeper_set(beeper);
 
 	return 0;
 }
-- 
2.1.4

Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic

[Dmitry Torokhov]

On Tue, May 24, 2016 at 10:32:53AM +0200, Manfred Schlaegl wrote:
> On 2016-05-20 18:59, Dmitry Torokhov wrote:
> > Hi Manfred,
> > 
> > On Wed, May 18, 2016 at 05:16:49PM +0200, Manfred Schlaegl wrote:
> >> @@ -133,6 +149,8 @@ static int pwm_beeper_remove(struct platform_device *pdev)
> >>  {
> >>  	struct pwm_beeper *beeper = platform_get_drvdata(pdev);
> >>  
> >> +	cancel_work_sync(&beeper->work);
> >> +
> >>  	input_unregister_device(beeper->input);
> > 
> > This is racy, request to play may come in after cancel_work_sync()
> > returns but before we unregistered input device. I think you want the
> > version below.
> > 
> 
> Hi Dmitry,
> 
> yes you are right. Thank you for your feedback.
> I also see that point, but I think it would be a simpler change just
> to cancel the worker after unregistering the device (to reorder 
> cancel_work_sync and input_unregister_device).

That is an option, but I wanter to have close() because I also want to
convert the driver to used devm for allocating resources, and then we'd
need close() anyway so that we can get rid of remove() method.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic

[Manfred Schlaegl]

On 2016-05-26 02:36, Dmitry Torokhov wrote:
> On Tue, May 24, 2016 at 10:32:53AM +0200, Manfred Schlaegl wrote:
>> On 2016-05-20 18:59, Dmitry Torokhov wrote:
>>> Hi Manfred,
>>>
>>> On Wed, May 18, 2016 at 05:16:49PM +0200, Manfred Schlaegl wrote:
>>>> @@ -133,6 +149,8 @@ static int pwm_beeper_remove(struct platform_device *pdev)
>>>>  {
>>>>  	struct pwm_beeper *beeper = platform_get_drvdata(pdev);
>>>>  
>>>> +	cancel_work_sync(&beeper->work);
>>>> +
>>>>  	input_unregister_device(beeper->input);
>>>
>>> This is racy, request to play may come in after cancel_work_sync()
>>> returns but before we unregistered input device. I think you want the
>>> version below.
>>>
>>
>> Hi Dmitry,
>>
>> yes you are right. Thank you for your feedback.
>> I also see that point, but I think it would be a simpler change just
>> to cancel the worker after unregistering the device (to reorder 
>> cancel_work_sync and input_unregister_device).
> 
> That is an option, but I wanter to have close() because I also want to
> convert the driver to used devm for allocating resources, and then we'd
> need close() anyway so that we can get rid of remove() method.
> 
> Thanks.
> 

Ok. Thanks for clarification.
I will send a patch with the modifications you suggested before.

The following patch will also have some slight modifications in line numbers to make it apply after
cfae56f18 (input: misc: pwm-beeper: Explicitly apply PWM config extracted from pwm_args).

best regards,
Manfred

Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic

[Manfred Schlaegl]

On 2016-05-27 10:54, Manfred Schlaegl wrote:
> 
> Ok. Thanks for clarification.
> I will send a patch with the modifications you suggested before.
> 
> The following patch will also have some slight modifications in line numbers to make it apply after
> cfae56f18 (input: misc: pwm-beeper: Explicitly apply PWM config extracted from pwm_args).
> 
> best regards,
> Manfred
> 
While testing the patch I found another problem.

calling 
pwm_config(beeper->pwm, period / 2, period) with periode=0 leads to 

[  199.964836] Division by zero in kernel.
[  199.964875] CPU: 0 PID: 277 Comm: kworker/0:1 Not tainted 4.6.0-general-1-11011-g928f0cf #24
[  199.964887] Hardware name: Freescale i.MX53 (Device Tree Support)
[  199.964925] Workqueue: events pwm_beeper_work
[  199.964937] Backtrace: 
[  199.964970] [<c010a73c>] (dump_backtrace) from [<c010a920>] (show_stack+0x18/0x1c)
[  199.964980]  r6:00000000 r5:ce9fed9c r4:00000000 r3:00000000
[  199.965018] [<c010a908>] (show_stack) from [<c031ca78>] (dump_stack+0x20/0x28)
[  199.965037] [<c031ca58>] (dump_stack) from [<c010a888>] (__div0+0x18/0x20)
[  199.965053] [<c010a870>] (__div0) from [<c031b86c>] (Ldiv0+0x8/0x14)
[  199.965080] [<c034bbec>] (imx_pwm_config_v2) from [<c034bfb8>] (imx_pwm_config+0x68/0x88)
[  199.965088]  r9:00000000 r8:ceabf2c0 r7:00000000 r6:00000000 r5:ceabf440 r4:ce9fed9c
[  199.965121] [<c034bf50>] (imx_pwm_config) from [<c034b288>] (pwm_apply_state+0xfc/0x188)
[  199.965129]  r9:00000000 r8:cedd9a00 r7:00000000 r6:ceabf2e0 r5:ce9f7ec0 r4:ceabf2c0
[  199.965164] [<c034b18c>] (pwm_apply_state) from [<c0475b28>] (__pwm_beeper_set+0x60/0xd8)
[  199.965172]  r7:00000000 r6:ceabf2c0 r5:00000000 r4:cea92e80
[  199.965200] [<c0475ac8>] (__pwm_beeper_set) from [<c0475bb4>] (pwm_beeper_work+0x14/0x18)
[  199.965209]  r7:ce9da998 r6:c0908a80 r5:cea92e88 r4:ce9da980
[  199.965240] [<c0475ba0>] (pwm_beeper_work) from [<c01315c8>] (process_one_work+0x1f4/0x334)
[  199.965255] [<c01313d4>] (process_one_work) from [<c0131dc4>] (worker_thread+0x330/0x4ac)
[  199.965264]  r10:00000000 r9:00000008 r8:c0908a94 r7:ce9da998 r6:c0908a80 r5:c0908a80
[  199.965289]  r4:ce9da980
[  199.965311] [<c0131a94>] (worker_thread) from [<c0136308>] (kthread+0xe4/0xf8)
[  199.965319]  r10:00000000 r9:00000000 r8:00000000 r7:c0131a94 r6:ce9da980 r5:00000000
[  199.965342]  r4:cea7d900 r3:ce9f6000
[  199.965364] [<c0136224>] (kthread) from [<c01073b8>] (ret_from_fork+0x14/0x3c)
[  199.965372]  r7:00000000 r6:00000000 r5:c0136224 r4:cea7d900

I modified the patch, so that pwm_config is called only with periode >0

-       pwm_config(beeper->pwm, period / 2, period);
-
-       if (period == 0)
-               pwm_disable(beeper->pwm);
-       else
+       if (period) {
+               pwm_config(beeper->pwm, period / 2, period);
                pwm_enable(beeper->pwm);
+       } else
+               pwm_disable(beeper->pwm);

I will send the corrected patch shortly.

Best regards,
Manfred

Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic

[Manfred Schlaegl]

Pwm config may sleep so defer it using a worker.

On a Freescale i.MX53 based board we ran into "BUG: scheduling while
atomic" because input_inject_event locks interrupts, but
imx_pwm_config_v2 sleeps.

Tested on Freescale i.MX53 SoC with 4.6.0.

Signed-off-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>
---
 drivers/input/misc/pwm-beeper.c | 69 ++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 21 deletions(-)

diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index 8d71332..5f9655d 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -20,21 +20,40 @@
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 #include <linux/slab.h>
+#include <linux/workqueue.h>
 
 struct pwm_beeper {
 	struct input_dev *input;
 	struct pwm_device *pwm;
+	struct work_struct work;
 	unsigned long period;
 };
 
 #define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
 
+static void __pwm_beeper_set(struct pwm_beeper *beeper)
+{
+	unsigned long period = beeper->period;
+
+	if (period) {
+		pwm_config(beeper->pwm, period / 2, period);
+		pwm_enable(beeper->pwm);
+	} else
+		pwm_disable(beeper->pwm);
+}
+
+static void pwm_beeper_work(struct work_struct *work)
+{
+	struct pwm_beeper *beeper =
+		container_of(work, struct pwm_beeper, work);
+
+	__pwm_beeper_set(beeper);
+}
+
 static int pwm_beeper_event(struct input_dev *input,
 			    unsigned int type, unsigned int code, int value)
 {
-	int ret = 0;
 	struct pwm_beeper *beeper = input_get_drvdata(input);
-	unsigned long period;
 
 	if (type != EV_SND || value < 0)
 		return -EINVAL;
@@ -49,22 +68,31 @@ static int pwm_beeper_event(struct input_dev *input,
 		return -EINVAL;
 	}
 
-	if (value == 0) {
-		pwm_disable(beeper->pwm);
-	} else {
-		period = HZ_TO_NANOSECONDS(value);
-		ret = pwm_config(beeper->pwm, period / 2, period);
-		if (ret)
-			return ret;
-		ret = pwm_enable(beeper->pwm);
-		if (ret)
-			return ret;
-		beeper->period = period;
-	}
+	if (value == 0)
+		beeper->period = 0;
+	else
+		beeper->period = HZ_TO_NANOSECONDS(value);
+
+	schedule_work(&beeper->work);
 
 	return 0;
 }
 
+static void pwm_beeper_stop(struct pwm_beeper *beeper)
+{
+	cancel_work_sync(&beeper->work);
+
+	if (beeper->period)
+		pwm_disable(beeper->pwm);
+}
+
+static void pwm_beeper_close(struct input_dev *input)
+{
+	struct pwm_beeper *beeper = input_get_drvdata(input);
+
+	pwm_beeper_stop(beeper);
+}
+
 static int pwm_beeper_probe(struct platform_device *pdev)
 {
 	unsigned long pwm_id = (unsigned long)dev_get_platdata(&pdev->dev);
@@ -93,6 +121,8 @@ static int pwm_beeper_probe(struct platform_device *pdev)
 	 */
 	pwm_apply_args(beeper->pwm);
 
+	INIT_WORK(&beeper->work, pwm_beeper_work);
+
 	beeper->input = input_allocate_device();
 	if (!beeper->input) {
 		dev_err(&pdev->dev, "Failed to allocate input device\n");
@@ -112,6 +142,7 @@ static int pwm_beeper_probe(struct platform_device *pdev)
 	beeper->input->sndbit[0] = BIT(SND_TONE) | BIT(SND_BELL);
 
 	beeper->input->event = pwm_beeper_event;
+	beeper->input->close = pwm_beeper_close;
 
 	input_set_drvdata(beeper->input, beeper);
 
@@ -141,7 +172,6 @@ static int pwm_beeper_remove(struct platform_device *pdev)
 
 	input_unregister_device(beeper->input);
 
-	pwm_disable(beeper->pwm);
 	pwm_free(beeper->pwm);
 
 	kfree(beeper);
@@ -153,8 +183,7 @@ static int __maybe_unused pwm_beeper_suspend(struct device *dev)
 {
 	struct pwm_beeper *beeper = dev_get_drvdata(dev);
 
-	if (beeper->period)
-		pwm_disable(beeper->pwm);
+	pwm_beeper_stop(beeper);
 
 	return 0;
 }
@@ -163,10 +192,8 @@ static int __maybe_unused pwm_beeper_resume(struct device *dev)
 {
 	struct pwm_beeper *beeper = dev_get_drvdata(dev);
 
-	if (beeper->period) {
-		pwm_config(beeper->pwm, beeper->period / 2, beeper->period);
-		pwm_enable(beeper->pwm);
-	}
+	if (beeper->period)
+		__pwm_beeper_set(beeper);
 
 	return 0;
 }
-- 
2.1.4

Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic

[Dmitry Torokhov]

On Fri, May 27, 2016 at 11:14:27AM +0200, Manfred Schlaegl wrote:
> Pwm config may sleep so defer it using a worker.
> 
> On a Freescale i.MX53 based board we ran into "BUG: scheduling while
> atomic" because input_inject_event locks interrupts, but
> imx_pwm_config_v2 sleeps.
> 
> Tested on Freescale i.MX53 SoC with 4.6.0.
> 
> Signed-off-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>

Applied, thank you.

> ---
>  drivers/input/misc/pwm-beeper.c | 69 ++++++++++++++++++++++++++++-------------
>  1 file changed, 48 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
> index 8d71332..5f9655d 100644
> --- a/drivers/input/misc/pwm-beeper.c
> +++ b/drivers/input/misc/pwm-beeper.c
> @@ -20,21 +20,40 @@
>  #include <linux/platform_device.h>
>  #include <linux/pwm.h>
>  #include <linux/slab.h>
> +#include <linux/workqueue.h>
>  
>  struct pwm_beeper {
>  	struct input_dev *input;
>  	struct pwm_device *pwm;
> +	struct work_struct work;
>  	unsigned long period;
>  };
>  
>  #define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
>  
> +static void __pwm_beeper_set(struct pwm_beeper *beeper)
> +{
> +	unsigned long period = beeper->period;
> +
> +	if (period) {
> +		pwm_config(beeper->pwm, period / 2, period);
> +		pwm_enable(beeper->pwm);
> +	} else
> +		pwm_disable(beeper->pwm);
> +}
> +
> +static void pwm_beeper_work(struct work_struct *work)
> +{
> +	struct pwm_beeper *beeper =
> +		container_of(work, struct pwm_beeper, work);
> +
> +	__pwm_beeper_set(beeper);
> +}
> +
>  static int pwm_beeper_event(struct input_dev *input,
>  			    unsigned int type, unsigned int code, int value)
>  {
> -	int ret = 0;
>  	struct pwm_beeper *beeper = input_get_drvdata(input);
> -	unsigned long period;
>  
>  	if (type != EV_SND || value < 0)
>  		return -EINVAL;
> @@ -49,22 +68,31 @@ static int pwm_beeper_event(struct input_dev *input,
>  		return -EINVAL;
>  	}
>  
> -	if (value == 0) {
> -		pwm_disable(beeper->pwm);
> -	} else {
> -		period = HZ_TO_NANOSECONDS(value);
> -		ret = pwm_config(beeper->pwm, period / 2, period);
> -		if (ret)
> -			return ret;
> -		ret = pwm_enable(beeper->pwm);
> -		if (ret)
> -			return ret;
> -		beeper->period = period;
> -	}
> +	if (value == 0)
> +		beeper->period = 0;
> +	else
> +		beeper->period = HZ_TO_NANOSECONDS(value);
> +
> +	schedule_work(&beeper->work);
>  
>  	return 0;
>  }
>  
> +static void pwm_beeper_stop(struct pwm_beeper *beeper)
> +{
> +	cancel_work_sync(&beeper->work);
> +
> +	if (beeper->period)
> +		pwm_disable(beeper->pwm);
> +}
> +
> +static void pwm_beeper_close(struct input_dev *input)
> +{
> +	struct pwm_beeper *beeper = input_get_drvdata(input);
> +
> +	pwm_beeper_stop(beeper);
> +}
> +
>  static int pwm_beeper_probe(struct platform_device *pdev)
>  {
>  	unsigned long pwm_id = (unsigned long)dev_get_platdata(&pdev->dev);
> @@ -93,6 +121,8 @@ static int pwm_beeper_probe(struct platform_device *pdev)
>  	 */
>  	pwm_apply_args(beeper->pwm);
>  
> +	INIT_WORK(&beeper->work, pwm_beeper_work);
> +
>  	beeper->input = input_allocate_device();
>  	if (!beeper->input) {
>  		dev_err(&pdev->dev, "Failed to allocate input device\n");
> @@ -112,6 +142,7 @@ static int pwm_beeper_probe(struct platform_device *pdev)
>  	beeper->input->sndbit[0] = BIT(SND_TONE) | BIT(SND_BELL);
>  
>  	beeper->input->event = pwm_beeper_event;
> +	beeper->input->close = pwm_beeper_close;
>  
>  	input_set_drvdata(beeper->input, beeper);
>  
> @@ -141,7 +172,6 @@ static int pwm_beeper_remove(struct platform_device *pdev)
>  
>  	input_unregister_device(beeper->input);
>  
> -	pwm_disable(beeper->pwm);
>  	pwm_free(beeper->pwm);
>  
>  	kfree(beeper);
> @@ -153,8 +183,7 @@ static int __maybe_unused pwm_beeper_suspend(struct device *dev)
>  {
>  	struct pwm_beeper *beeper = dev_get_drvdata(dev);
>  
> -	if (beeper->period)
> -		pwm_disable(beeper->pwm);
> +	pwm_beeper_stop(beeper);
>  
>  	return 0;
>  }
> @@ -163,10 +192,8 @@ static int __maybe_unused pwm_beeper_resume(struct device *dev)
>  {
>  	struct pwm_beeper *beeper = dev_get_drvdata(dev);
>  
> -	if (beeper->period) {
> -		pwm_config(beeper->pwm, beeper->period / 2, beeper->period);
> -		pwm_enable(beeper->pwm);
> -	}
> +	if (beeper->period)
> +		__pwm_beeper_set(beeper);
>  
>  	return 0;
>  }
> -- 
> 2.1.4
> 

-- 
Dmitry