| Message ID | 1392026859-4977-1-git-send-email-21cnbao@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Barry, On Mon, Feb 10, 2014 at 06:07:39PM +0800, Barry Song wrote: > > static int sirfsoc_pwrc_remove(struct platform_device *pdev) > { > + struct sirfsoc_pwrc_drvdata *pwrcdrv = dev_get_drvdata(&pdev->dev); > + > device_init_wakeup(&pdev->dev, 0); > > + cancel_delayed_work_sync(&pwrcdrv->work); > + This is racy: interrupt is freed later and can schedule work again. Thanks.
2014-02-13 7:11 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>: > Hi Barry, > > On Mon, Feb 10, 2014 at 06:07:39PM +0800, Barry Song wrote: >> >> static int sirfsoc_pwrc_remove(struct platform_device *pdev) >> { >> + struct sirfsoc_pwrc_drvdata *pwrcdrv = dev_get_drvdata(&pdev->dev); >> + >> device_init_wakeup(&pdev->dev, 0); >> >> + cancel_delayed_work_sync(&pwrcdrv->work); >> + > > This is racy: interrupt is freed later and can schedule work again. thanks, Dmitry. i will do a manual devm_free_irq() before cancelling the work and before devres removes the resources. > > Thanks. > > -- > Dmitry -barry -- 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 February 12, 2014 6:32:03 PM PST, Barry Song <21cnbao@gmail.com> wrote: >2014-02-13 7:11 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>: >> Hi Barry, >> >> On Mon, Feb 10, 2014 at 06:07:39PM +0800, Barry Song wrote: >>> >>> static int sirfsoc_pwrc_remove(struct platform_device *pdev) >>> { >>> + struct sirfsoc_pwrc_drvdata *pwrcdrv = >dev_get_drvdata(&pdev->dev); >>> + >>> device_init_wakeup(&pdev->dev, 0); >>> >>> + cancel_delayed_work_sync(&pwrcdrv->work); >>> + >> >> This is racy: interrupt is freed later and can schedule work again. > >thanks, Dmitry. i will do a manual devm_free_irq() before cancelling >the work and before devres removes the resources. Another option would be to use devm custom action to ensure that work is canceled after freeing IRQ.
2014-02-13 11:41 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>: > On February 12, 2014 6:32:03 PM PST, Barry Song <21cnbao@gmail.com> wrote: >>2014-02-13 7:11 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>: >>> Hi Barry, >>> >>> On Mon, Feb 10, 2014 at 06:07:39PM +0800, Barry Song wrote: >>>> >>>> static int sirfsoc_pwrc_remove(struct platform_device *pdev) >>>> { >>>> + struct sirfsoc_pwrc_drvdata *pwrcdrv = >>dev_get_drvdata(&pdev->dev); >>>> + >>>> device_init_wakeup(&pdev->dev, 0); >>>> >>>> + cancel_delayed_work_sync(&pwrcdrv->work); >>>> + >>> >>> This is racy: interrupt is freed later and can schedule work again. >> >>thanks, Dmitry. i will do a manual devm_free_irq() before cancelling >>the work and before devres removes the resources. > > Another option would be to use devm custom action to ensure that work is canceled after freeing IRQ. yes. you did a great job to have a devm_add_action() API. > > > -- > Dmitry -barry -- 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/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c index e8897c3..9cd810b 100644 --- a/drivers/input/misc/sirfsoc-onkey.c +++ b/drivers/input/misc/sirfsoc-onkey.c @@ -13,16 +13,44 @@ #include <linux/input.h> #include <linux/rtc/sirfsoc_rtciobrg.h> #include <linux/of.h> +#include <linux/workqueue.h> struct sirfsoc_pwrc_drvdata { u32 pwrc_base; struct input_dev *input; + struct delayed_work work; }; #define PWRC_ON_KEY_BIT (1 << 0) #define PWRC_INT_STATUS 0xc #define PWRC_INT_MASK 0x10 +#define PWRC_PIN_STATUS 0x14 +#define PWRC_KEY_DETECT_UP_TIME 20 /* ms*/ + +static inline int sirfsoc_pwrc_is_on_key_down( + struct sirfsoc_pwrc_drvdata *pwrcdrv) +{ + int state = sirfsoc_rtc_iobrg_readl( + pwrcdrv->pwrc_base + PWRC_PIN_STATUS) + & PWRC_ON_KEY_BIT; + return !state; /* ON_KEY is active low */ +} + +static void sirfsoc_pwrc_report_event(struct work_struct *work) +{ + struct sirfsoc_pwrc_drvdata *pwrcdrv = + container_of((struct delayed_work *)work, + struct sirfsoc_pwrc_drvdata, work); + + if (!sirfsoc_pwrc_is_on_key_down(pwrcdrv)) { + input_event(pwrcdrv->input, EV_PWR, KEY_POWER, 0); + input_sync(pwrcdrv->input); + } else { + schedule_delayed_work(&pwrcdrv->work, + msecs_to_jiffies(PWRC_KEY_DETECT_UP_TIME)); + } +} static irqreturn_t sirfsoc_pwrc_isr(int irq, void *dev_id) { @@ -34,17 +62,11 @@ static irqreturn_t sirfsoc_pwrc_isr(int irq, void *dev_id) sirfsoc_rtc_iobrg_writel(int_status & ~PWRC_ON_KEY_BIT, pwrcdrv->pwrc_base + PWRC_INT_STATUS); - /* - * For a typical Linux system, we report KEY_SUSPEND to trigger apm-power.c - * to queue a SUSPEND APM event - */ - input_event(pwrcdrv->input, EV_PWR, KEY_SUSPEND, 1); - input_sync(pwrcdrv->input); - /* - * Todo: report KEY_POWER event for Android platforms, Android PowerManager - * will handle the suspend and powerdown/hibernation - */ + input_event(pwrcdrv->input, EV_PWR, KEY_POWER, 1); + input_sync(pwrcdrv->input); + schedule_delayed_work(&pwrcdrv->work, + msecs_to_jiffies(PWRC_KEY_DETECT_UP_TIME)); return IRQ_HANDLED; } @@ -88,6 +110,8 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev) pwrcdrv->input->phys = "pwrc/input0"; pwrcdrv->input->evbit[0] = BIT_MASK(EV_PWR); + INIT_DELAYED_WORK(&pwrcdrv->work, sirfsoc_pwrc_report_event); + irq = platform_get_irq(pdev, 0); error = devm_request_irq(&pdev->dev, irq, sirfsoc_pwrc_isr, IRQF_SHARED, @@ -119,8 +143,12 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev) static int sirfsoc_pwrc_remove(struct platform_device *pdev) { + struct sirfsoc_pwrc_drvdata *pwrcdrv = dev_get_drvdata(&pdev->dev); + device_init_wakeup(&pdev->dev, 0); + cancel_delayed_work_sync(&pwrcdrv->work); + return 0; }