* [PATCH] workaround regression in ina2xx introduced by cb47755725da("time: Prevent undefined behaviour in timespec64_to_ns()")
@ 2021-09-11 11:36 Iain Hunter
2021-09-11 13:23 ` Jonathan Cameron
0 siblings, 1 reply; 2+ messages in thread
From: Iain Hunter @ 2021-09-11 11:36 UTC (permalink / raw)
Cc: lothar.felten, iain, Jonathan Cameron, Lars-Peter Clausen,
Alexandru Ardelean, Matt Ranostay, Gwendal Grignou, 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 replace the call to timespec64_to_ns() with the
contents of that function without the overflow test introduced by the
commit (ie revert to pre kernel 5.4 behaviour)
Signed-off-by: Iain Hunter <iain@hunterembedded.co.uk>
---
drivers/iio/adc/ina2xx-adc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index a4b2ff9e0..ba3e98fde 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -777,6 +777,7 @@ static int ina2xx_capture_thread(void *data)
int ret;
struct timespec64 next, now, delta;
s64 delay_us;
+ s64 delta_ns;
/*
* Poll a bit faster than the chip internal Fs, in case
@@ -818,7 +819,8 @@ 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);
+ delta_ns = (((s64)delta.tv_sec) * NSEC_PER_SEC)+delta.tv_nsec;
+ delay_us = div_s64(delta_ns, 1000);
} while (delay_us <= 0);
usleep_range(delay_us, (delay_us * 3) >> 1);
--
2.17.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] workaround regression in ina2xx introduced by cb47755725da("time: Prevent undefined behaviour in timespec64_to_ns()")
2021-09-11 11:36 [PATCH] workaround regression in ina2xx introduced by cb47755725da("time: Prevent undefined behaviour in timespec64_to_ns()") Iain Hunter
@ 2021-09-11 13:23 ` Jonathan Cameron
0 siblings, 0 replies; 2+ messages in thread
From: Jonathan Cameron @ 2021-09-11 13:23 UTC (permalink / raw)
To: Iain Hunter
Cc: lothar.felten, iain, Lars-Peter Clausen, Alexandru Ardelean,
Matt Ranostay, Gwendal Grignou, linux-iio, linux-kernel
On Sat, 11 Sep 2021 12:36:23 +0100
Iain Hunter <drhunter95@gmail.com> wrote:
> 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 replace the call to timespec64_to_ns() with the
> contents of that function without the overflow test introduced by the
> commit (ie revert to pre kernel 5.4 behaviour)
>
> Signed-off-by: Iain Hunter <iain@hunterembedded.co.uk>
Needs a fixes tag with the patch you mention above that added the check
so that we can tell how far back this needs to be backported.
> ---
> drivers/iio/adc/ina2xx-adc.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index a4b2ff9e0..ba3e98fde 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -777,6 +777,7 @@ static int ina2xx_capture_thread(void *data)
> int ret;
> struct timespec64 next, now, delta;
> s64 delay_us;
> + s64 delta_ns;
>
> /*
> * Poll a bit faster than the chip internal Fs, in case
> @@ -818,7 +819,8 @@ 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);
> + delta_ns = (((s64)delta.tv_sec) * NSEC_PER_SEC)+delta.tv_nsec;
spaces around the +
> + delay_us = div_s64(delta_ns, 1000);
Hmm. The negative timestamp is a bit of a mess anyway. Perhaps we can do something
neater using the standard functions by checking the validity of the timestamp
using timespec64_valid_strict() in the while loop and dropping the div_s64 out
of the loop.
What do you think? Would need a comment to explain why we the check on
it being valid though.
Jonathan
> } while (delay_us <= 0);
>
> usleep_range(delay_us, (delay_us * 3) >> 1);
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-09-11 13:22 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-11 11:36 [PATCH] workaround regression in ina2xx introduced by cb47755725da("time: Prevent undefined behaviour in timespec64_to_ns()") Iain Hunter
2021-09-11 13:23 ` 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.