Re: [Y2038] [PATCH] hil_mlc: convert timeval to timespec64

[Arnd Bergmann]

On Friday 23 October 2015 17:12:38 Pingbo Wen wrote:
> On Monday, October 19, 2015 04:58 PM, Arnd Bergmann wrote:
> >> -do_gettimeofday();
> >> -tv.tv_usec += USEC_PER_SEC * (tv.tv_sec - 
> >> mlc->instart.tv_sec);
> >> -tv.tv_usec -= mlc->instart.tv_usec;
> >> -if (tv.tv_usec >= mlc->intimeout) goto sched;
> >> -tv.tv_usec = (mlc->intimeout - tv.tv_usec) * HZ / 
> >> USEC_PER_SEC;
> >> -if (!tv.tv_usec) goto sched;
> >> -mod_timer(_mlcs_kicker, jiffies + tv.tv_usec);
> >> +ktime_get_ts64();
> >> +ts64.tv_nsec += NSEC_PER_SEC *
> >> +(ts64.tv_sec - mlc->instart.tv_sec);
> >> +ts64.tv_nsec -= mlc->instart.tv_nsec;
> > 
> > tv_nsec will overflow here for any timeout over 4.3 seconds, where it
> > used to overflow after 4294 seconds. This is almost certainly a bug.
> > 
> > You could work around that by using ktime_get_ns() to get a nanosecond
> > value right away, but a 64-bit number is more expensive to convert to
> > jiffies.
> 
> You are right, I didn't notice that tv_nsec is a 32bit variable. Maybe
> we should use ktime_t here, so that handling sec and nsec separately is
> needless.
> 
> Using jiffies here will need to take more codes to handle jiffies overflow
> carefully. I think coverting 64bit number to jiffies is the price we must 
> take, if we use 64bit version here.

Handling the jiffies overflow is trivially done through the time_before()
and time_after() helpers, like


start = jiffies;
...
now = jiffies;
timeout = start + HZ * timeout_usec / USEC_PER_SEC;
if (time_after(now, start + timeout_jiffies)
timeout();
else
mod_timer(timer, start + timeout_jiffies);

The time_after function works because unsigned overflow is well-defined
in C (unlike signed overflow).

As a side-note, please take the time to delete any lines from the original
mail that you are not referencing when you reply.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Y2038] [PATCH] hil_mlc: convert timeval to timespec64

[Pingbo Wen]

On Monday, October 19, 2015 04:58 PM, Arnd Bergmann wrote:
> On Sunday 18 October 2015 17:45:19 WEN Pingbo wrote:
>> Using struct timeval will cause time overflow in 2038, replacing it with
>> a 64bit version.
>>
>> In addition, the origin driver try to covert usec to jiffies manually in
>> hilse_donode(). This is not a universal and safe way, using
>> nsecs_to_jiffies() to fix that.
>>
>> Signed-off-by: WEN Pingbo 
> 
> You should mention somewhere that you are also converting from real
> time to monotonic time, and why this is done.
> 
>> ---
>>  drivers/input/serio/hil_mlc.c| 31 +--
>>  drivers/input/serio/hp_sdc_mlc.c | 10 ++
>>  include/linux/hil_mlc.h  |  4 ++--
>>  3 files changed, 25 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/input/serio/hil_mlc.c b/drivers/input/serio/hil_mlc.c
>> index 65605e4..4e3b926 100644
>> --- a/drivers/input/serio/hil_mlc.c
>> +++ b/drivers/input/serio/hil_mlc.c
>> @@ -274,14 +274,14 @@ static int hilse_match(hil_mlc *mlc, int unused)
>>  /* An LCV used to prevent runaway loops, forces 5 second sleep when reset. 
>> */
>>  static int hilse_init_lcv(hil_mlc *mlc, int unused)
>>  {
>> -struct timeval tv;
>> +struct timespec64 ts64;
>>  
>> -do_gettimeofday();
>> +ktime_get_ts64();
>>  
>> -if (mlc->lcv && (tv.tv_sec - mlc->lcv_tv.tv_sec) < 5)
>> +if (mlc->lcv && (ts64.tv_sec - mlc->lcv_ts64.tv_sec) < 5)
>>  return -1;
>>  
>> -mlc->lcv_tv = tv;
>> +mlc->lcv_ts64 = ts64;
>>  mlc->lcv = 0;
> 
> No need to rename the two variables here. Also, it seems we never access the
> tv_nsec portion at all, so this could use the simpler ktime_get_seconds()
> or even 'jiffies' instead.
> 
>> @@ -605,7 +605,7 @@ static inline void hilse_setup_input(hil_mlc *mlc, const 
>> struct hilse_node *node
>>  }
>>  mlc->istarted = 1;
>>  mlc->intimeout = node->arg;
>> -do_gettimeofday(&(mlc->instart));
>> +ktime_get_ts64(&(mlc->instart));
>>  mlc->icount = 15;
>>  memset(mlc->ipacket, 0, 16 * sizeof(hil_packet));
>>  BUG_ON(down_trylock(>isem));
> 
> This looks unrelated to the change above, so I would suggest making separate 
> patches.
> 
>> @@ -710,7 +710,7 @@ static int hilse_donode(hil_mlc *mlc)
>>  break;
>>  }
>>  mlc->ostarted = 0;
>> -do_gettimeofday(&(mlc->instart));
>> +ktime_get_ts64(&(mlc->instart));
>>  write_unlock_irqrestore(>lock, flags);
>>  nextidx = HILSEN_NEXT;
>>  break;
>> @@ -731,18 +731,21 @@ static int hilse_donode(hil_mlc *mlc)
>>  #endif
>>  
>>  while (nextidx & HILSEN_SCHED) {
>> -struct timeval tv;
>> +struct timespec64 ts64;
>>  
>>  if (!sched_long)
>>  goto sched;
>>  
>> -do_gettimeofday();
>> -tv.tv_usec += USEC_PER_SEC * (tv.tv_sec - mlc->instart.tv_sec);
>> -tv.tv_usec -= mlc->instart.tv_usec;
>> -if (tv.tv_usec >= mlc->intimeout) goto sched;
>> -tv.tv_usec = (mlc->intimeout - tv.tv_usec) * HZ / USEC_PER_SEC;
>> -if (!tv.tv_usec) goto sched;
>> -mod_timer(_mlcs_kicker, jiffies + tv.tv_usec);
>> +ktime_get_ts64();
>> +ts64.tv_nsec += NSEC_PER_SEC *
>> +(ts64.tv_sec - mlc->instart.tv_sec);
>> +ts64.tv_nsec -= mlc->instart.tv_nsec;
> 
> tv_nsec will overflow here for any timeout over 4.3 seconds, where it
> used to overflow after 4294 seconds. This is almost certainly a bug.
> 
> You could work around that by using ktime_get_ns() to get a nanosecond
> value right away, but a 64-bit number is more expensive to convert to
> jiffies.

You are right, I didn't notice that tv_nsec is a 32bit variable. Maybe
we should use ktime_t here, so that handling sec and nsec separately is
needless.

Using jiffies here will need to take more codes to handle jiffies overflow
carefully. I think coverting 64bit number to jiffies is the price we must 
take, if we use 64bit version here.

> 
>> +if (ts64.tv_nsec >= (mlc->intimeout * NSEC_PER_USEC))
>> +goto sched;
>> +ts64.tv_nsec = mlc->intimeout * NSEC_PER_USEC - ts64.tv_nsec;
>> +if (!ts64.tv_nsec) goto sched;
> 
> As you are modifying the line, you should also fix the coding style to
> write
> 
>   if (!ts64.tv_nsec)
>   goto sched;
> 
> I also notice that you modify the behavior here, by changing from
> microsecond to nanosecond resolution, the equivalent of the original
> would have been
> 
>   if (ts64.tv_nsec < NSECS_PER_USEC)
> 
> 
> Your current version looks like it will practically never be true (meaning
> you hit the exact nanosecond). Is this conditional actually needed at all
> then? If it is, what is the intention and what should it be?
> 

Yes, compare to 

Re: [Y2038] [PATCH] hil_mlc: convert timeval to timespec64

[Pingbo Wen]

> 在 2015年10月23日，17:45，Arnd Bergmann  写道：
> 
> On Friday 23 October 2015 17:12:38 Pingbo Wen wrote:
>> On Monday, October 19, 2015 04:58 PM, Arnd Bergmann wrote:
 -do_gettimeofday();
 -
> Handling the jiffies overflow is trivially done through the time_before()
> and time_after() helpers, like
> 
> 
>   start = jiffies;
>   ...
>   now = jiffies;
>   timeout = start + HZ * timeout_usec / USEC_PER_SEC;
>   if (time_after(now, start + timeout_jiffies)
>   timeout();
>   else
>   mod_timer(timer, start + timeout_jiffies);
> 
> The time_after function works because unsigned overflow is well-defined
> in C (unlike signed overflow).
> 

Make sense, I will try this in next version.

Thanks,
Pingbo

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html