linux-kernel - Re: [PATCH v2 1/1] iio: imx8qxp-adc: fix irq flood when call imx8qxp_adc_read

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] [day] [month] [year] [list]

Date:   Sun, 4 Dec 2022 15:53:17 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Cai Huoqing <cai.huoqing@...ux.dev>
Cc:     Bough Chen <haibo.chen@....com>, Frank Li <frank.li@....com>,
        "festevam@...il.com" <festevam@...il.com>,
        "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>
Subject: Re: [PATCH v2 1/1] iio: imx8qxp-adc: fix irq flood when call
 imx8qxp_adc_read_raw()

On Fri, 2 Dec 2022 17:05:54 +0800
Cai Huoqing <cai.huoqing@...ux.dev> wrote:

> On 02 12月 22 03:23:55, Bough Chen wrote:
> > > -----Original Message-----
> > > From: Frank Li <frank.li@....com>
> > > Sent: 2022年12月1日 22:01
> > > To: Bough Chen <haibo.chen@....com>
> > > Cc: cai.huoqing@...ux.dev; festevam@...il.com; Frank Li <frank.li@....com>;
> > > imx@...ts.linux.dev; jic23@...nel.org; kernel@...gutronix.de;
> > > lars@...afoo.de; linux-arm-kernel@...ts.infradead.org;
> > > linux-iio@...r.kernel.org; dl-linux-imx <linux-imx@....com>;
> > > linux-kernel@...r.kernel.org; s.hauer@...gutronix.de; shawnguo@...nel.org
> > > Subject: [PATCH v2 1/1] iio: imx8qxp-adc: fix irq flood when call
> > > imx8qxp_adc_read_raw()
> > > 
> > > irq flood happen when run
> > >     cat /sys/bus/iio/devices/iio:device0/in_voltage1_raw
> > > 
> > > imx8qxp_adc_read_raw()
> > > {
> > > 	...
> > > 	enable irq
> > > 	/* adc start */
> > > 	writel(1, adc->regs + IMX8QXP_ADR_ADC_SWTRIG);
> > > 	^^^^ trigger irq flood.
> > > 	wait_for_completion_interruptible_timeout();
> > > 	readl(adc->regs + IMX8QXP_ADR_ADC_RESFIFO);
> > > 	^^^^ clear irq here.
> > > 	...
> > > }
> > > 
> > > There is only FIFO watermark interrupt at this ADC controller.
> > > IRQ line will be assert until software read data from FIFO.
> > > So IRQ flood happen during wait_for_completion_interruptible_timeout().
> > > 
> > > Move FIFO read into irq handle to avoid irq flood.
> > > 
> > > Fixes: 1e23dcaa1a9f ("iio: imx8qxp-adc: Add driver support for NXP IMX8QXP
> > > ADC")
> > > Cc: stable@...r.kernel.org
> > > 
> > > Signed-off-by: Frank Li <Frank.Li@....com>  
> > 
> > Thanks for the quick fix. The total read count in irq handler is fifo_count which is read from register, this is reasonable.
> > 
> > Reviewed-by: Haibo Chen <haibo.chen@....com>  
> The same, thanks.
> Reviewed-by: Cai Huoqing <cai.huoqing@...ux.dev>

I briefly wondered if it would be better to return the newest value,
but given it's not obvious and they will be close together in this case,
I'm fine with this.

Applied to the fixes-togreg branch of iio.git and marked for
stable inclusion.  Note I'm unlikely to send a pull request until after rc1 now
so this will take a few weeks to land upstream.

Thanks,

Jonathan

> > 
> > Best Regards
> > Haibo Chen  
> > > ---
> > > Change from v1 to v2
> > >  - move complete() after read fifo
> > > 
> > > 
> > >  drivers/iio/adc/imx8qxp-adc.c | 11 +++++++++--
> > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/imx8qxp-adc.c b/drivers/iio/adc/imx8qxp-adc.c index
> > > 36777b827165..f5a0fc9e64c5 100644
> > > --- a/drivers/iio/adc/imx8qxp-adc.c
> > > +++ b/drivers/iio/adc/imx8qxp-adc.c
> > > @@ -86,6 +86,8 @@
> > > 
> > >  #define IMX8QXP_ADC_TIMEOUT		msecs_to_jiffies(100)
> > > 
> > > +#define IMX8QXP_ADC_MAX_FIFO_SIZE		16
> > > +
> > >  struct imx8qxp_adc {
> > >  	struct device *dev;
> > >  	void __iomem *regs;
> > > @@ -95,6 +97,7 @@ struct imx8qxp_adc {
> > >  	/* Serialise ADC channel reads */
> > >  	struct mutex lock;
> > >  	struct completion completion;
> > > +	u32 fifo[IMX8QXP_ADC_MAX_FIFO_SIZE];
> > >  };
> > > 
> > >  #define IMX8QXP_ADC_CHAN(_idx) {				\
> > > @@ -238,8 +241,7 @@ static int imx8qxp_adc_read_raw(struct iio_dev
> > > *indio_dev,
> > >  			return ret;
> > >  		}
> > > 
> > > -		*val = FIELD_GET(IMX8QXP_ADC_RESFIFO_VAL_MASK,
> > > -				 readl(adc->regs + IMX8QXP_ADR_ADC_RESFIFO));
> > > +		*val = adc->fifo[0];
> > > 
> > >  		mutex_unlock(&adc->lock);
> > >  		return IIO_VAL_INT;
> > > @@ -265,10 +267,15 @@ static irqreturn_t imx8qxp_adc_isr(int irq, void
> > > *dev_id)  {
> > >  	struct imx8qxp_adc *adc = dev_id;
> > >  	u32 fifo_count;
> > > +	int i;
> > > 
> > >  	fifo_count = FIELD_GET(IMX8QXP_ADC_FCTRL_FCOUNT_MASK,
> > >  			       readl(adc->regs + IMX8QXP_ADR_ADC_FCTRL));
> > > 
> > > +	for (i = 0; i < fifo_count; i++)
> > > +		adc->fifo[i] = FIELD_GET(IMX8QXP_ADC_RESFIFO_VAL_MASK,
> > > +				readl_relaxed(adc->regs + IMX8QXP_ADR_ADC_RESFIFO));
> > > +
> > >  	if (fifo_count)
> > >  		complete(&adc->completion);
> > > 
> > > --
> > > 2.34.1  
> >