Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1400869 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] Input: pwm-beeper - defer pwm config if pwm can sleep Manfred Schlaegl <manfred.schlaegl@gmx.at> - 2016-05-13 17:40 +0200
    [PATCH] Input: pwm-beeper - fix: scheduling while atomic Manfred Schlaegl <manfred.schlaegl@gmx.at> - 2016-05-18 17:20 +0200
      Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-05-18 18:10 +0200
        Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic Manfred Schlaegl <manfred.schlaegl@gmx.at> - 2016-05-19 10:00 +0200
      Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-05-20 19:00 +0200
        Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic Manfred Schlaegl <manfred.schlaegl@gmx.at> - 2016-05-24 10:40 +0200
          Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic Manfred Schlaegl <manfred.schlaegl@gmx.at> - 2016-05-24 10:40 +0200
          Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-05-26 02:40 +0200
            Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic Manfred Schlaegl <manfred.schlaegl@gmx.at> - 2016-05-27 11:00 +0200
              Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic Manfred Schlaegl <manfred.schlaegl@gmx.at> - 2016-05-27 11:20 +0200
                Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic Manfred Schlaegl <manfred.schlaegl@gmx.at> - 2016-05-27 11:20 +0200
                  Re: [PATCH] Input: pwm-beeper - fix: scheduling while atomic Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-05-28 01:40 +0200

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

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

[toc] | [next] | [standalone]

#1403089 — [PATCH] 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 <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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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;
 }

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1405945 — 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 <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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1407949 — 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 <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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web