diff mbox

isdn: Use ktime_t instead of 'struct timeval'

Message ID 20150511023257.GA2612@tinar
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Tina Ruchandani May 11, 2015, 2:32 a.m. UTC 
'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 use of wall-clock time (do_gettimeofday) with 
monotonic time (ktime_get) since we only care about elapsed time here.

Signed-off-by: Tina Ruchandani <ruchandani.tina@gmail.com>
---
 drivers/isdn/mISDN/clock.c | 53 +++++++++++++++++-----------------------------
 include/linux/mISDNif.h    |  2 +-
 2 files changed, 21 insertions(+), 34 deletions(-)

Comments

Arnd Bergmann May 11, 2015, 3:31 p.m. UTC | #1 
On Monday 11 May 2015 08:02:57 Tina Ruchandani wrote:

> -			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();

A minor coding style comment here: If you have a multi-line
block after if() using curly braces, then you should also use
that for the else path, even if that is just one line.

However, if both sides do not need curly braces, leave them
out entirely.

> +		delta = ktime_to_us(ktime_sub(tv_now, iclock_tv));

We have a ktime_us_delta() function now that would simplify this
slightly.

> +		delta = delta / 125;

This looks like a possible bug: delta is a 16-bit number, so it will
be truncated to 65536 microseconds when assigning the result of
ktime_us_delta() or ktime_to_us(), and then you divide by 125, so
it will now be a number that is smaller than 524, and if the elapsed
time is more than 65536 microseconds, you can an incorrect result.

You can avoid that by using a 32-bit temporary, or doing it
implicitly as

	delta = ktime_us_delta(tv_now, iclock_tv) / 125;

Another option would be to calculate the right number from the
nanoseconds:

	delta = ktime_divns(ktime_sub(tv_now, iclock_tv), (NS_PER_SEC / 8000));

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/isdn/mISDN/clock.c b/drivers/isdn/mISDN/clock.c
index 693fb7c..7550f2e 100644
--- a/drivers/isdn/mISDN/clock.c
+++ b/drivers/isdn/mISDN/clock.c
@@ -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,20 @@  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;
+			iclock_tv = *tv;
 		} else
-			do_gettimeofday(&iclock_tv);
+			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;
+			tv_now = *tv;
 		} 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 = ktime_get();
+		delta = ktime_to_us(ktime_sub(tv_now, iclock_tv));
+		delta = delta / 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 +187,17 @@  unsigned short
 mISDN_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_to_us(ktime_sub(tv_now, iclock_tv));
+	delta = delta / 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;
 }
diff --git a/include/linux/mISDNif.h b/include/linux/mISDNif.h
index 246a352..ac02c54 100644
--- a/include/linux/mISDNif.h
+++ b/include/linux/mISDNif.h
@@ -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 *);