[PATCH 1/2] iio: adc: at91: fix acking DRDY irq on simple conversions

[PATCH 1/2] iio: adc: at91: fix acking DRDY irq on simple conversions

[Eugen Hristev]

When doing simple conversions, the driver did not acknowledge the DRDY irq.
If this irq is not acked, it will be left pending, and as soon as a trigger
is enabled, the irq handler will be called, it doesn't know why this irq
has occurred because no channel is pending, and then we will have irq loop
and board will hang.

Fixes 0e589d5fb ("ARM: AT91: IIO: Add AT91 ADC driver.")
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/iio/adc/at91_adc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index 44b5168..e85f859 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -712,6 +712,11 @@ static int at91_adc_read_raw(struct iio_dev *idev,
 		at91_adc_writel(st, AT91_ADC_CHDR,
 				AT91_ADC_CH(chan->channel));
 		at91_adc_writel(st, AT91_ADC_IDR, BIT(chan->channel));
+		/*
+		 * we need to ack the DRDY irq, otherwise it will be
+		 * left pending and irq handler will be confused
+		 */
+		at91_adc_readl(st, AT91_ADC_LCDR);
 
 		st->last_value = 0;
 		st->done = false;
-- 
2.7.4

[PATCH 2/2] iio: adc: at91: fix wrong channel number in triggered buffer mode

[Eugen Hristev]

When channels are registered, the hardware channel number is not the
actual iio channel number.
This is because the driver is probed with a certain number of accessible
channels. Some pins are routed and some not, depending on the description of
the board in the DT.
Because of that, channels 0,1,2,3 can correspond to hardware channels
2,3,4,5 for example.
In the buffered triggered case, we need to do the translation accordingly.
Fixed the channel number to stop reading the wrong channel.

Fixes 0e589d5fb "ARM: AT91: IIO: Add AT91 ADC driver."
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/iio/adc/at91_adc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index e85f859..6698804 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -248,12 +248,14 @@ static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *idev = pf->indio_dev;
 	struct at91_adc_state *st = iio_priv(idev);
+	struct iio_chan_spec *chan;
 	int i, j = 0;
 
 	for (i = 0; i < idev->masklength; i++) {
 		if (!test_bit(i, idev->active_scan_mask))
 			continue;
-		st->buffer[j] = at91_adc_readl(st, AT91_ADC_CHAN(st, i));
+		chan = idev->channels + i;
+		st->buffer[j] = at91_adc_readl(st, AT91_ADC_CHAN(st, chan->channel));
 		j++;
 	}
 
-- 
2.7.4

Re: [PATCH 2/2] iio: adc: at91: fix wrong channel number in triggered buffer mode

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2198 bytes --]

Hi Eugen,

I love your patch! Perhaps something to improve:

[auto build test WARNING on iio/togreg]
[also build test WARNING on v4.19-rc4 next-20180919]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Eugen-Hristev/iio-adc-at91-fix-acking-DRDY-irq-on-simple-conversions/20180920-215908
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=arm 

All warnings (new ones prefixed by >>):

   drivers/iio/adc/at91_adc.c: In function 'at91_adc_trigger_handler':
>> drivers/iio/adc/at91_adc.c:257:8: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
      chan = idev->channels + i;
           ^

vim +/const +257 drivers/iio/adc/at91_adc.c

   245	
   246	static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
   247	{
   248		struct iio_poll_func *pf = p;
   249		struct iio_dev *idev = pf->indio_dev;
   250		struct at91_adc_state *st = iio_priv(idev);
   251		struct iio_chan_spec *chan;
   252		int i, j = 0;
   253	
   254		for (i = 0; i < idev->masklength; i++) {
   255			if (!test_bit(i, idev->active_scan_mask))
   256				continue;
 > 257			chan = idev->channels + i;
   258			st->buffer[j] = at91_adc_readl(st, AT91_ADC_CHAN(st, chan->channel));
   259			j++;
   260		}
   261	
   262		iio_push_to_buffers_with_timestamp(idev, st->buffer, pf->timestamp);
   263	
   264		iio_trigger_notify_done(idev->trig);
   265	
   266		/* Needed to ACK the DRDY interruption */
   267		at91_adc_readl(st, AT91_ADC_LCDR);
   268	
   269		enable_irq(st->irq);
   270	
   271		return IRQ_HANDLED;
   272	}
   273	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 67150 bytes --]

Re: [PATCH 2/2] iio: adc: at91: fix wrong channel number in triggered buffer mode

[Jonathan Cameron]

On Fri, 21 Sep 2018 15:26:03 +0800
kbuild test robot <lkp@intel.com> wrote:

> Hi Eugen,
This one is leaving me stumped...

Anyone care to point out what I'm missing that is wrong here?

Also Eugen, please don't cc stable on a patch directly.  It is fine to send
a backport request once a patch has hit mainline, but before that it's just
adding noise to a list as they won't take it directly anyway.

Jonathan

> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on iio/togreg]
> [also build test WARNING on v4.19-rc4 next-20180919]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Eugen-Hristev/iio-adc-at91-fix-acking-DRDY-irq-on-simple-conversions/20180920-215908
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> config: arm-allmodconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         GCC_VERSION=7.2.0 make.cross ARCH=arm 
> 
> All warnings (new ones prefixed by >>):
> 
>    drivers/iio/adc/at91_adc.c: In function 'at91_adc_trigger_handler':
> >> drivers/iio/adc/at91_adc.c:257:8: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]  
>       chan = idev->channels + i;
>            ^
> 
> vim +/const +257 drivers/iio/adc/at91_adc.c
> 
>    245	
>    246	static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
>    247	{
>    248		struct iio_poll_func *pf = p;
>    249		struct iio_dev *idev = pf->indio_dev;
>    250		struct at91_adc_state *st = iio_priv(idev);
>    251		struct iio_chan_spec *chan;
>    252		int i, j = 0;
>    253	
>    254		for (i = 0; i < idev->masklength; i++) {
>    255			if (!test_bit(i, idev->active_scan_mask))
>    256				continue;
>  > 257			chan = idev->channels + i;  
>    258			st->buffer[j] = at91_adc_readl(st, AT91_ADC_CHAN(st, chan->channel));
>    259			j++;
>    260		}
>    261	
>    262		iio_push_to_buffers_with_timestamp(idev, st->buffer, pf->timestamp);
>    263	
>    264		iio_trigger_notify_done(idev->trig);
>    265	
>    266		/* Needed to ACK the DRDY interruption */
>    267		at91_adc_readl(st, AT91_ADC_LCDR);
>    268	
>    269		enable_irq(st->irq);
>    270	
>    271		return IRQ_HANDLED;
>    272	}
>    273	
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 2/2] iio: adc: at91: fix wrong channel number in triggered buffer mode

[Song Qiang]

On Sat, Sep 22, 2018 at 11:36:16AM +0100, Jonathan Cameron wrote:
> On Fri, 21 Sep 2018 15:26:03 +0800
> kbuild test robot <lkp@intel.com> wrote:
> 
> > Hi Eugen,
> This one is leaving me stumped...
> 
> Anyone care to point out what I'm missing that is wrong here?
> 
> Also Eugen, please don't cc stable on a patch directly.  It is fine to send
> a backport request once a patch has hit mainline, but before that it's just
> adding noise to a list as they won't take it directly anyway.
> 
> Jonathan
> 

Hi Jonathan,

I run some test code here and found out that C compilers don't allow
us to assign a 'const int *' to 'int *', while assign a 'const int' to
'int' is fine.
In this case this driver was tring to assign a 'struct iio_chan_spec
const *' to a 'struct iio_chan_spec *', mostly because of this issue.

I googled this issue and someone on stack overflow said it's because
this kind of action will break the check mechanism of 'const'.

yours,
Song Qiang

> > 
> > I love your patch! Perhaps something to improve:
> > 
> > [auto build test WARNING on iio/togreg]
> > [also build test WARNING on v4.19-rc4 next-20180919]
> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> > 
> > url:    https://github.com/0day-ci/linux/commits/Eugen-Hristev/iio-adc-at91-fix-acking-DRDY-irq-on-simple-conversions/20180920-215908
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> > config: arm-allmodconfig (attached as .config)
> > compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
> > reproduce:
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # save the attached .config to linux build tree
> >         GCC_VERSION=7.2.0 make.cross ARCH=arm 
> > 
> > All warnings (new ones prefixed by >>):
> > 
> >    drivers/iio/adc/at91_adc.c: In function 'at91_adc_trigger_handler':
> > >> drivers/iio/adc/at91_adc.c:257:8: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]  
> >       chan = idev->channels + i;
> >            ^
> > 
> > vim +/const +257 drivers/iio/adc/at91_adc.c
> > 
> >    245	
> >    246	static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
> >    247	{
> >    248		struct iio_poll_func *pf = p;
> >    249		struct iio_dev *idev = pf->indio_dev;
> >    250		struct at91_adc_state *st = iio_priv(idev);
> >    251		struct iio_chan_spec *chan;
> >    252		int i, j = 0;
> >    253	
> >    254		for (i = 0; i < idev->masklength; i++) {
> >    255			if (!test_bit(i, idev->active_scan_mask))
> >    256				continue;
> >  > 257			chan = idev->channels + i;  
> >    258			st->buffer[j] = at91_adc_readl(st, AT91_ADC_CHAN(st, chan->channel));
> >    259			j++;
> >    260		}
> >    261	
> >    262		iio_push_to_buffers_with_timestamp(idev, st->buffer, pf->timestamp);
> >    263	
> >    264		iio_trigger_notify_done(idev->trig);
> >    265	
> >    266		/* Needed to ACK the DRDY interruption */
> >    267		at91_adc_readl(st, AT91_ADC_LCDR);
> >    268	
> >    269		enable_irq(st->irq);
> >    270	
> >    271		return IRQ_HANDLED;
> >    272	}
> >    273	
> > 
> > ---
> > 0-DAY kernel test infrastructure                Open Source Technology Center
> > https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> 

Re: [PATCH 2/2] iio: adc: at91: fix wrong channel number in triggered buffer mode

[Himanshu Jha]

On Sat, Sep 22, 2018 at 11:36:16AM +0100, Jonathan Cameron wrote:
> On Fri, 21 Sep 2018 15:26:03 +0800
> kbuild test robot <lkp@intel.com> wrote:
> 
> > Hi Eugen,
> This one is leaving me stumped...
> 
> Anyone care to point out what I'm missing that is wrong here?
> 
> Also Eugen, please don't cc stable on a patch directly.  It is fine to send
> a backport request once a patch has hit mainline, but before that it's just
> adding noise to a list as they won't take it directly anyway.

Song is correct on this one.

So, just replace the declaration to:

	struct iio_chan_spec const *chan;

and then warning goes away...

Another thing, its better not to reply the automated bot and instead to
the author by changing "From" address. :)

That is what I do often when replying to that thread.

https://lore.kernel.org/lkml/20180921182616.GA2077@himanshu-Vostro-3559/

And it nests properly in the given thread avoiding any noise to kbuild
bot.


-- 
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology

Re: [PATCH 2/2] iio: adc: at91: fix wrong channel number in triggered buffer mode

[Jonathan Cameron]

On 22 September 2018 12:48:49 BST, Himanshu Jha <himanshujha199640@gmail.com> wrote:
>On Sat, Sep 22, 2018 at 11:36:16AM +0100, Jonathan Cameron wrote:
>> On Fri, 21 Sep 2018 15:26:03 +0800
>> kbuild test robot <lkp@intel.com> wrote:
>> 
>> > Hi Eugen,
>> This one is leaving me stumped...
>> 
>> Anyone care to point out what I'm missing that is wrong here?
>> 
>> Also Eugen, please don't cc stable on a patch directly.  It is fine
>to send
>> a backport request once a patch has hit mainline, but before that
>it's just
>> adding noise to a list as they won't take it directly anyway.
>
>Song is correct on this one.
>
>So, just replace the declaration to:
>
>	struct iio_chan_spec const *chan;
>
>and then warning goes away...

Good point. Not enough coffee this morning..

Obvious now :)
>
>Another thing, its better not to reply the automated bot and instead to
>the author by changing "From" address. :)
>
>That is what I do often when replying to that thread.
>
>https://lore.kernel.org/lkml/20180921182616.GA2077@himanshu-Vostro-3559/
>
>And it nests properly in the given thread avoiding any noise to kbuild
>bot.



-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 1/2] iio: adc: at91: fix acking DRDY irq on simple conversions

[Jonathan Cameron]

On Thu, 20 Sep 2018 15:40:37 +0300
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> When doing simple conversions, the driver did not acknowledge the DRDY irq.
> If this irq is not acked, it will be left pending, and as soon as a trigger
> is enabled, the irq handler will be called, it doesn't know why this irq
> has occurred because no channel is pending, and then we will have irq loop
> and board will hang.
> 
> Fixes 0e589d5fb ("ARM: AT91: IIO: Add AT91 ADC driver.")
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>  drivers/iio/adc/at91_adc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> index 44b5168..e85f859 100644
> --- a/drivers/iio/adc/at91_adc.c
> +++ b/drivers/iio/adc/at91_adc.c
> @@ -712,6 +712,11 @@ static int at91_adc_read_raw(struct iio_dev *idev,
>  		at91_adc_writel(st, AT91_ADC_CHDR,
>  				AT91_ADC_CH(chan->channel));
>  		at91_adc_writel(st, AT91_ADC_IDR, BIT(chan->channel));
> +		/*
> +		 * we need to ack the DRDY irq, otherwise it will be
> +		 * left pending and irq handler will be confused
> +		 */
> +		at91_adc_readl(st, AT91_ADC_LCDR);

I'm curious as to how things were working before.  Does this only occur
if we do a raw_read whilst the buffer is enabled?

I would have assumed when it's not enabled, the irq would be masked and
never generated in the first place...

It may be that what we actually need to do is to prevent read_raw accesses
when the buffer is enabled (like lots of other drivers do precisely to
avoid this sort of condition). The problem there comes if we have
existing applications doing this combination as we are then breaking
userspace. If that's the case we'll need to be a bit more clever.

Hammering down an irq state in a non irq handling path isn't a good
solution.

Jonathan

>  
>  		st->last_value = 0;
>  		st->done = false;

Re: [PATCH 1/2] iio: adc: at91: fix acking DRDY irq on simple conversions

[Eugen Hristev]

On 22.09.2018 13:31, Jonathan Cameron wrote:
> On Thu, 20 Sep 2018 15:40:37 +0300
> Eugen Hristev <eugen.hristev@microchip.com> wrote:
> 
>> When doing simple conversions, the driver did not acknowledge the DRDY irq.
>> If this irq is not acked, it will be left pending, and as soon as a trigger
>> is enabled, the irq handler will be called, it doesn't know why this irq
>> has occurred because no channel is pending, and then we will have irq loop
>> and board will hang.
>>
>> Fixes 0e589d5fb ("ARM: AT91: IIO: Add AT91 ADC driver.")
>> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>> ---
>>   drivers/iio/adc/at91_adc.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
>> index 44b5168..e85f859 100644
>> --- a/drivers/iio/adc/at91_adc.c
>> +++ b/drivers/iio/adc/at91_adc.c
>> @@ -712,6 +712,11 @@ static int at91_adc_read_raw(struct iio_dev *idev,
>>   		at91_adc_writel(st, AT91_ADC_CHDR,
>>   				AT91_ADC_CH(chan->channel));
>>   		at91_adc_writel(st, AT91_ADC_IDR, BIT(chan->channel));
>> +		/*
>> +		 * we need to ack the DRDY irq, otherwise it will be
>> +		 * left pending and irq handler will be confused
>> +		 */
>> +		at91_adc_readl(st, AT91_ADC_LCDR);
> 
> I'm curious as to how things were working before.  Does this only occur
> if we do a raw_read whilst the buffer is enabled?

No. The situation is that the read raw does not properly cleans itself 
up after it's done.
The DRDY bit is cleared only when LCDR (last converted data ) is being read.
Even if we read the per channel conversion data, the LCDR still needs to 
be read to clear this irq status.
The driver does not use the DRDY irq but this irq status is still being 
set in the status register.

> 
> I would have assumed when it's not enabled, the irq would be masked and
> never generated in the first place...
> 
> It may be that what we actually need to do is to prevent read_raw accesses
> when the buffer is enabled (like lots of other drivers do precisely to
> avoid this sort of condition). The problem there comes if we have
> existing applications doing this combination as we are then breaking
> userspace. If that's the case we'll need to be a bit more clever.
> 
> Hammering down an irq state in a non irq handling path isn't a good
> solution.

Ok, I will move the clearing of the DRDY (LCDR read) in the irq path 
then, and send a v2.


> 
> Jonathan
> 
>>   
>>   		st->last_value = 0;
>>   		st->done = false;
> 

Re: [PATCH 1/2] iio: adc: at91: fix acking DRDY irq on simple conversions

[Jonathan Cameron]

On Mon, 24 Sep 2018 09:19:43 +0300
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> On 22.09.2018 13:31, Jonathan Cameron wrote:
> > On Thu, 20 Sep 2018 15:40:37 +0300
> > Eugen Hristev <eugen.hristev@microchip.com> wrote:
> >   
> >> When doing simple conversions, the driver did not acknowledge the DRDY irq.
> >> If this irq is not acked, it will be left pending, and as soon as a trigger
> >> is enabled, the irq handler will be called, it doesn't know why this irq
> >> has occurred because no channel is pending, and then we will have irq loop
> >> and board will hang.
> >>
> >> Fixes 0e589d5fb ("ARM: AT91: IIO: Add AT91 ADC driver.")
> >> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> >> Cc: <stable@vger.kernel.org>
> >> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> >> ---
> >>   drivers/iio/adc/at91_adc.c | 5 +++++
> >>   1 file changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> >> index 44b5168..e85f859 100644
> >> --- a/drivers/iio/adc/at91_adc.c
> >> +++ b/drivers/iio/adc/at91_adc.c
> >> @@ -712,6 +712,11 @@ static int at91_adc_read_raw(struct iio_dev *idev,
> >>   		at91_adc_writel(st, AT91_ADC_CHDR,
> >>   				AT91_ADC_CH(chan->channel));
> >>   		at91_adc_writel(st, AT91_ADC_IDR, BIT(chan->channel));
> >> +		/*
> >> +		 * we need to ack the DRDY irq, otherwise it will be
> >> +		 * left pending and irq handler will be confused
> >> +		 */
> >> +		at91_adc_readl(st, AT91_ADC_LCDR);  
> > 
> > I'm curious as to how things were working before.  Does this only occur
> > if we do a raw_read whilst the buffer is enabled?  
> 
> No. The situation is that the read raw does not properly cleans itself 
> up after it's done.
> The DRDY bit is cleared only when LCDR (last converted data ) is being read.
> Even if we read the per channel conversion data, the LCDR still needs to 
> be read to clear this irq status.
> The driver does not use the DRDY irq but this irq status is still being 
> set in the status register.

Hmm. That is somewhat nasty if it results in false interrupts when you
then enable them.

> 
> > 
> > I would have assumed when it's not enabled, the irq would be masked and
> > never generated in the first place...
> > 
> > It may be that what we actually need to do is to prevent read_raw accesses
> > when the buffer is enabled (like lots of other drivers do precisely to
> > avoid this sort of condition). The problem there comes if we have
> > existing applications doing this combination as we are then breaking
> > userspace. If that's the case we'll need to be a bit more clever.
> > 
> > Hammering down an irq state in a non irq handling path isn't a good
> > solution.  
> 
> Ok, I will move the clearing of the DRDY (LCDR read) in the irq path 
> then, and send a v2.

If that can be done cleanly, let us go with that approach. If not
what you have here with the addition of a comments saying that the
interrupt status is not masked, merely the interrupt and as a result needs
clearing for when you later enabled the interrupt, is fine.

This definitely sounds like one of those bits of hardware that
you can write software to use safely but they certainly didn't make
it easy to do so!

Jonathan
> 
> 
> > 
> > Jonathan
> >   
> >>   
> >>   		st->last_value = 0;
> >>   		st->done = false;  
> >   

Re: [PATCH 1/2] iio: adc: at91: fix acking DRDY irq on simple conversions

[Jonathan Cameron]

On Mon, 24 Sep 2018 21:00:56 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 24 Sep 2018 09:19:43 +0300
> Eugen Hristev <eugen.hristev@microchip.com> wrote:
> 
> > On 22.09.2018 13:31, Jonathan Cameron wrote:  
> > > On Thu, 20 Sep 2018 15:40:37 +0300
> > > Eugen Hristev <eugen.hristev@microchip.com> wrote:
> > >     
> > >> When doing simple conversions, the driver did not acknowledge the DRDY irq.
> > >> If this irq is not acked, it will be left pending, and as soon as a trigger
> > >> is enabled, the irq handler will be called, it doesn't know why this irq
> > >> has occurred because no channel is pending, and then we will have irq loop
> > >> and board will hang.
> > >>
> > >> Fixes 0e589d5fb ("ARM: AT91: IIO: Add AT91 ADC driver.")
> > >> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> > >> Cc: <stable@vger.kernel.org>
> > >> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> > >> ---
> > >>   drivers/iio/adc/at91_adc.c | 5 +++++
> > >>   1 file changed, 5 insertions(+)
> > >>
> > >> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> > >> index 44b5168..e85f859 100644
> > >> --- a/drivers/iio/adc/at91_adc.c
> > >> +++ b/drivers/iio/adc/at91_adc.c
> > >> @@ -712,6 +712,11 @@ static int at91_adc_read_raw(struct iio_dev *idev,
> > >>   		at91_adc_writel(st, AT91_ADC_CHDR,
> > >>   				AT91_ADC_CH(chan->channel));
> > >>   		at91_adc_writel(st, AT91_ADC_IDR, BIT(chan->channel));
> > >> +		/*
> > >> +		 * we need to ack the DRDY irq, otherwise it will be
> > >> +		 * left pending and irq handler will be confused
> > >> +		 */
> > >> +		at91_adc_readl(st, AT91_ADC_LCDR);    
> > > 
> > > I'm curious as to how things were working before.  Does this only occur
> > > if we do a raw_read whilst the buffer is enabled?    
> > 
> > No. The situation is that the read raw does not properly cleans itself 
> > up after it's done.
> > The DRDY bit is cleared only when LCDR (last converted data ) is being read.
> > Even if we read the per channel conversion data, the LCDR still needs to 
> > be read to clear this irq status.
> > The driver does not use the DRDY irq but this irq status is still being 
> > set in the status register.  
> 
> Hmm. That is somewhat nasty if it results in false interrupts when you
> then enable them.

I'm talking rubbish here.  Please ignore!

> 
> >   
> > > 
> > > I would have assumed when it's not enabled, the irq would be masked and
> > > never generated in the first place...
> > > 
> > > It may be that what we actually need to do is to prevent read_raw accesses
> > > when the buffer is enabled (like lots of other drivers do precisely to
> > > avoid this sort of condition). The problem there comes if we have
> > > existing applications doing this combination as we are then breaking
> > > userspace. If that's the case we'll need to be a bit more clever.
> > > 
> > > Hammering down an irq state in a non irq handling path isn't a good
> > > solution.    
> > 
> > Ok, I will move the clearing of the DRDY (LCDR read) in the irq path 
> > then, and send a v2.  
> 
> If that can be done cleanly, let us go with that approach. If not
> what you have here with the addition of a comments saying that the
> interrupt status is not masked, merely the interrupt and as a result needs
> clearing for when you later enabled the interrupt, is fine.
> 
> This definitely sounds like one of those bits of hardware that
> you can write software to use safely but they certainly didn't make
> it easy to do so!
> 
> Jonathan
> > 
> >   
> > > 
> > > Jonathan
> > >     
> > >>   
> > >>   		st->last_value = 0;
> > >>   		st->done = false;    
> > >     
> 

Re: [PATCH 1/2] iio: adc: at91: fix acking DRDY irq on simple conversions

[Eugen Hristev]

On 24.09.2018 23:27, Jonathan Cameron wrote:
> On Mon, 24 Sep 2018 21:00:56 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
>> On Mon, 24 Sep 2018 09:19:43 +0300
>> Eugen Hristev <eugen.hristev@microchip.com> wrote:
>>
>>> On 22.09.2018 13:31, Jonathan Cameron wrote:
>>>> On Thu, 20 Sep 2018 15:40:37 +0300
>>>> Eugen Hristev <eugen.hristev@microchip.com> wrote:
>>>>      
>>>>> When doing simple conversions, the driver did not acknowledge the DRDY irq.
>>>>> If this irq is not acked, it will be left pending, and as soon as a trigger
>>>>> is enabled, the irq handler will be called, it doesn't know why this irq
>>>>> has occurred because no channel is pending, and then we will have irq loop
>>>>> and board will hang.
>>>>>
>>>>> Fixes 0e589d5fb ("ARM: AT91: IIO: Add AT91 ADC driver.")
>>>>> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
>>>>> Cc: <stable@vger.kernel.org>
>>>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>>>> ---
>>>>>    drivers/iio/adc/at91_adc.c | 5 +++++
>>>>>    1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
>>>>> index 44b5168..e85f859 100644
>>>>> --- a/drivers/iio/adc/at91_adc.c
>>>>> +++ b/drivers/iio/adc/at91_adc.c
>>>>> @@ -712,6 +712,11 @@ static int at91_adc_read_raw(struct iio_dev *idev,
>>>>>    		at91_adc_writel(st, AT91_ADC_CHDR,
>>>>>    				AT91_ADC_CH(chan->channel));
>>>>>    		at91_adc_writel(st, AT91_ADC_IDR, BIT(chan->channel));
>>>>> +		/*
>>>>> +		 * we need to ack the DRDY irq, otherwise it will be
>>>>> +		 * left pending and irq handler will be confused
>>>>> +		 */
>>>>> +		at91_adc_readl(st, AT91_ADC_LCDR);
>>>>
>>>> I'm curious as to how things were working before.  Does this only occur
>>>> if we do a raw_read whilst the buffer is enabled?
>>>
>>> No. The situation is that the read raw does not properly cleans itself
>>> up after it's done.
>>> The DRDY bit is cleared only when LCDR (last converted data ) is being read.
>>> Even if we read the per channel conversion data, the LCDR still needs to
>>> be read to clear this irq status.
>>> The driver does not use the DRDY irq but this irq status is still being
>>> set in the status register.
>>
>> Hmm. That is somewhat nasty if it results in false interrupts when you
>> then enable them.
> 
> I'm talking rubbish here.  Please ignore!
> 
>>
>>>    
>>>>
>>>> I would have assumed when it's not enabled, the irq would be masked and
>>>> never generated in the first place...
>>>>
>>>> It may be that what we actually need to do is to prevent read_raw accesses
>>>> when the buffer is enabled (like lots of other drivers do precisely to
>>>> avoid this sort of condition). The problem there comes if we have
>>>> existing applications doing this combination as we are then breaking
>>>> userspace. If that's the case we'll need to be a bit more clever.
>>>>
>>>> Hammering down an irq state in a non irq handling path isn't a good
>>>> solution.
>>>
>>> Ok, I will move the clearing of the DRDY (LCDR read) in the irq path
>>> then, and send a v2.
>>
>> If that can be done cleanly, let us go with that approach. If not
>> what you have here with the addition of a comments saying that the
>> interrupt status is not masked, merely the interrupt and as a result needs
>> clearing for when you later enabled the interrupt, is fine.
>>
>> This definitely sounds like one of those bits of hardware that
>> you can write software to use safely but they certainly didn't make
>> it easy to do so!

For what it's worth a small explanation:

The status in the ISR (interrupt status) is updated regardless of 
whether the IRQ is enabled in the IER (interrupt enable) or not. 
Actually "enabling DRDY" makes the hardware start raising the interrupt 
line on DRDY event if it's the case. but DRDY bit is still being 
asserted in the ISR. It works like this because someone might want to 
write some simple driver without IRQ, just poll the ISR for the status.

One other thing, in my driver, at91-sama5d2_adc, I had the same issue, 
and I am reading the DRDY in the EOC part after waking up from the wait. 
This makes the IRQ handler shorter by a few hundred cycles : read LCDR 
in the userspace-called read_raw.
Do you want me to have a patch at some point to move the LCDR read in 
the IRQ handler for at91-sama5d2_adc as well ?

Eugen

>>
>> Jonathan
>>>
>>>    
>>>>
>>>> Jonathan
>>>>      
>>>>>    
>>>>>    		st->last_value = 0;
>>>>>    		st->done = false;
>>>>      
>>
> 
> 

Re: [PATCH 1/2] iio: adc: at91: fix acking DRDY irq on simple conversions

[Jonathan Cameron]

On Tue, 25 Sep 2018 09:50:24 +0300
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> On 24.09.2018 23:27, Jonathan Cameron wrote:
> > On Mon, 24 Sep 2018 21:00:56 +0100
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >   
> >> On Mon, 24 Sep 2018 09:19:43 +0300
> >> Eugen Hristev <eugen.hristev@microchip.com> wrote:
> >>  
> >>> On 22.09.2018 13:31, Jonathan Cameron wrote:  
> >>>> On Thu, 20 Sep 2018 15:40:37 +0300
> >>>> Eugen Hristev <eugen.hristev@microchip.com> wrote:
> >>>>        
> >>>>> When doing simple conversions, the driver did not acknowledge the DRDY irq.
> >>>>> If this irq is not acked, it will be left pending, and as soon as a trigger
> >>>>> is enabled, the irq handler will be called, it doesn't know why this irq
> >>>>> has occurred because no channel is pending, and then we will have irq loop
> >>>>> and board will hang.
> >>>>>
> >>>>> Fixes 0e589d5fb ("ARM: AT91: IIO: Add AT91 ADC driver.")
> >>>>> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> >>>>> Cc: <stable@vger.kernel.org>
> >>>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> >>>>> ---
> >>>>>    drivers/iio/adc/at91_adc.c | 5 +++++
> >>>>>    1 file changed, 5 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> >>>>> index 44b5168..e85f859 100644
> >>>>> --- a/drivers/iio/adc/at91_adc.c
> >>>>> +++ b/drivers/iio/adc/at91_adc.c
> >>>>> @@ -712,6 +712,11 @@ static int at91_adc_read_raw(struct iio_dev *idev,
> >>>>>    		at91_adc_writel(st, AT91_ADC_CHDR,
> >>>>>    				AT91_ADC_CH(chan->channel));
> >>>>>    		at91_adc_writel(st, AT91_ADC_IDR, BIT(chan->channel));
> >>>>> +		/*
> >>>>> +		 * we need to ack the DRDY irq, otherwise it will be
> >>>>> +		 * left pending and irq handler will be confused
> >>>>> +		 */
> >>>>> +		at91_adc_readl(st, AT91_ADC_LCDR);  
> >>>>
> >>>> I'm curious as to how things were working before.  Does this only occur
> >>>> if we do a raw_read whilst the buffer is enabled?  
> >>>
> >>> No. The situation is that the read raw does not properly cleans itself
> >>> up after it's done.
> >>> The DRDY bit is cleared only when LCDR (last converted data ) is being read.
> >>> Even if we read the per channel conversion data, the LCDR still needs to
> >>> be read to clear this irq status.
> >>> The driver does not use the DRDY irq but this irq status is still being
> >>> set in the status register.  
> >>
> >> Hmm. That is somewhat nasty if it results in false interrupts when you
> >> then enable them.  
> > 
> > I'm talking rubbish here.  Please ignore!
> >   
> >>  
> >>>      
> >>>>
> >>>> I would have assumed when it's not enabled, the irq would be masked and
> >>>> never generated in the first place...
> >>>>
> >>>> It may be that what we actually need to do is to prevent read_raw accesses
> >>>> when the buffer is enabled (like lots of other drivers do precisely to
> >>>> avoid this sort of condition). The problem there comes if we have
> >>>> existing applications doing this combination as we are then breaking
> >>>> userspace. If that's the case we'll need to be a bit more clever.
> >>>>
> >>>> Hammering down an irq state in a non irq handling path isn't a good
> >>>> solution.  
> >>>
> >>> Ok, I will move the clearing of the DRDY (LCDR read) in the irq path
> >>> then, and send a v2.  
> >>
> >> If that can be done cleanly, let us go with that approach. If not
> >> what you have here with the addition of a comments saying that the
> >> interrupt status is not masked, merely the interrupt and as a result needs
> >> clearing for when you later enabled the interrupt, is fine.
> >>
> >> This definitely sounds like one of those bits of hardware that
> >> you can write software to use safely but they certainly didn't make
> >> it easy to do so!  
> 
> For what it's worth a small explanation:
> 
> The status in the ISR (interrupt status) is updated regardless of 
> whether the IRQ is enabled in the IER (interrupt enable) or not. 
> Actually "enabling DRDY" makes the hardware start raising the interrupt 
> line on DRDY event if it's the case. but DRDY bit is still being 
> asserted in the ISR. It works like this because someone might want to 
> write some simple driver without IRQ, just poll the ISR for the status.
Hmm.  I think the odd blend here that makes this a little unusual is that
devices normally fall into one of two categories.
1) Auto reset upon complete reading of output registers. This one works
   in either interrupt or polled mode.
2) Interrupt that needs an explicit reset.

Not sure why but it's unusual to need an explicit reset for a status
register. Not sure why but that's now it is ;)

> 
> One other thing, in my driver, at91-sama5d2_adc, I had the same issue, 
> and I am reading the DRDY in the EOC part after waking up from the wait. 
> This makes the IRQ handler shorter by a few hundred cycles : read LCDR 
> in the userspace-called read_raw.
> Do you want me to have a patch at some point to move the LCDR read in 
> the IRQ handler for at91-sama5d2_adc as well ?

I don't know why I convinced myself before that it was semantically odd
to do it in this location.

I suppose I felt that it should be handled the same in the triggered
case (where it is in the interrupt handler - be it very indirectly) and
in the polled case.  If it's well commented I don't think it matters
either way.  In some senses you are just making passing out of the interrupt
into a threaded handler in both paths.

Jonathan

> 
> Eugen
> 
> >>
> >> Jonathan  
> >>>
> >>>      
> >>>>
> >>>> Jonathan
> >>>>        
> >>>>>    
> >>>>>    		st->last_value = 0;
> >>>>>    		st->done = false;  
> >>>>        
> >>  
> > 
> >