Re: [PATCH] iio: adc: at91: disable adc channel interrupt in timeout case

[Ludovic Desroches]

On Mon, Feb 04, 2019 at 11:03:18AM +0000, Georg Ottinger wrote:
> I don't know how the race condition is triggered in detail.  All I know is that if Touch Acquisition is enabled the adc_demo_error2 will provoke a systemhang. If Touch is disabled the issue goes away. The architecture of at91_adc.c uses (at least) two trigger mechanisms of the ADC peripheral: Touch Trigger and Channel Trigger (started by at91_adc_read_raw() ). In my theory the following happens:

I think you trigger this race condition because when a touchscreen
conversion is done, it performs a conversion on *all active channels*.

Regards

Ludovic

> 
> 1.) at91_adc_read_raw is called, a ADC conversation gets started
> 2.) Touch trigger initiates a new conversation (killing the first conversation)
> 
> .... the conversation started from  at91_adc_read_raw() never finishes -> TIMEOUT!
> 
> The following Email is a summary of our findings (I mailed to the microchip people):
> 
> -----
> 
> I want to let you know our findings of our ADC bughunting marathon. We put around 100 hours of work to identify this issues and I think we found 4  of them (one of them is more hardware related). Although we could develop workarounds for this issues - not all of them are fully understood - so if possible including someone experienced with the ADC peripheral (maybe from the hardware team) should be considered.
> 
> Attached you will find our version of at91_adc.c together with a simple error demonstrator program called adc_demo_error.c
> 
> * 1st issue: Unwanted PEN/NOPEN Interrupts at low temperatures
> 
> When in Pendetect mode we received a lot of unwanted interrupts. This issue only appeared in our test settings when our board approached -20 degrees Celsius. We suspect that the internal Pull-down Resistor connected before the Pendetect-Schmitt trigger (selected by PENDETSENS), has a negative temperature coefficient and at low temperature the impedance might be too high and so false detects are triggered by capturing surrounding noise -> we could solve this issue by changing the value  ts_pen_detect_sensitivity = 2 to ts_pen_detect_sensitivity = 3.
> 
> This issue can be considered to be related to our specific configuration, nevertheless we want to mention that the current Datasheet States on Page 1711 states the following:
> 
> "PENDETSENS: Pen Detection Sensitivity-Modifies the pen detection input pull-up resistor value. See the section 'Electrical Characteristics' for further details."
> 
> There are two issues with this description. First: This resistor is a pull-down resistor (measured and logical). Second the 'Electrical Characteristics' sections is lacking the detailed description of this resistor value.
> 
> * 2nd issue: at91_adc_read_raw() keeps Channel and corresponding interrupt enabled in case of timeout.
> 
> Having a brief look at at91_adc_read_raw() it is obvious that in the case of a timeout the setting of AT91_ADC_CHDR and AT91_ADC_IDR registers is omitted.
> If 2 different channels are queried  we can end up with a situation where two Interrupts are enabled - but only one Interrupt is cleared in the Interrupt handler. Resulting in a interrupt loop and a system hang.
> 
> This Issue can be reproduced by compiling and running the error demonstration with two channel support. (-D2ND_CHANNEL)
> 
> $ arm-buildroot-linux-gnueabihf-gcc adc_demo_error.c -D2ND_CHANNEL -o adc_demo_error2
> 
> * 3rd issue: Race condition between PEN/NOPEN Interrupts using periodic trigger and ADC_START in at91_adc_read_raw()
> 
> This issue represents one way how timeout situations are provoked. Actually we consider the architecture of at91_adc.c a bit risky because it is using two trigger sources (periodic trigger for Touch, and ADC_START for reading raw values). We believe that changing the trigger source during an ongoing conversion can have unwanted side effects, like destroying an ongoing conversion. We currently don't fully understand what is going on, but we developed a workaround that is enhancing the timeout handling in at91_adc_read_raw() with a simple retry mechanism. (restarting with ADC_START)
> 
> * 4th issue: when touch operation is enabled (TSMODE) a delay time between a channel disable (AT91_ADC_CHDR) and channel enable (AT91_ADC_CHER) is needed otherwise the following conversion will fail.
> 
> This issue represents the second way how timeout situations can arise. We measured around 40 to 42 ADC Cycles (at different ADC Clocks 500khz, 1mhz, 2mhz, 3mhz, 5mhz) of delay needed between the channel disable and the channel enable. At higher adc clock frequency (10mhz, 20 mhz) the issue doesn't show up (most likely because the function calls are delaying already enough). We also observed that the need delay time increases further according to the transfer time settings (TRANSFER) by 0, 2, 4 or 8  ADC cycles.
> 
> This Issue can be reproduced by compiling and running the error demonstrator (single channel) and including a simple error message in the kernel module in the timeout case.
> 
> $ arm-buildroot-linux-gnueabihf-gcc adc_demo_error.c -o adc_demo_error
> 
> This issue is least understood by us and I think that talking to the hardware people regarding this issue can be worthwhile. 
> As a workaround we put at91_adc_read_raw() to sleep after the writing AT91_ADC_CHDR for 50 to 75 ADC Cycles. 
> 
> ------------------------------------
> 
> We will run another round of tests next week - and after that I would like to prepare a pull request concerning the 2nd Issue. For the 3rd and the 4th Issue I would like to hear your opinion if our modifications are sound before I will write an pull request.
> 
> Thanks for your time,
> Best wishes,
> Georg Ottinger
> 
> -----
> 
> -----Ursprüngliche Nachricht-----
> Von: Jonathan Cameron [mailto:jonathan.cameron@xxxxxxxxxx] 
> Gesendet: Montag, 04. Februar 2019 10:46
> An: Georg Ottinger <g.ottinger@xxxxxxxxx>
> Cc: Jonathan Cameron <jic23@xxxxxxxxxx>; eugen.hristev@xxxxxxxxxxxxx; Stefan Etzlstorfer <s.etzlstorfer@xxxxxxxxx>; Hartmut Knaack <knaack.h@xxxxxx>; Lars-Peter Clausen <lars@xxxxxxxxxx>; Peter Meerwald-Stadler <pmeerw@xxxxxxxxxx>; Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx>; Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>; Ludovic Desroches <ludovic.desroches@xxxxxxxxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>; Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>; Kees Cook <keescook@xxxxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Maxime Ripard <maxime.ripard@xxxxxxxxxxx>
> Betreff: Re: [PATCH] iio: adc: at91: disable adc channel interrupt in timeout case
> 
> On Mon, 4 Feb 2019 07:17:07 +0000
> Georg Ottinger <g.ottinger@xxxxxxxxx> wrote:
> 
> > Actually this issue occurred to us with an concrete product, where we experienced a system hang at -20 °C.
> > It was triggered by a race condition between the Touch Trigger and the Channel Trigger of the ADC. Once triggered we got in to the situation where an ongoing Channel Conversion was lost (Timeout case).When we queried a second channel than we got a system hang. Investigating this issue we developed an error demonstrator - reading alternating two channels as fast as possible (when Touch is enabled). This also provokes this issue at room temperature.
> > 
> > For the error demonstrator use following commandline to compile:
> > 
> > $ arm-buildroot-linux-gnueabihf-gcc adc_demo_error.c -D2ND_CHANNEL -o 
> > adc_demo_error2
> > 
> > -------------
> > // adc_demo_error.c
> > #include <unistd.h>
> > #include <fcntl.h>
> > 
> > #define VLEN 10
> > 
> > int main()
> > {
> >   int fd_adc,fd_adc2;
> >   int ret;
> >   char dummy[VLEN];
> >   
> >   fd_adc = open 
> > ("/sys/devices/platform/ahb/ahb:apb/f8018000.adc/iio:device0/in_voltag
> > e4_raw",O_RDONLY);
> > #ifdef 2ND_CHANNEL
> >   fd_adc2 = open 
> > ("/sys/devices/platform/ahb/ahb:apb/f8018000.adc/iio:device0/in_voltag
> > e5_raw",O_RDONLY);
> > #endif
> > 
> >   while(1) {
> > 
> >     lseek(fd_adc, 0, SEEK_SET);
> >     ret = read(fd_adc, dummy, VLEN);
> > #ifdef 2ND_CHANNEL
> >     lseek(fd_adc2, 0, SEEK_SET);
> >     ret = read(fd_adc2, dummy, VLEN);
> > #endif
> > 
> >   }
> > }
> > 
> > ------------
> > 
> > 
> > Greeting, Georg
> Hi Georg,
> 
> Thanks for the detailed error report and reproducer.
> 
> What has me wondering now is where the race is that is triggering this?
> Could you talk us through it?  I don't know this driver that well so probably much quicker for you to fill in the gaps rather than me try to figure out the race path!
> 
> I have no problem with defense in depth (with appropriate comments) but I'd like to close that down as well.  If it really is unsolvable I'd like at least to have some clear comments in the code explaining why.
> 
> Thanks,
> 
> Jonathan
> 
> > 
> > -----Ursprüngliche Nachricht-----
> > Von: Jonathan Cameron [mailto:jic23@xxxxxxxxxx]
> > Gesendet: Samstag, 02. Februar 2019 11:21
> > An: Georg Ottinger <g.ottinger@xxxxxxxxx>
> > Cc: eugen.hristev@xxxxxxxxxxxxx; Stefan Etzlstorfer 
> > <s.etzlstorfer@xxxxxxxxx>; Hartmut Knaack <knaack.h@xxxxxx>; 
> > Lars-Peter Clausen <lars@xxxxxxxxxx>; Peter Meerwald-Stadler 
> > <pmeerw@xxxxxxxxxx>; Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx>; 
> > Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>; Ludovic Desroches 
> > <ludovic.desroches@xxxxxxxxxxxxx>; David S. Miller 
> > <davem@xxxxxxxxxxxxx>; Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>; 
> > Kees Cook <keescook@xxxxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx; 
> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; 
> > Maxime Ripard <maxime.ripard@xxxxxxxxxxx>
> > Betreff: Re: [PATCH] iio: adc: at91: disable adc channel interrupt in 
> > timeout case
> > 
> > On Wed, 30 Jan 2019 14:42:02 +0100
> > <g.ottinger@xxxxxxxxx> wrote:
> > 
> > > From: Georg Ottinger <g.ottinger@xxxxxxxxx>
> > > 
> > > Having a brief look at at91_adc_read_raw() it is obvious that in the 
> > > case of a timeout the setting of AT91_ADC_CHDR and AT91_ADC_IDR 
> > > registers is omitted. If 2 different channels are queried we can end 
> > > up with a situation where two interrupts are enabled, but only one 
> > > interrupt is cleared in the interrupt handler. Resulting in a 
> > > interrupt loop and a system hang.
> > > 
> > > Signed-off-by: Georg Ottinger <g.ottinger@xxxxxxxxx>
> > 
> > Whilst I agree this looks like a correct change, I would like Maxime to take a look as he is obviously much more familiar with the driver than I am.
> > 
> > I suspect you can only actually get there in the event of a hardware failure as that isn't actually a timeout you should ever see.
> > Could be wrong though!
> > 
> > Thanks,
> > 
> > Jonathan
> > 
> > > ---
> > >  drivers/iio/adc/at91_adc.c | 28 +++++++++++++++++-----------
> > >  1 file changed, 17 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c 
> > > index 75d2f7358..596841a3c 100644
> > > --- a/drivers/iio/adc/at91_adc.c
> > > +++ b/drivers/iio/adc/at91_adc.c
> > > @@ -704,23 +704,29 @@ static int at91_adc_read_raw(struct iio_dev *idev,
> > >  		ret = wait_event_interruptible_timeout(st->wq_data_avail,
> > >  						       st->done,
> > >  						       msecs_to_jiffies(1000));
> > > -		if (ret == 0)
> > > -			ret = -ETIMEDOUT;
> > > -		if (ret < 0) {
> > > -			mutex_unlock(&st->lock);
> > > -			return ret;
> > > -		}
> > > -
> > > -		*val = st->last_value;
> > >  
> > > +		/* Disable interrupts, regardless if adc conversion was
> > > +		 * successful or not
> > > +		 */
> > >  		at91_adc_writel(st, AT91_ADC_CHDR,
> > >  				AT91_ADC_CH(chan->channel));
> > >  		at91_adc_writel(st, AT91_ADC_IDR, BIT(chan->channel));
> > >  
> > > -		st->last_value = 0;
> > > -		st->done = false;
> > > +		if (ret > 0) {
> > > +			/* a valid conversion took place */
> > > +			*val = st->last_value;
> > > +			st->last_value = 0;
> > > +			st->done = false;
> > > +			ret = IIO_VAL_INT;
> > > +		} else if (ret == 0) {
> > > +			/* conversion timeout */
> > > +			dev_err(&idev->dev, "ADC Channel %d timeout.\n",
> > > +				chan->channel);
> > > +			ret = -ETIMEDOUT;
> > > +		}
> > > +
> > >  		mutex_unlock(&st->lock);
> > > -		return IIO_VAL_INT;
> > > +		return ret;
> > >  
> > >  	case IIO_CHAN_INFO_SCALE:
> > >  		*val = st->vref_mv;
> > 
> 
>