From: Iain Hunter <[email protected]>
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.
Signed-off-by: Iain Hunter <[email protected]>
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());
--
2.17.1
On 12/16/21 7:34 PM, Iain Hunter wrote:
> From: Iain Hunter <[email protected]>
>
> 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
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=39ff83f2f6cc
>
> Signed-off-by: Iain Hunter <[email protected]>
>
> 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());
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 <[email protected]>
> >
> > 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 <[email protected]>
> >
> > 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());
On Sun, 19 Dec 2021 11:39:20 +0000
Iain Hunter <[email protected]> wrote:
> 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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > >
> > > Fixes: cb47755725da("time: Prevent undef$
Please fix this tag and resend. Also there should be no blank lines in the tag
block.
Or perhaps drop the tag and treat this as a cleanup, which is what I believe it
is after the fix Lars called out.
Patch title also should be in keeping with patches to the subsystem in question.
Here something like:
iio:adc:ina2xx: Switch from timespec64 to ktime_t.
Thanks,
Jonathan
> > > ---
> > >
> > > 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());
>
>
>
>