linux-kernel - Re: [PATCH v2 6/6] Input: snvs_pwrkey - only IRQ

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f3c4f7791c86235683541a3d51ed02631b784bf6.camel@andred.net>
Date:   Wed, 26 Feb 2020 17:23:21 +0000
From:   André Draszik <git@...red.net>
To:     Robin Gong <yibin.gong@....com>
Cc:     Horia Geanta <horia.geanta@....com>,
        Aymen Sghaier <aymen.sghaier@....com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        dl-linux-imx <linux-imx@....com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Anson Huang <anson.huang@....com>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 6/6] Input: snvs_pwrkey - only IRQ_HANDLED for our
 own events

On Wed, 2020-02-26 at 01:15 +0000, Robin Gong wrote:
> On 2020/02/26 André Draszik <git@...red.net> wrote: 
> > The snvs_pwrkey shares the SNVS LPSR status register with the snvs_rtc.
> > 
> > This driver here should only return IRQ_HANDLED if the status register
> > indicates that the event we're handling in the irq handler was genuinely
> > intended for this driver. Otheriwse the interrupt subsystem will assume the
> > interrupt was handled successfully even though it wasn't at all.
> > 
> > Signed-off-by: André Draszik <git@...red.net>
> > Cc: "Horia Geantă" <horia.geanta@....com>
> > Cc: Aymen Sghaier <aymen.sghaier@....com>
> > Cc: Herbert Xu <herbert@...dor.apana.org.au>
> > Cc: "David S. Miller" <davem@...emloft.net>
> > Cc: Rob Herring <robh+dt@...nel.org>
> > Cc: Mark Rutland <mark.rutland@....com>
> > Cc: Shawn Guo <shawnguo@...nel.org>
> > Cc: Sascha Hauer <s.hauer@...gutronix.de>
> > Cc: Pengutronix Kernel Team <kernel@...gutronix.de>
> > Cc: Fabio Estevam <festevam@...il.com>
> > Cc: NXP Linux Team <linux-imx@....com>
> > Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>
> > Cc: Anson Huang <Anson.Huang@....com>
> > Cc: Robin Gong <yibin.gong@....com>
> > Cc: linux-crypto@...r.kernel.org
> > Cc: devicetree@...r.kernel.org
> > Cc: linux-arm-kernel@...ts.infradead.org
> > Cc: linux-input@...r.kernel.org
> > 
> > ---
> > v2:
> > * no changes
> > ---
> >  drivers/input/keyboard/snvs_pwrkey.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/input/keyboard/snvs_pwrkey.c
> > b/drivers/input/keyboard/snvs_pwrkey.c
> > index 382d2ae82c9b..980867886b34 100644
> > --- a/drivers/input/keyboard/snvs_pwrkey.c
> > +++ b/drivers/input/keyboard/snvs_pwrkey.c
> > @@ -82,7 +82,9 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void
> > *dev_id)
> >  	clk_enable(pdata->clk);
> > 
> >  	regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status);
> > -	if (lp_status & SNVS_LPSR_SPO) {
> > +	lp_status &= SNVS_LPSR_SPO;
> > +
> > +	if (lp_status) {
> >  		if (pdata->minor_rev == 0) {
> >  			/*
> >  			 * The first generation i.MX[6|7] SoCs only send an @@ -98,14
> > +100,14 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void
> > *dev_id)
> >  			mod_timer(&pdata->check_timer,
> >  			          jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> >  		}
> > -	}
> > 
> > -	/* clear SPO status */
> > -	regmap_write(pdata->snvs, SNVS_LPSR_REG, SNVS_LPSR_SPO);
> > +		/* clear SPO status */
> > +		regmap_write(pdata->snvs, SNVS_LPSR_REG, SNVS_LPSR_SPO);
> But irq storm will come in once there is other interrupt triggered as unexpected,
> although I never met it before. Could we drop this patch now? Others are ok for me.

I don't have strong feelings about this patch, but this bit merely changes behaviour to
clear SP0 if SP0 was in fact != 0 in the first place, whereas before SP0 was always
cleared, even if it was == 0 anyway. Seems more logical in my eyes.


> Reviewed-by: Robin Gong <yibin.gong@nxp>
> > +	}
> > 
> >  	clk_disable(pdata->clk);
> > 
> > -	return IRQ_HANDLED;
> > +	return lp_status ? IRQ_HANDLED : IRQ_NONE;

If you're talking about this part, the rtc-snvs driver does the same in its interrupt handler.
In other words, this driver here could prevent the rtc-snvs driver from seeing its events.



Cheers,
Andre'

 
> >  }
> > 
> >  static void imx_snvs_pwrkey_act(void *pdata)
> > --
> > 2.23.0.rc1