On Sun, 19 Dec 2021 11:39:20 +0000
Iain Hunter <drhunter95@xxxxxxxxx> 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 <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$
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());
>
>
>
>
