[PATCH] input: keyboard: snvs: make sure irq is handled correctly

[PATCH] input: keyboard: snvs: make sure irq is handled correctly

[Anson Huang]

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

Re: [PATCH] input: keyboard: snvs: make sure irq is handled correctly

[dmitry.torokhov]

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

RE: [PATCH] input: keyboard: snvs: make sure irq is handled correctly

[Anson Huang]

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