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 <pingbo.wen@xxxxxxxxxx>
>
> 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(&tv);
>> + ktime_get_ts64(&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(&mlc->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(&mlc->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.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(&hil_mlcs_kicker, jiffies + tv.tv_usec);
>> + ktime_get_ts64(&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 NSEC_PER_USEC is more safe, since we use nanosecond here.
>> + mod_timer(&hil_mlcs_kicker,
>> + jiffies + nsecs_to_jiffies(ts64.tv_nsec));
>> break;
>> sched:
>> tasklet_schedule(&hil_mlcs_tasklet);
>
> This part seems like it would be easier to just use jiffies instead
> of timspec64, to avoid having to convert it back.
>
>> @@ -160,9 +160,11 @@ static int hp_sdc_mlc_in(hil_mlc *mlc, suseconds_t timeout)
>> /* printk(KERN_DEBUG PREFIX ">[%x]\n", mlc->ipacket[0]); */
>> goto wasup;
>> }
>> - do_gettimeofday(&tv);
>> - tv.tv_usec += USEC_PER_SEC * (tv.tv_sec - mlc->instart.tv_sec);
>> - if (tv.tv_usec - mlc->instart.tv_usec > mlc->intimeout) {
>> + ktime_get_ts64(&ts64);
>> + ts64.tv_nsec += NSEC_PER_SEC *
>> + (ts64.tv_sec - mlc->instart.tv_sec);
>> + if (ts64.tv_nsec - mlc->instart.tv_nsec > mlc->intimeout *
>> + NSEC_PER_USEC) {
>> /* printk("!%i %i",
>> tv.tv_usec - mlc->instart.tv_usec,
>> mlc->intimeout);
>
> same here.
>
> Arnd
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
