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@xxxxxxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxxx>
> >
> > 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());
