On Mon, May 11, 2015 at 7:39 PM, John Stultz <[email protected]> wrote: > On Sun, May 10, 2015 at 9:38 AM, Ksenija Stanojevic > <[email protected]> wrote: >> 'struct timeval last_tv' is used to get the time of last signal change >> and 'struct timeval last_intr_tv' is used to get the time of last UART >> interrupt. >> 32-bit systems using 'struct timeval' will break in the year 2038, so we >> have to replace that code with more appropriate types. >> Here struct timeval is replaced with ktime_t. >> >> Signed-off-by: Ksenija Stanojevic <[email protected]> >> --- >> drivers/staging/media/lirc/lirc_sir.c | 41 >> ++++++++++++++--------------------- >> 1 file changed, 16 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/staging/media/lirc/lirc_sir.c >> b/drivers/staging/media/lirc/lirc_sir.c >> index 29087f6..a4ea2b1 100644 >> --- a/drivers/staging/media/lirc/lirc_sir.c >> +++ b/drivers/staging/media/lirc/lirc_sir.c >> @@ -44,7 +44,7 @@ >> #include <linux/ioport.h> >> #include <linux/kernel.h> >> #include <linux/serial_reg.h> >> -#include <linux/time.h> >> +#include <linux/ktime.h> >> #include <linux/string.h> >> #include <linux/types.h> >> #include <linux/wait.h> >> @@ -127,9 +127,9 @@ static int threshold = 3; >> static DEFINE_SPINLOCK(timer_lock); >> static struct timer_list timerlist; >> /* time of last signal change detected */ >> -static struct timeval last_tv = {0, 0}; >> +static ktime_t last_tv; >> /* time of last UART data ready interrupt */ >> -static struct timeval last_intr_tv = {0, 0}; >> +static ktime_t last_intr_tv; > > > So here and for the other values, the _tv postfix means timeval. So > since you're changing the type, it probably would be cleaner to change > the name to something that makes more sense. ie: last_signal_time, > last_interrupt_time, curr_time. > >> static int last_value; >> >> static DECLARE_WAIT_QUEUE_HEAD(lirc_read_queue); >> @@ -400,17 +400,16 @@ static void drop_chrdev(void) >> } >> >> /* SECTION: Hardware */ >> -static long delta(struct timeval *tv1, struct timeval *tv2) >> +static long delta(ktime_t tv1, ktime_t tv2) >> { >> unsigned long deltv; >> + ktime_t delkt; >> >> - deltv = tv2->tv_sec - tv1->tv_sec; >> - if (deltv > 15) >> + delkt = ktime_sub(tv2, tv1); >> + if (ktime_compare(delkt, ktime_set(15, 0)) > 0) >> deltv = 0xFFFFFF; >> else >> - deltv = deltv*1000000 + >> - tv2->tv_usec - >> - tv1->tv_usec; >> + deltv = (int)ktime_to_us(delkt); >> return deltv; >> } > > > Can this whole function be killed and replaced w/ a > ktime_to_us(ktime_sub(t1, t2))? > > The if (>15 sec) return -1 bit is curious, since it doesn't seem > clearly documented I don't see much error checking in the file for > this case. I'm guessing its just trying to force the 32bit ceiling > instead of overflowing, but that would be easy enough to preserve w/ > something like > > static inline long delta(ktime_t t1, ktime_t t2) > { > /* return the delta in 32bit usecs, but cap to UINTMAX in case the > delta is greater then 32bits */ > return (long) min(ktime_to_us(ktime_sub(t1, t2)), UINT_MAX); > }
Hi John, Thank you for the review. I already did v2 of this patch because I messed up the subject line : timeva -> timeval. Arnd replied to that version and also cc linux-media group, so they might know better whether the change you proposed is correct. Thanks, Ksenija _______________________________________________ Y2038 mailing list [email protected] https://lists.linaro.org/mailman/listinfo/y2038
