Input: pwm-beeper - defer pwm config if pwm can sleep

PATCH In review Manfred Schlaegl 4 participants 17 messages Feb 17, 2016 – May 27, 2016

Subsystems (2)

HID Input drivers

lore

Share & tools

lore

Manfred Schlaegl

author

2016-02-17 13:19 UTC #0 lore raw

PATCH 1/1 Input: pwm-beeper - defer pwm config if pwm can sleep

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 <[email protected]>

---
 drivers/input/misc/pwm-beeper.c | 62 +++++++++++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 18 deletions(-)

drivers/input/misc/pwm-beeper.c +44 ~~-18~~

index f2261ab..c160b5e 100644
@@ -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;
 }

Signature · 1 line

2.1.4

Dmitry Torokhov

maintainer

2016-02-22 19:46 UTC #1 lore raw

Quoted 30 lines · 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 <[email protected]>
---
 drivers/input/misc/pwm-beeper.c | 62 +++++++++++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 18 deletions(-)
drivers/input/misc/pwm-beeper.c +0 -0
index f2261ab..c160b5e 100644
@@ -20,21 +20,42 @@
 #include <linux/platform_device.h>

drivers/input/misc/pwm-beeper.c line 30

Quoted 10 lines

drivers/input/misc/pwm-beeper.c +3 -0

 #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.

Signature · 1 line

Dmitry

Manfred Schlaegl

author

2016-02-23 08:46 UTC #2 lore raw

Quoted 45 lines · Dmitry Torokhov

Quoted 30 lines · 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 <[email protected]>
---
 drivers/input/misc/pwm-beeper.c | 62 +++++++++++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 18 deletions(-)
drivers/input/misc/pwm-beeper.c +0 -0
index f2261ab..c160b5e 100644
@@ -20,21 +20,42 @@
 #include <linux/platform_device.h>
drivers/input/misc/pwm-beeper.c line 30
Quoted 10 lines
drivers/input/misc/pwm-beeper.c +3 -0
 #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.

Quoted 2 lines

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

Manfred Schlaegl

author

2016-03-30 14:57 UTC #3 lore raw

Hello,

Any more feedback on this?

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

kindly regards,
manfred

Thierry Reding

2016-05-12 12:18 UTC #4 lore raw

In reply to Dmitry Torokhov

Quoted 35 lines · Dmitry Torokhov

Quoted 34 lines · 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 <[email protected]>
---
 drivers/input/misc/pwm-beeper.c | 62 +++++++++++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 18 deletions(-)
drivers/input/misc/pwm-beeper.c +1 -0
index f2261ab..c160b5e 100644
@@ -20,21 +20,42 @@
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 #include <linux/slab.h>
+#include <linux/workqueue.h>
 

Quoted 9 lines

drivers/input/misc/pwm-beeper.c line 30
Quoted 6 lines
drivers/input/misc/pwm-beeper.c +2 -0
 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

Manfred Schlaegl

author

2016-05-13 15:38 UTC #5 lore raw

Quoted 7 lines · Thierry Reding

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

Manfred Schlaegl

author

2016-05-18 15:17 UTC #6 lore raw

PATCH 1/1 Input: pwm-beeper - fix: scheduling while atomic

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 <[email protected]>

---
 drivers/input/misc/pwm-beeper.c | 54 +++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 18 deletions(-)

drivers/input/misc/pwm-beeper.c +36 ~~-18~~

index f2261ab..7d783c8 100644
@@ -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;
 }

Signature · 1 line

2.1.4

Greg Kroah-Hartman

2016-05-18 16:06 UTC #7 lore raw

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

Quoted 9 lines · 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.

Quoted 8 lines

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

Manfred Schlaegl

author

2016-05-19 07:53 UTC #8 lore raw

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

Quoted 19 lines · Greg Kroah-Hartman

Quoted 9 lines · 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.
Quoted 8 lines
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)

Quoted 8 lines

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

Dmitry Torokhov

maintainer

2016-05-20 16:59 UTC #9 lore raw

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

In reply to Manfred Schlaegl

Hi Manfred,

Review static int pwm_beeper_remove(struct platform_device *pdev) · line 154

Quoted 7 lines · Manfred Schlaegl

(quoted diff) +2 -0

@@ -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.

Signature · 21 lines

Dmitry


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

From: Manfred Schlaegl <[email protected]>

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 <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
---
 drivers/input/misc/pwm-beeper.c |   70 +++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 21 deletions(-)

drivers/input/misc/pwm-beeper.c +49 ~~-21~~

index f2261ab..27de150 100644
@@ -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;
 }

Manfred Schlaegl

author

2016-05-24 08:32 UTC #10 lore raw

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

Quoted 7 lines · Dmitry Torokhov

Hi Manfred,

Quoted 4 lines · Manfred Schlaegl

(quoted diff) +0 -0

@@ -133,6 +149,8 @@ static int pwm_beeper_remove(struct platform_device *pdev)
 {
 	struct pwm_beeper *beeper = platform_get_drvdata(pdev);

Quoted 8 lines

drivers/input/misc/pwm-beeper.c static int pwm_beeper_event(struct input_dev *input,
Quoted 3 lines
drivers/input/misc/pwm-beeper.c +2 -0
+	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

Manfred Schlaegl

author

2016-05-24 08:37 UTC #11 lore raw

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

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 <[email protected]>

---
 drivers/input/misc/pwm-beeper.c | 54 +++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 18 deletions(-)

drivers/input/misc/pwm-beeper.c +36 ~~-18~~

index f2261ab..014495d3 100644
@@ -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;
 }

Signature · 1 line

2.1.4

Dmitry Torokhov

maintainer

2016-05-26 00:36 UTC #12 lore raw

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

In reply to Manfred Schlaegl

Quoted 17 lines · Manfred Schlaegl

Quoted 7 lines · Dmitry Torokhov
Hi Manfred,
Quoted 4 lines · Manfred Schlaegl
(quoted diff) +0 -0
@@ -133,6 +149,8 @@ static int pwm_beeper_remove(struct platform_device *pdev)
 {
 	struct pwm_beeper *beeper = platform_get_drvdata(pdev);
 
Quoted 8 lines
drivers/input/misc/pwm-beeper.c static int pwm_beeper_event(struct input_dev *input,
Quoted 3 lines
drivers/input/misc/pwm-beeper.c +2 -0
+	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.

Quoted 6 lines

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.

Signature · 1 line

Dmitry

Manfred Schlaegl

author

2016-05-27 08:54 UTC #13 lore raw

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

Quoted 25 lines · Dmitry Torokhov

Quoted 17 lines · Manfred Schlaegl
Quoted 7 lines · Dmitry Torokhov
Hi Manfred,
Quoted 4 lines · Manfred Schlaegl
(quoted diff) +0 -0
@@ -133,6 +149,8 @@ static int pwm_beeper_remove(struct platform_device *pdev)
 {
 	struct pwm_beeper *beeper = platform_get_drvdata(pdev);
 
Quoted 8 lines
drivers/input/misc/pwm-beeper.c static int pwm_beeper_event(struct input_dev *input,
Quoted 3 lines
drivers/input/misc/pwm-beeper.c +2 -0
+	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.
Quoted 6 lines
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).

Quoted 6 lines

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

Manfred Schlaegl

author

2016-05-27 09:11 UTC #14 lore raw

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

Quoted 10 lines · Manfred Schlaegl

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

Manfred Schlaegl

author

2016-05-27 09:14 UTC #15 lore raw

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

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 <[email protected]>

---
 drivers/input/misc/pwm-beeper.c | 69 ++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 21 deletions(-)

drivers/input/misc/pwm-beeper.c +48 ~~-21~~

index 8d71332..5f9655d 100644
@@ -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;
 }

Signature · 1 line

2.1.4

Dmitry Torokhov

maintainer Applied

2016-05-27 23:38 UTC #16 lore raw

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

Quoted 9 lines · 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 <[email protected]>

Applied, thank you.

Quoted 146 lines

---
 drivers/input/misc/pwm-beeper.c | 69 ++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 21 deletions(-)

drivers/input/misc/pwm-beeper.c +48 ~~-21~~

index 8d71332..5f9655d 100644
@@ -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;
 }

Signature · 1 line

2.1.4

Signature · 1 line

Dmitry