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....@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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
