Re: [PATCH v2 1/1] iio: imx8qxp-adc: fix irq flood when call imx8qxp_adc_read_raw()

[Jonathan Cameron]

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

> On 02 12月 22 03:23:55, Bough Chen wrote:
> > > -----Original Message-----
> > > From: Frank Li <frank.li@xxxxxxx>
> > > Sent: 2022年12月1日 22:01
> > > To: Bough Chen <haibo.chen@xxxxxxx>
> > > Cc: cai.huoqing@xxxxxxxxx; festevam@xxxxxxxxx; Frank Li <frank.li@xxxxxxx>;
> > > imx@xxxxxxxxxxxxxxx; jic23@xxxxxxxxxx; kernel@xxxxxxxxxxxxxx;
> > > lars@xxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> > > linux-iio@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>;
> > > linux-kernel@xxxxxxxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; shawnguo@xxxxxxxxxx
> > > 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@xxxxxxxxxxxxxxx
> > > 
> > > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>  
> > 
> > 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@xxxxxxx>  
> The same, thanks.
> Reviewed-by: Cai Huoqing <cai.huoqing@xxxxxxxxx>

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  
> >