* [PATCH] input: keyboard: snvs: make sure irq is handled correctly
@ 2019-03-27 2:47 Anson Huang
2019-03-27 4:29 ` dmitry.torokhov
0 siblings, 1 reply; 3+ messages in thread
From: Anson Huang @ 2019-03-27 2:47 UTC (permalink / raw)
To: dmitry.torokhov@gmail.com, Fabio Estevam,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: dl-linux-imx
SNVS IRQ is requested before necessary driver data initialized,
if there is a pending IRQ during driver probe phase, kernel
NULL pointer panic will occur in IRQ handler. To avoid such
scenario, need to move the IRQ request to after driver data
initialization done. This patch is inspired by NXP's internal
kernel tree.
Fixes: d3dc6e232215 ("input: keyboard: imx: add snvs power key driver")
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
drivers/input/keyboard/snvs_pwrkey.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index effb632..6ff41fd 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -148,15 +148,6 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
return error;
}
- error = devm_request_irq(&pdev->dev, pdata->irq,
- imx_snvs_pwrkey_interrupt,
- 0, pdev->name, pdev);
-
- if (error) {
- dev_err(&pdev->dev, "interrupt not available.\n");
- return error;
- }
-
error = input_register_device(input);
if (error < 0) {
dev_err(&pdev->dev, "failed to register input device\n");
@@ -166,6 +157,14 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
pdata->input = input;
platform_set_drvdata(pdev, pdata);
+ error = devm_request_irq(&pdev->dev, pdata->irq,
+ imx_snvs_pwrkey_interrupt,
+ 0, pdev->name, pdev);
+ if (error) {
+ dev_err(&pdev->dev, "interrupt not available.\n");
+ return error;
+ }
+
device_init_wakeup(&pdev->dev, pdata->wakeup);
return 0;
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] input: keyboard: snvs: make sure irq is handled correctly
2019-03-27 2:47 [PATCH] input: keyboard: snvs: make sure irq is handled correctly Anson Huang
@ 2019-03-27 4:29 ` dmitry.torokhov
2019-03-27 5:05 ` Anson Huang
0 siblings, 1 reply; 3+ messages in thread
From: dmitry.torokhov @ 2019-03-27 4:29 UTC (permalink / raw)
To: Anson Huang
Cc: Fabio Estevam, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, dl-linux-imx
Hi Anson,
On Wed, Mar 27, 2019 at 02:47:06AM +0000, Anson Huang wrote:
> SNVS IRQ is requested before necessary driver data initialized,
> if there is a pending IRQ during driver probe phase, kernel
> NULL pointer panic will occur in IRQ handler. To avoid such
> scenario, need to move the IRQ request to after driver data
> initialization done. This patch is inspired by NXP's internal
> kernel tree.
>
> Fixes: d3dc6e232215 ("input: keyboard: imx: add snvs power key driver")
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> drivers/input/keyboard/snvs_pwrkey.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index effb632..6ff41fd 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -148,15 +148,6 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> return error;
> }
>
> - error = devm_request_irq(&pdev->dev, pdata->irq,
> - imx_snvs_pwrkey_interrupt,
> - 0, pdev->name, pdev);
> -
> - if (error) {
> - dev_err(&pdev->dev, "interrupt not available.\n");
> - return error;
> - }
> -
> error = input_register_device(input);
> if (error < 0) {
> dev_err(&pdev->dev, "failed to register input device\n");
> @@ -166,6 +157,14 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> pdata->input = input;
> platform_set_drvdata(pdev, pdata);
>
> + error = devm_request_irq(&pdev->dev, pdata->irq,
> + imx_snvs_pwrkey_interrupt,
> + 0, pdev->name, pdev);
> + if (error) {
> + dev_err(&pdev->dev, "interrupt not available.\n");
> + return error;
> + }
Instead of moving devm_request_irq() around could you simply move
pdata->input = input assignment higher? It is perfectly fine to try
calling input_event() on input device that is allocated but not yet
registered.
> +
> device_init_wakeup(&pdev->dev, pdata->wakeup);
Unrelated suggestion:
Can you try calling dev_pm_set_wake_irq(&pdev->dev, pdata->irq) here and
I think you will be able to get rid of suspend/resume methods in the
driver.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 3+ messages in thread* RE: [PATCH] input: keyboard: snvs: make sure irq is handled correctly
2019-03-27 4:29 ` dmitry.torokhov
@ 2019-03-27 5:05 ` Anson Huang
0 siblings, 0 replies; 3+ messages in thread
From: Anson Huang @ 2019-03-27 5:05 UTC (permalink / raw)
To: dmitry.torokhov@gmail.com
Cc: Fabio Estevam, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, dl-linux-imx
Hi, Dmitry
Best Regards!
Anson Huang
> -----Original Message-----
> From: dmitry.torokhov@gmail.com [mailto:dmitry.torokhov@gmail.com]
> Sent: 2019年3月27日 12:29
> To: Anson Huang <anson.huang@nxp.com>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>; linux-input@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH] input: keyboard: snvs: make sure irq is handled
> correctly
>
> Hi Anson,
>
> On Wed, Mar 27, 2019 at 02:47:06AM +0000, Anson Huang wrote:
> > SNVS IRQ is requested before necessary driver data initialized, if
> > there is a pending IRQ during driver probe phase, kernel NULL pointer
> > panic will occur in IRQ handler. To avoid such scenario, need to move
> > the IRQ request to after driver data initialization done. This patch
> > is inspired by NXP's internal kernel tree.
> >
> > Fixes: d3dc6e232215 ("input: keyboard: imx: add snvs power key
> > driver")
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > drivers/input/keyboard/snvs_pwrkey.c | 17 ++++++++---------
> > 1 file changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/snvs_pwrkey.c
> > b/drivers/input/keyboard/snvs_pwrkey.c
> > index effb632..6ff41fd 100644
> > --- a/drivers/input/keyboard/snvs_pwrkey.c
> > +++ b/drivers/input/keyboard/snvs_pwrkey.c
> > @@ -148,15 +148,6 @@ static int imx_snvs_pwrkey_probe(struct
> platform_device *pdev)
> > return error;
> > }
> >
> > - error = devm_request_irq(&pdev->dev, pdata->irq,
> > - imx_snvs_pwrkey_interrupt,
> > - 0, pdev->name, pdev);
> > -
> > - if (error) {
> > - dev_err(&pdev->dev, "interrupt not available.\n");
> > - return error;
> > - }
> > -
> > error = input_register_device(input);
> > if (error < 0) {
> > dev_err(&pdev->dev, "failed to register input device\n");
> @@ -166,6
> > +157,14 @@ static int imx_snvs_pwrkey_probe(struct platform_device
> *pdev)
> > pdata->input = input;
> > platform_set_drvdata(pdev, pdata);
> >
> > + error = devm_request_irq(&pdev->dev, pdata->irq,
> > + imx_snvs_pwrkey_interrupt,
> > + 0, pdev->name, pdev);
> > + if (error) {
> > + dev_err(&pdev->dev, "interrupt not available.\n");
> > + return error;
> > + }
>
> Instead of moving devm_request_irq() around could you simply move
> pdata->input = input assignment higher? It is perfectly fine to try
> calling input_event() on input device that is allocated but not yet registered.
OK, make sense.
>
> > +
> > device_init_wakeup(&pdev->dev, pdata->wakeup);
>
> Unrelated suggestion:
>
> Can you try calling dev_pm_set_wake_irq(&pdev->dev, pdata->irq) here and
> I think you will be able to get rid of suspend/resume methods in the driver.
OK, I will look into it, and send another patch to improve this.
Thanks.
Anson
>
> Thanks.
>
> --
> Dmitry
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-03-27 5:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-27 2:47 [PATCH] input: keyboard: snvs: make sure irq is handled correctly Anson Huang
2019-03-27 4:29 ` dmitry.torokhov
2019-03-27 5:05 ` Anson Huang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).