linux-kernel - Re: [PATCH v2 8/8] drivers: iio: imu: Add support for adis1657x family

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 for Android: free password hash cracker in your pocket

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

Date: Sun, 19 May 2024 18:43:30 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Ramona Gradinariu <ramona.bolboaca13@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, conor+dt@...nel.org,
 krzysztof.kozlowski+dt@...aro.org, robh@...nel.org, nuno.sa@...log.com
Subject: Re: [PATCH v2 8/8] drivers: iio: imu: Add support for adis1657x
 family

On Fri, 17 May 2024 11:00:36 +0300
Ramona Gradinariu <ramona.bolboaca13@...il.com> wrote:

> >> + * device will send the data popped with the (n-1)th consecutive burst request.
> >> + * In order to read the data which was popped previously, without popping the FIFO,
> >> + * the 0x00 0x00 burst request has to be sent.
> >> + * If after a 0x68 0x00 FIFO pop burst request, there is any other device access
> >> + * different from a 0x68 0x00 or a 0x00 0x00 burst request, the FIFO data popped
> >> + * previously will be lost.
> >> + */
> >> +static irqreturn_t adis16475_trigger_handler_with_fifo(int irq, void *p)
> >>  {
> >>  	struct iio_poll_func *pf = p;
> >>  	struct iio_dev *indio_dev = pf->indio_dev;
> >> +	struct adis16475 *st = iio_priv(indio_dev);
> >> +	struct adis *adis = &st->adis;
> >> +	int ret;
> >> +	u16 fifo_cnt, i;
> >>
> >> -	adis16475_push_single_sample(pf);
> >> +	adis_dev_lock(&st->adis);
> >> +
> >> +	ret = __adis_read_reg_16(adis, ADIS16575_REG_FIFO_CNT, &fifo_cnt);
> >> +	if (ret)
> >> +		goto unlock;
> >> +
> >> +	/*
> >> +	 * If no sample is available, nothing can be read. This can happen if
> >> +	 * a the used trigger has a higher frequency than the selected sample rate.
> >> +	 */
> >> +	if (!fifo_cnt)
> >> +		goto unlock;
> >> +
> >> +	/*
> >> +	 * First burst request - FIFO pop: popped data will be returned in the
> >> +	 * next burst request.
> >> +	 */
> >> +	ret = adis16575_custom_burst_read(pf, adis->data->burst_reg_cmd);
> >> +	if (ret)
> >> +		goto unlock;
> >> +
> >> +	for (i = 0; i < fifo_cnt - 1; i++) {
> >> +		ret = adis16475_push_single_sample(pf);
> >> +		if (ret)
> >> +			goto unlock;
> >> +	}
> >> +  
> > My paranoid instincts for potential race conditions kick in.
> > Is this one of those annoying devices where the fifo interrupt will only occur
> > again if we successfully read enough data to get below the threshold?
> > Snag with no public datasheet is I can't just look it up!
> > If it's a level interrupt this won't be a problem.
> >
> > If so the race is the following.
> > 1. Interrupt happens, we read the number of entries in fifo.
> > 2. We read out the fifo, but for some reason our read is slow... (contention on
> >    bus, CPU overheating, who knows).  The data fills up at roughly the
> >    same rate as we are reading.
> > 3. We try to carry on but in reality the fifo contents never dropped below
> >    the watermark, so not more interrupts ever occur.
> >
> > Solution normally is to put this read sequence in a while (fifo_cnt)
> > and reread that after you've done the burst read.  If there is more data
> > go around again.  That way we drive for definitely having gotten to zero
> > at some stage - and hence whatever the threshold is set to a new interrupt
> > will occur.  
> 
> Hello Jonathan,
> 
> Indeed the watermark interrupt is a level interrupt. However adis lib does not 
> allow for level interrupts, so I had to create a new patch in v3 to handle it.
> Until now I tested the watermark interrupt as and edge interrupt and I did not
> see any issues, but indeed if the FIFO is not read fast enough the watermark pin 
> will stay high (or low depending on the configured polarity), so the correct 
> implementation is to use level interrupts for FIFO watermark interrupts.
That is the easy way certainly!

Sounds good to me.

Jonathan

> 
> Ramona G.
> 
>