All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] workaround regression in ina2xx introduced by cb47755725da("time: Prevent undefined behaviour in timespec64_to_ns()")
@ 2021-09-26 17:16 Iain Hunter
  2021-09-26 21:18 ` Thomas Gleixner
  0 siblings, 1 reply; 4+ messages in thread
From: Iain Hunter @ 2021-09-26 17:16 UTC (permalink / raw)
  Cc: lothar.felten, iain, Jonathan Cameron, Lars-Peter Clausen,
	Alexandru Ardelean, Gwendal Grignou, Matt Ranostay,
	Arnd Bergmann, Thomas Gleixner, Zeng Tao, linux-iio,
	linux-kernel

From: Iain Hunter <iain@hunterembedded.co.uk>

That change adds an error check to avoid saturation during multiplication
to calculate nano seconds in timespec64_to_ns().
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. In the -1 case timespec64_to_ns()
now clamps the -1 second value to KTIME_MAX. This essentially puts ina2xx
thread to sleep forever.
Proposed patch is to split the functionality in the loop into two parts:
1 do while loop only does the test to see if the next sample time is in 
the future or in the past. If it is in the past it will be skipped
and the sample time incremented until it is in the future. This 
comparision can be done with timespec64_compare() as we are only 
interested in the sign being positive or negative.
2 after do while loop we know that next is later than now and so delta is
guaranteed to be positive. This means timespec64_to_ns() can be safely 
used.

Signed-off-by: Iain Hunter <iain@hunterembedded.co.uk>

Fixes: cb47755725da("time: Prevent undefined behaviour in timespec64_to_ns()")
---
 drivers/iio/adc/ina2xx-adc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index a4b2ff9e0..661bcf707 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -817,10 +817,10 @@ static int ina2xx_capture_thread(void *data)
 		 */
 		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);
+		} while (timespec64_compare(&next, &now) < 0);
 
+		delta = timespec64_sub(next, now);
+		delay_us = div_s64(timespec64_to_ns(&delta), 1000);
 		usleep_range(delay_us, (delay_us * 3) >> 1);
 
 	} while (!kthread_should_stop());
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] workaround regression in ina2xx introduced by cb47755725da("time: Prevent undefined behaviour in timespec64_to_ns()")
  2021-09-26 17:16 [PATCH v3] workaround regression in ina2xx introduced by cb47755725da("time: Prevent undefined behaviour in timespec64_to_ns()") Iain Hunter
@ 2021-09-26 21:18 ` Thomas Gleixner
  2021-09-30 16:18   ` Jonathan Cameron
  0 siblings, 1 reply; 4+ messages in thread
From: Thomas Gleixner @ 2021-09-26 21:18 UTC (permalink / raw)
  To: Iain Hunter
  Cc: lothar.felten, iain, Jonathan Cameron, Lars-Peter Clausen,
	Alexandru Ardelean, Gwendal Grignou, Matt Ranostay,
	Arnd Bergmann, Zeng Tao, linux-iio, linux-kernel

On Sun, Sep 26 2021 at 18:16, Iain Hunter wrote:
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -817,10 +817,10 @@ static int ina2xx_capture_thread(void *data)
>  		 */
>  		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);
> +		} while (timespec64_compare(&next, &now) < 0);
>  
> +		delta = timespec64_sub(next, now);
> +		delay_us = div_s64(timespec64_to_ns(&delta), 1000);

This whole timespec dance does not make any sense and can be completely
avoided by using just scalar nanoseconds. Untested patch below.

Thanks,

        tglx
---
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -775,7 +775,7 @@ static int ina2xx_capture_thread(void *d
 	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, delta;
 	s64 delay_us;
 
 	/*
@@ -785,7 +785,7 @@ static int ina2xx_capture_thread(void *d
 	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 *d
 			 * 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 *d
 		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 *d
 		 * 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());



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] workaround regression in ina2xx introduced by cb47755725da("time: Prevent undefined behaviour in timespec64_to_ns()")
  2021-09-26 21:18 ` Thomas Gleixner
@ 2021-09-30 16:18   ` Jonathan Cameron
  2021-09-30 16:20     ` Jonathan Cameron
  0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2021-09-30 16:18 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Iain Hunter, lothar.felten, iain, Lars-Peter Clausen,
	Alexandru Ardelean, Gwendal Grignou, Matt Ranostay,
	Arnd Bergmann, Zeng Tao, linux-iio, linux-kernel

On Sun, 26 Sep 2021 23:18:42 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Sun, Sep 26 2021 at 18:16, Iain Hunter wrote:
> > --- a/drivers/iio/adc/ina2xx-adc.c
> > +++ b/drivers/iio/adc/ina2xx-adc.c
> > @@ -817,10 +817,10 @@ static int ina2xx_capture_thread(void *data)
> >  		 */
> >  		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);
> > +		} while (timespec64_compare(&next, &now) < 0);
> >  
> > +		delta = timespec64_sub(next, now);
> > +		delay_us = div_s64(timespec64_to_ns(&delta), 1000);  
> 
> This whole timespec dance does not make any sense and can be completely
> avoided by using just scalar nanoseconds. Untested patch below.
> 
> Thanks,
> 
>         tglx

Thanks Thomas.

Iain could you test this approach?

Thanks,

Jonathan

> ---
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -775,7 +775,7 @@ static int ina2xx_capture_thread(void *d
>  	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, delta;
>  	s64 delay_us;
>  
>  	/*
> @@ -785,7 +785,7 @@ static int ina2xx_capture_thread(void *d
>  	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 *d
>  			 * 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 *d
>  		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 *d
>  		 * 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());
> 
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] workaround regression in ina2xx introduced by cb47755725da("time: Prevent undefined behaviour in timespec64_to_ns()")
  2021-09-30 16:18   ` Jonathan Cameron
@ 2021-09-30 16:20     ` Jonathan Cameron
  0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2021-09-30 16:20 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Iain Hunter, lothar.felten, iain, Lars-Peter Clausen,
	Alexandru Ardelean, Gwendal Grignou, Matt Ranostay,
	Arnd Bergmann, Zeng Tao, linux-iio, linux-kernel

On Thu, 30 Sep 2021 17:18:44 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sun, 26 Sep 2021 23:18:42 +0200
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > On Sun, Sep 26 2021 at 18:16, Iain Hunter wrote:  
> > > --- a/drivers/iio/adc/ina2xx-adc.c
> > > +++ b/drivers/iio/adc/ina2xx-adc.c
> > > @@ -817,10 +817,10 @@ static int ina2xx_capture_thread(void *data)
> > >  		 */
> > >  		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);
> > > +		} while (timespec64_compare(&next, &now) < 0);
> > >  
> > > +		delta = timespec64_sub(next, now);
> > > +		delay_us = div_s64(timespec64_to_ns(&delta), 1000);    
> > 
> > This whole timespec dance does not make any sense and can be completely
> > avoided by using just scalar nanoseconds. Untested patch below.
> > 
> > Thanks,
> > 
> >         tglx  
> 
> Thanks Thomas.
> 
> Iain could you test this approach?

Ah. Just seen v4, so I guess you did.

Thanks,

J
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> > --- a/drivers/iio/adc/ina2xx-adc.c
> > +++ b/drivers/iio/adc/ina2xx-adc.c
> > @@ -775,7 +775,7 @@ static int ina2xx_capture_thread(void *d
> >  	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, delta;
> >  	s64 delay_us;
> >  
> >  	/*
> > @@ -785,7 +785,7 @@ static int ina2xx_capture_thread(void *d
> >  	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 *d
> >  			 * 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 *d
> >  		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 *d
> >  		 * 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());
> > 
> >   
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-09-30 16:16 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-26 17:16 [PATCH v3] workaround regression in ina2xx introduced by cb47755725da("time: Prevent undefined behaviour in timespec64_to_ns()") Iain Hunter
2021-09-26 21:18 ` Thomas Gleixner
2021-09-30 16:18   ` Jonathan Cameron
2021-09-30 16:20     ` Jonathan Cameron

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.