From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753617AbbJSI6T (ORCPT ); Mon, 19 Oct 2015 04:58:19 -0400 Received: from mout.kundenserver.de ([212.227.126.187]:64087 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751209AbbJSI6R (ORCPT ); Mon, 19 Oct 2015 04:58:17 -0400 From: Arnd Bergmann To: y2038@lists.linaro.org Cc: WEN Pingbo , dmitry.torokhov@gmail.com, linux-kernel@vger.kernel.org, linux-input@vger.kernel.org Subject: Re: [Y2038] [PATCH] hil_mlc: convert timeval to timespec64 Date: Mon, 19 Oct 2015 10:58:11 +0200 Message-ID: <46052377.96MapaKFAy@wuerfel> User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; ) In-Reply-To: <1445161519-27000-1-git-send-email-pingbo.wen@linaro.org> References: <1445161519-27000-1-git-send-email-pingbo.wen@linaro.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:puuIxPzeBTGfBm1EUie/yovpHIb052B7XiHm92fz8iEpT9Fkbz6 bqNcz9WdIR5BMrTieeb8TER+TUFOd2dTzQcfu2deeytN/ZRJBYIV24sCf3+HTYmEwaLJmDJ bprHZP8ldDIrjdOhwMs1segyb4I+GsVaYX+YyA8qAhOrLfTo5yMDn5IqjABKoyX2DcNpNaC D9sUWvEF4bxjAfH203h4w== X-UI-Out-Filterresults: notjunk:1;V01:K0:TI5xO0VHeIA=:raexsJ9OXltwOmQ8bwbKzq V8FNYVQsr3+1EDr0GD0bwandaE9TnaZJJh5/jskIRKf3gz/k5ix3JdLQ9VQvDkN392rPMfp71 mHcLifw63rJ4sbqLVgmGKouZ4TPee7hW1qfajPkBnOSNfZTltW+xps9zlObl9xQwA0ys1MGWA i5T6C5yUAqv9pAHJD1czdH8zTWH+pFPnT0+qYh7oVQIU5TZDKimtgYmS5AOwSqYoph8bHr8Zl 4mdKqinaB+rhcc0S02ZAe6TplAlA7WrCpUFWIYtdecdLCkKVkYfxJx2hPeufNL3nccOLLMO26 3A+Ib62/WPlMzI98CVMGWZYFtKohRCNNBM9XAjqaUOmo4WwY30tdWK28OW98V2Uku8dBmxsQJ RtstrKwAtkD4aT8JNGAuCmnGKX0AOiyqO4wZU1lNF5rztqfYoON5kOYn+Tq7a/VqyPdOcH+gq PwKcjP1mP90fKkmElZ4yghJMhiemno57RSGlPmqJ947r/SjBGouTx/qVvlXdO7qKfMHdoV2FR 6xXqgMInu3Mb5ouwIvM9YTnGS6IxxkNVC6IaP0dz0efZ5jPFiFVENocZoqWtHG4iuNp3LaEQD U4OouVXzA0xxqDvsJwyv3iI1Lnr9j7bhUo9K9glGhiEFaqMvFeR4i6t2Zby8JkM2sRJTPublb kshFjAicUIjGXuPPsB6HA9+msSa+YBei2ZlDlFKvlKr+3rhAF6+DYJJimcD9kQVeZRbrfmPgN OujCzIrpsH5kUp6k Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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