Re: [PATCH v5] workaround regression in ina2xx introduced by cb47755725da("time: Prevent undefined behaviour in timespec64_to_ns()")

[Iain Hunter <drhunter95@gmail.com>]

On Thursday, 16 December 2021 18:47:30 GMT Lars-Peter Clausen wrote:
> On 12/16/21 7:34 PM, Iain Hunter wrote:
> > From: Iain Hunter <iain@hunterembedded.co.uk>
> > 
> > Commit cb47755725da("time: Prevent undefined behaviour in
> > timespec64_to_ns()") introduced a regression in the ina2xx driver.
> > In ina2xx_capture_thread() a timespec64 structure is used to calculate
> > the delta time until the next sample time. This delta can be negative if
> > the next sample time was in the past which is common in ina2xx driver.
> > In the negative case timespec64_to_ns() now clamps the negative time
> > to KTIME_MAX. This essentially puts ina2xx thread to sleep forever.
> > Proposed patch is to:
> > a) change from timespec64_XXX() to standard raw ktime_XXX() APIs to remove
> > non-standard timespec64 calls.
> > 
> > b) split the functionality in the loop into two parts:
> >   - do while loop only does the test to see if the next sample time is in
> >   the
> > 
> > future or in the past. If in the past and the next sample time will be
> > incremented until it is in the future. This test is done with a simple
> > signed comparison as we are only interested in the sign being positive or
> > negative.
> > 
> >   - after do while loop we know that next is later than now and so delay
> >   is
> > 
> > positive and ksub_sub() can be used to get the delay which is positive.
> 
> This sounds to me as if the original commit that introduced the change
> is broken since it doesn't handle negative timespecs. And other drivers
> would be affected by this as well.
> 
> Had a quick look and there is commit 39ff83f2f6cc "time: Handle negative
> seconds correctly in timespec64_to_ns()"[1].
> 
> Which should also fix this driver.
> 
> - Lars

Hi Lars,
From a functionality point of view commit[1] would fix the ina2xx driver. 
However, during the original patch discussion it was pointed out that ktime 
API is a much more standard solution to work out timings and that timespec64 
didn't provide any benefit. There is only one other reference to timespec64 in 
drivers/iio (in industrialio-core.c) but many usages of ktime.
Iain
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i
> d=39ff83f2f6cc
> > Signed-off-by: Iain Hunter <iain@hunterembedded.co.uk>
> > 
> > Fixes: cb47755725da("time: Prevent undef$
> > ---
> > 
> >   drivers/iio/adc/ina2xx-adc.c | 15 +++++++--------
> >   1 file changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> > index a4b2ff9e0..17f702772 100644
> > --- a/drivers/iio/adc/ina2xx-adc.c
> > +++ b/drivers/iio/adc/ina2xx-adc.c
> > @@ -775,7 +775,7 @@ static int ina2xx_capture_thread(void *data)
> > 
> >   	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> >   	int sampling_us = SAMPLING_PERIOD(chip);
> >   	int ret;
> > 
> > -	struct timespec64 next, now, delta;
> > +	ktime_t next, now;
> > 
> >   	s64 delay_us;
> >   	
> >   	/*
> > 
> > @@ -785,7 +785,7 @@ static int ina2xx_capture_thread(void *data)
> > 
> >   	if (!chip->allow_async_readout)
> >   	
> >   		sampling_us -= 200;
> > 
> > -	ktime_get_ts64(&next);
> > +	next = ktime_get();
> > 
> >   	do {
> >   	
> >   		while (!chip->allow_async_readout) {
> > 
> > @@ -798,7 +798,7 @@ static int ina2xx_capture_thread(void *data)
> > 
> >   			 * reset the reference timestamp.
> >   			 */
> >   			
> >   			if (ret == 0)
> > 
> > -				ktime_get_ts64(&next);
> > +				next = ktime_get();
> > 
> >   			else
> >   			
> >   				break;
> >   		
> >   		}
> > 
> > @@ -807,7 +807,7 @@ static int ina2xx_capture_thread(void *data)
> > 
> >   		if (ret < 0)
> >   		
> >   			return ret;
> > 
> > -		ktime_get_ts64(&now);
> > +		now = ktime_get();
> > 
> >   		/*
> >   		
> >   		 * Advance the timestamp for the next poll by one sampling
> > 
> > @@ -816,11 +816,10 @@ static int ina2xx_capture_thread(void *data)
> > 
> >   		 * multiple times, i.e. samples are dropped.
> >   		 */
> >   		
> >   		do {
> > 
> > -			timespec64_add_ns(&next, 1000 * sampling_us);
> > -			delta = timespec64_sub(next, now);
> > -			delay_us = div_s64(timespec64_to_ns(&delta), 
1000);
> > -		} while (delay_us <= 0);
> > +			next = ktime_add_us(next, sampling_us);
> > +		} while (next <= now);
> > 
> > +		delay_us = ktime_to_us(ktime_sub(next, now));
> > 
> >   		usleep_range(delay_us, (delay_us * 3) >> 1);
> >   	
> >   	} while (!kthread_should_stop());