| Message ID | 1445592300-30894-1-git-send-email-pingbo.wen@linaro.org |
|---|---|
| State | New |
| Headers | show |
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@linaro.org> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 在 2015年10月23日，17:55，Arnd Bergmann <arnd@arndb.de> 写道： > > 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. >> > > 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. Ok, I will split it in next version. >> - 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. More explanation here:) the judgement here is to prevent mod_timer with zero delta. I can not make sure whether the module have nanosecond precise, so just keep same. > >> 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. I see, thanks for point out. 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
On Friday 23 October 2015 20:32:50 Pingbo Wen 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); > >> + 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. > > More explanation here:) > the judgement here is to prevent mod_timer with zero delta. I can not > make sure whether the module have nanosecond precise, so just keep same. Ok, I guess I was misreading the original code. What it actually does is to check the remaining time in jiffies, not in microseconds, so the algorithm is: if (already expired) schedule tasklet else { convert to jiffies if (expired just now) schedule tasklet else schedule tasklet from timer } So the entire code is meant to guarantee that the tasklet is getting scheduled, and the first two cases are just an optimization to avoid going through the timer. I checked the code for mod_timer to verify that mod_timer with an argument in the past will just cause the handler to be called at the next tick, so we don't really need the middle case, and the logic becomes really simple if you use jiffies instead of ktime_t. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/drivers/input/serio/hil_mlc.c b/drivers/input/serio/hil_mlc.c index 65605e4..2746509 100644 --- a/drivers/input/serio/hil_mlc.c +++ 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; @@ -605,7 +603,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)); + mlc->instart = ktime_get(); mlc->icount = 15; memset(mlc->ipacket, 0, 16 * sizeof(hil_packet)); BUG_ON(down_trylock(&mlc->isem)); @@ -710,7 +708,7 @@ static int hilse_donode(hil_mlc *mlc) break; } mlc->ostarted = 0; - do_gettimeofday(&(mlc->instart)); + mlc->instart = ktime_get(); write_unlock_irqrestore(&mlc->lock, flags); nextidx = HILSEN_NEXT; break; @@ -731,18 +729,18 @@ static int hilse_donode(hil_mlc *mlc) #endif while (nextidx & HILSEN_SCHED) { - struct timeval tv; + ktime_t tmp = ktime_sub(ktime_get(), mlc->instart); 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); + 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); 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 & @@ -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); diff --git a/include/linux/hil_mlc.h b/include/linux/hil_mlc.h index 394a840..2222ec6 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; + ktime_t instart; suseconds_t intimeout; int ddi; /* Last operational device id */ int lcv; /* LCV to throttle loops */ - struct timeval lcv_tv; /* Time loop was started */ + time64_t lcv_t; /* 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 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@linaro.org> --- drivers/input/serio/hil_mlc.c | 28 +++++++++++++--------------- drivers/input/serio/hp_sdc_mlc.c | 8 ++++---- include/linux/hil_mlc.h | 4 ++-- 3 files changed, 19 insertions(+), 21 deletions(-)