isdn: Use ktime_t instead of 'struct timeval'
Subsystems (2)
PATCH v2 1/1 isdn: Use ktime_t instead of 'struct timeval'
'struct timeval' uses 32-bit representation for seconds which will overflow in year 2038 and beyond. mISDN/clock.c needs to compute and store elapsed time in intervals of 125 microseconds. This patch replaces the usage of 'struct timeval' with 64-bit ktime_t which is y2038 safe. The patch also replaces do_gettimeofday() (wall-clock time) with ktime_get() (monotonic time) since we only care about elapsed time here.
--
Changes in v2:
- Avoid possible truncation bug caused by assigning ktime_us_delta output to a 16-bit number.
- Use ktime_us_delta, more concise than a combination of ktime_sub and ktime_to_us.
--- drivers/isdn/mISDN/clock.c | 55 ++++++++++++++++++---------------------------- include/linux/mISDNif.h | 2 +- 2 files changed, 22 insertions(+), 35 deletions(-)
drivers/isdn/mISDN/clock.c +21 -34
@@ -37,6 +37,7 @@#include <linux/types.h> #include <linux/stddef.h> #include <linux/spinlock.h> +#include <linux/ktime.h> #include <linux/mISDNif.h> #include <linux/export.h> #include "core.h"
@@ -45,7 +46,7 @@ static u_int *debug;static LIST_HEAD (iclock_list);static DEFINE_RWLOCK (iclock_lock);static u16 iclock_count;/* counter of last clock */ -static struct timeval iclock_tv;/* time stamp of last clock */ +static ktime_t iclock_tv;/* time stamp of last clock */ static int iclock_tv_valid;/* already received one timestamp */ static struct mISDNclock *iclock_current;
@@ -53,7 +54,7 @@ void mISDN_init_clock(u_int *dp) { debug = dp; -do_gettimeofday (&iclock_tv); + iclock_tv =ktime_get (); } static void
@@ -139,12 +140,11 @@ mISDN_unregister_clock(struct mISDNclock *iclock)EXPORT_SYMBOL (mISDN_unregister_clock);void -mISDN_clock_update (struct mISDNclock *iclock,int samples,struct timeval *tv) +mISDN_clock_update (struct mISDNclock *iclock,int samples,ktime_t *tv) {u_long flags; -struct timeval tv_now; -time_t elapsed_sec; -int elapsed_8000th; +ktime_t tv_now; +u16 delta;write_lock_irqsave (&iclock_lock, flags);if (iclock_current != iclock) {
@@ -160,28 +160,21 @@ mISDN_clock_update(struct mISDNclock *iclock, int samples, struct timeval *tv)/* increment sample counter by given samples */ iclock_count += samples;if (tv) {/* tv must be set, if function call is delayed */ - iclock_tv.tv_sec = tv->tv_sec; - iclock_tv.tv_usec = tv->tv_usec; - }else -do_gettimeofday (&iclock_tv); + iclock_tv = *tv; + }else { + iclock_tv =ktime_get (); + } }else {/* calc elapsed time by system clock */ if (tv) {/* tv must be set, if function call is delayed */ - tv_now.tv_sec = tv->tv_sec; - tv_now.tv_usec = tv->tv_usec; - }else -do_gettimeofday (&tv_now); - elapsed_sec = tv_now.tv_sec - iclock_tv.tv_sec; - elapsed_8000th = (tv_now.tv_usec /125 ) - - (iclock_tv.tv_usec /125 ); -if (elapsed_8000th <0 ) { - elapsed_sec -=1 ; - elapsed_8000th +=8000 ; + tv_now = *tv; + }else { + tv_now =ktime_get (); } + delta =ktime_us_delta (tv_now, iclock_tv) /125 ;/* add elapsed time to counter and set new timestamp */ - iclock_count += elapsed_sec *8000 + elapsed_8000th; - iclock_tv.tv_sec = tv_now.tv_sec; - iclock_tv.tv_usec = tv_now.tv_usec; + iclock_count += delta; + iclock_tv = tv_now; iclock_tv_valid =1 ;if (*debug &DEBUG_CLOCK ) printk("Received first clock from source '%s'.\n",
@@ -195,22 +188,16 @@ unsigned shortmISDN_clock_get (void) {u_long flags; -struct timeval tv_now; -time_t elapsed_sec; -int elapsed_8000th; +ktime_t tv_now; +u16 delta;u16 count;read_lock_irqsave (&iclock_lock, flags);/* calc elapsed time by system clock */ -do_gettimeofday (&tv_now); - elapsed_sec = tv_now.tv_sec - iclock_tv.tv_sec; - elapsed_8000th = (tv_now.tv_usec /125 ) - (iclock_tv.tv_usec /125 ); -if (elapsed_8000th <0 ) { - elapsed_sec -=1 ; - elapsed_8000th +=8000 ; - } + tv_now =ktime_get (); + delta =ktime_us_delta (tv_now, iclock_tv) /125 ;/* add elapsed time to counter */ - count = iclock_count + elapsed_sec *8000 + elapsed_8000th; + count = iclock_count + delta;read_unlock_irqrestore (&iclock_lock, flags);return count; }
include/linux/mISDNif.h +1 -1
@@ -596,7 +596,7 @@ static inline struct mISDNdevice *dev_to_mISDN(struct device *dev) }extern void set_channel_address (struct mISDNchannel *,u_int ,u_int ); -extern void mISDN_clock_update (struct mISDNclock *,int ,struct timeval *); +extern void mISDN_clock_update (struct mISDNclock *,int ,ktime_t *);extern unsigned short mISDN_clock_get (void );extern const char *mISDNDevName4ch (struct mISDNchannel *);
Signature · 1 line
2.2.0.rc0.207.ga3a616c
Quoted 9 lines · On Tuesday 19 May 2015 13:03:06 Tina Ruchandani wrote:
'struct timeval' uses 32-bit representation for seconds which will overflow in year 2038 and beyond.
mISDN/clock.cneeds to compute and store elapsed time in intervals of 125 microseconds. This patch replaces the usage of 'struct timeval' with 64-bitktime_twhich is y2038 safe. The patch also replacesdo_gettimeofday()(wall-clock time) withktime_get()(monotonic time) since we only care about elapsed time here.Signed-off-by Tina Ruchandani <[email protected]>Suggested-by Arnd Bergmnann <[email protected]>
Looks mostly ok now, just one detail left:
Review
Quoted 1 line
(quoted diff) +1
-0+ delta = ktime_us_delta(tv_now, iclock_tv) / 125;
This line is something I suggested, but I have now realized that it's still wrong, because you introduce a 64-by-32-bit division here that will fail to link on most 32-bit architectures.
Using a cast to 32-bit value would solve that, like
delta = ((u32)ktime_us_delta(tv_now, iclock_tv)) / 125;
as that would replace the 64-bit division with a 32-bit division. This is fine as long as the delta is never more than 71 minutes, otherwise you have to use
delta = ktime_divns(ktime_sub(tv_now, iclock_tv), (NS_PER_SEC / 8000));
which is also potentially more efficient.
Arnd