| Message ID | 1445161519-27000-1-git-send-email-pingbo.wen@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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@linaro.org> 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. > + 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? > + 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@linaro.org> > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.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. 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> ? 2015?10?23??17:45?Arnd Bergmann <arnd@arndb.de> ??? > > 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); >>>> - > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; return 0; @@ -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)); @@ -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; + 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; + mod_timer(&hil_mlcs_kicker, + jiffies + nsecs_to_jiffies(ts64.tv_nsec)); break; sched: tasklet_schedule(&hil_mlcs_tasklet); diff --git a/drivers/input/serio/hp_sdc_mlc.c b/drivers/input/serio/hp_sdc_mlc.c index d50f067..369885d 100644 --- a/drivers/input/serio/hp_sdc_mlc.c +++ b/drivers/input/serio/hp_sdc_mlc.c @@ -149,7 +149,7 @@ static int hp_sdc_mlc_in(hil_mlc *mlc, suseconds_t timeout) /* Try to down the semaphore */ if (down_trylock(&mlc->isem)) { - struct timeval tv; + struct timespec64 ts64; if (priv->emtestmode) { mlc->ipacket[0] = HIL_ERR_INT | (mlc->opacket & @@ -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); diff --git a/include/linux/hil_mlc.h b/include/linux/hil_mlc.h index 394a840..a6b62fb 100644 --- a/include/linux/hil_mlc.h +++ b/include/linux/hil_mlc.h @@ -144,12 +144,12 @@ struct hil_mlc { hil_packet ipacket[16]; hil_packet imatch; int icount; - struct timeval instart; + struct timespec64 instart; suseconds_t intimeout; int ddi; /* Last operational device id */ int lcv; /* LCV to throttle loops */ - struct timeval lcv_tv; /* Time loop was started */ + struct timespec64 lcv_ts64; /* Time loop was started */ int di_map[7]; /* Maps below items to live devs */ struct hil_mlc_devinfo di[HIL_MLC_DEVMEM];
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@linaro.org> --- 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(-)