On Friday 23 October 2015 17:24:59 WEN Pingbo wrote: > Using struct timeval will cause time overflow in 2038, replacing it with > ktime_t. And we don't need to handle sec and nsec separately. > > Since mlc->lcv_t is only interested in seconds, directly using > time64_t here. > > And monotonic time is better here, since the original driver don't care > the wall time. > > In addition, the original 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> The changelog text looks good. > +++ b/drivers/input/serio/hil_mlc.c > @@ -274,14 +274,12 @@ 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; > + time64_t now = ktime_get_seconds(); > > - do_gettimeofday(&tv); > - > - if (mlc->lcv && (tv.tv_sec - mlc->lcv_tv.tv_sec) < 5) > + if (mlc->lcv && (now - mlc->lcv_t) < 5) > return -1; > > - mlc->lcv_tv = tv; > + mlc->lcv_t = now; > mlc->lcv = 0; > > return 0; This part looks good now, but as I commented in version 1, it should really be a separate patch rather than combined with the rest that is dealing with another use of timeval. > > - 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); > + if (tmp.tv64 >= (mlc->intimeout * NSEC_PER_USEC)) > + goto sched; > + tmp.tv64 = mlc->intimeout * NSEC_PER_USEC - tmp.tv64; > + if (tmp.tv64 < NSEC_PER_USEC) > + goto sched; > + mod_timer(&hil_mlcs_kicker, > + jiffies + nsecs_to_jiffies(tmp.tv64)); > break; > sched: > tasklet_schedule(&hil_mlcs_tasklet); If I read this right, the code is executed one for each input event such as a keypress or mouse movement. In the latter case, doing nsecs_to_jiffies() here is actually a bit expensive, and I stil think it can be avoided by just using jiffies. For the (tmp.tv64 < NSEC_PER_USEC) part, did you just do that because I said this, or did you actually prove that it is required? I'm still confused about what the driver is trying to achieve here. > diff --git a/drivers/input/serio/hp_sdc_mlc.c b/drivers/input/serio/hp_sdc_mlc.c > index d50f067..0a27b89 100644 > --- a/drivers/input/serio/hp_sdc_mlc.c > +++ b/drivers/input/serio/hp_sdc_mlc.c > @@ -149,7 +149,8 @@ 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; > + ktime_t tmp = ktime_sub(ktime_get(), mlc->instart); > + > if (priv->emtestmode) { > mlc->ipacket[0] = > HIL_ERR_INT | (mlc->opacket & Maybe give the variable a more useful name than 'tmp'? You could also remove the variable entirely if you slightly restructure the condition below. > @@ -160,9 +161,8 @@ 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) { > + > + if (tmp.tv64 > mlc->intimeout * NSEC_PER_USEC) { > /* printk("!%i %i", > tv.tv_usec - mlc->instart.tv_usec, > mlc->intimeout); As I mentioned, better use ktime_to_ns() instead of accessing the tv64 member directly, but that is just a small style question. 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