Hi, Dmitry Best Regards! Anson Huang > -----Original Message----- > From: dmitry.torokhov@xxxxxxxxx [mailto:dmitry.torokhov@xxxxxxxxx] > Sent: 2019年3月27日 12:29 > To: Anson Huang <anson.huang@xxxxxxx> > Cc: Fabio Estevam <fabio.estevam@xxxxxxx>; linux-input@xxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > 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@xxxxxxx> > > --- > > 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
