| Message ID | 20160520165918.GE14951@dtor-ws (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }