struct timeval uses a 32-bit seconds representation which will overflow in the year 2038 and beyond. This patch replaces the usage of struct timeval with ktime_t which is a 64-bit timestamp and is year 2038 safe. This patch is part of a larger attempt to remove all instances of 32-bit timekeeping variables (timeval, timespec, time_t) which are not year 2038 safe, from the kernel. The patch is a work-in-progress - correctness of the following changes is unclear: (a) Usage of timeval_usec_diff - The function seems to subtract usec values without caring about the difference of the seconds field. There may be an implicit assumption in the original code that the time delta is always of the order of microseconds. The patch replaces the usage of timeval_usec_diff with ktime_to_us(ktime_sub()) which computes the real timestamp difference, not just the difference in the usec field. (b) printk diffing the tv[i] and tv[i-1] values. The original printk statement seems to get the order wrong. This patch preserves that order.
Signed-off-by: Tina Ruchandani ruchandani.tina@gmail.com --- drivers/media/dvb-core/dvb_frontend.c | 46 ++++++++++++----------------------- drivers/media/dvb-core/dvb_frontend.h | 3 +-- drivers/media/dvb-frontends/stv0299.c | 15 ++++++------ 3 files changed, 24 insertions(+), 40 deletions(-)
diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c index 882ca41..48c3daf 100644 --- a/drivers/media/dvb-core/dvb_frontend.c +++ b/drivers/media/dvb-core/dvb_frontend.c @@ -40,6 +40,7 @@ #include <linux/freezer.h> #include <linux/jiffies.h> #include <linux/kthread.h> +#include <linux/ktime.h> #include <asm/processor.h>
#include "dvb_frontend.h" @@ -889,42 +890,25 @@ static void dvb_frontend_stop(struct dvb_frontend *fe) fepriv->thread); }
-s32 timeval_usec_diff(struct timeval lasttime, struct timeval curtime) -{ - return ((curtime.tv_usec < lasttime.tv_usec) ? - 1000000 - lasttime.tv_usec + curtime.tv_usec : - curtime.tv_usec - lasttime.tv_usec); -} -EXPORT_SYMBOL(timeval_usec_diff); - -static inline void timeval_usec_add(struct timeval *curtime, u32 add_usec) -{ - curtime->tv_usec += add_usec; - if (curtime->tv_usec >= 1000000) { - curtime->tv_usec -= 1000000; - curtime->tv_sec++; - } -} - /* * Sleep until gettimeofday() > waketime + add_usec * This needs to be as precise as possible, but as the delay is * usually between 2ms and 32ms, it is done using a scheduled msleep * followed by usleep (normally a busy-wait loop) for the remainder */ -void dvb_frontend_sleep_until(struct timeval *waketime, u32 add_usec) +void dvb_frontend_sleep_until(ktime_t waketime, u32 add_usec) { - struct timeval lasttime; + ktime_t lasttime; s32 delta, newdelta;
- timeval_usec_add(waketime, add_usec); + ktime_add_us(waketime, add_usec);
- do_gettimeofday(&lasttime); - delta = timeval_usec_diff(lasttime, *waketime); + lasttime = ktime_get_real(); + delta = ktime_to_us(ktime_sub(lasttime, waketime)); if (delta > 2500) { msleep((delta - 1500) / 1000); - do_gettimeofday(&lasttime); - newdelta = timeval_usec_diff(lasttime, *waketime); + lasttime = ktime_get_real(); + newdelta = ktime_to_us(ktime_sub(lasttime, waketime)); delta = (newdelta > delta) ? 0 : newdelta; } if (delta > 0) @@ -2458,24 +2442,24 @@ static int dvb_frontend_ioctl_legacy(struct file *file, * include the initialization or start bit */ unsigned long swcmd = ((unsigned long) parg) << 1; - struct timeval nexttime; - struct timeval tv[10]; + ktime_t nexttime; + ktime_t tv[10]; int i; u8 last = 1; if (dvb_frontend_debug) printk("%s switch command: 0x%04lx\n", __func__, swcmd); - do_gettimeofday(&nexttime); + nexttime = ktime_get_real(); if (dvb_frontend_debug) tv[0] = nexttime; /* before sending a command, initialize by sending * a 32ms 18V to the switch */ fe->ops.set_voltage(fe, SEC_VOLTAGE_18); - dvb_frontend_sleep_until(&nexttime, 32000); + dvb_frontend_sleep_until(nexttime, 32000);
for (i = 0; i < 9; i++) { if (dvb_frontend_debug) - do_gettimeofday(&tv[i + 1]); + tv[i+1] = ktime_get_real(); if ((swcmd & 0x01) != last) { /* set voltage to (last ? 13V : 18V) */ fe->ops.set_voltage(fe, (last) ? SEC_VOLTAGE_13 : SEC_VOLTAGE_18); @@ -2483,13 +2467,13 @@ static int dvb_frontend_ioctl_legacy(struct file *file, } swcmd = swcmd >> 1; if (i != 8) - dvb_frontend_sleep_until(&nexttime, 8000); + dvb_frontend_sleep_until(nexttime, 8000); } if (dvb_frontend_debug) { printk("%s(%d): switch delay (should be 32k followed by all 8k\n", __func__, fe->dvb->num); for (i = 1; i < 10; i++) - printk("%d: %d\n", i, timeval_usec_diff(tv[i-1] , tv[i])); + printk("%d: %d\n", i, (int) ktime_to_us(ktime_sub(tv[i-1], tv[i]))); } err = 0; fepriv->state = FESTATE_DISEQC; diff --git a/drivers/media/dvb-core/dvb_frontend.h b/drivers/media/dvb-core/dvb_frontend.h index 816269e..5ddd55f 100644 --- a/drivers/media/dvb-core/dvb_frontend.h +++ b/drivers/media/dvb-core/dvb_frontend.h @@ -439,7 +439,6 @@ extern void dvb_frontend_reinitialise(struct dvb_frontend *fe); extern int dvb_frontend_suspend(struct dvb_frontend *fe); extern int dvb_frontend_resume(struct dvb_frontend *fe);
-extern void dvb_frontend_sleep_until(struct timeval *waketime, u32 add_usec); -extern s32 timeval_usec_diff(struct timeval lasttime, struct timeval curtime); +extern void dvb_frontend_sleep_until(ktime_t waketime, u32 add_usec);
#endif diff --git a/drivers/media/dvb-frontends/stv0299.c b/drivers/media/dvb-frontends/stv0299.c index b57ecf4..8d30865 100644 --- a/drivers/media/dvb-frontends/stv0299.c +++ b/drivers/media/dvb-frontends/stv0299.c @@ -44,6 +44,7 @@
#include <linux/init.h> #include <linux/kernel.h> +#include <linux/ktime.h> #include <linux/module.h> #include <linux/string.h> #include <linux/slab.h> @@ -404,8 +405,8 @@ static int stv0299_send_legacy_dish_cmd (struct dvb_frontend* fe, unsigned long u8 lv_mask = 0x40; u8 last = 1; int i; - struct timeval nexttime; - struct timeval tv[10]; + ktime_t nexttime; + ktime_t tv[10];
reg0x08 = stv0299_readreg (state, 0x08); reg0x0c = stv0299_readreg (state, 0x0c); @@ -418,16 +419,16 @@ static int stv0299_send_legacy_dish_cmd (struct dvb_frontend* fe, unsigned long if (debug_legacy_dish_switch) printk ("%s switch command: 0x%04lx\n",__func__, cmd);
- do_gettimeofday (&nexttime); + nexttime = ktime_get_real(); if (debug_legacy_dish_switch) tv[0] = nexttime; stv0299_writeregI (state, 0x0c, reg0x0c | 0x50); /* set LNB to 18V */
- dvb_frontend_sleep_until(&nexttime, 32000); + dvb_frontend_sleep_until(nexttime, 32000);
for (i=0; i<9; i++) { if (debug_legacy_dish_switch) - do_gettimeofday (&tv[i+1]); + tv[i+1] = ktime_get_real(); if((cmd & 0x01) != last) { /* set voltage to (last ? 13V : 18V) */ stv0299_writeregI (state, 0x0c, reg0x0c | (last ? lv_mask : 0x50)); @@ -437,13 +438,13 @@ static int stv0299_send_legacy_dish_cmd (struct dvb_frontend* fe, unsigned long cmd = cmd >> 1;
if (i != 8) - dvb_frontend_sleep_until(&nexttime, 8000); + dvb_frontend_sleep_until(nexttime, 8000); } if (debug_legacy_dish_switch) { printk ("%s(%d): switch delay (should be 32k followed by all 8k\n", __func__, fe->dvb->num); for (i = 1; i < 10; i++) - printk ("%d: %d\n", i, timeval_usec_diff(tv[i-1] , tv[i])); + printk("%d: %d\n", i, (int) ktime_to_us(ktime_sub(tv[i-1], tv[i]))); }
return 0;
On 05/11/2015 04:37 AM, Tina Ruchandani wrote:
struct timeval uses a 32-bit seconds representation which will overflow in the year 2038 and beyond. This patch replaces the usage of struct timeval with ktime_t which is a 64-bit timestamp and is year 2038 safe. This patch is part of a larger attempt to remove all instances of 32-bit timekeeping variables (timeval, timespec, time_t) which are not year 2038 safe, from the kernel. The patch is a work-in-progress - correctness of the following changes is unclear: (a) Usage of timeval_usec_diff - The function seems to subtract usec values without caring about the difference of the seconds field. There may be an implicit assumption in the original code that the time delta is always of the order of microseconds. The patch replaces the usage of timeval_usec_diff with ktime_to_us(ktime_sub()) which computes the real timestamp difference, not just the difference in the usec field. (b) printk diffing the tv[i] and tv[i-1] values. The original printk statement seems to get the order wrong. This patch preserves that order.
Signed-off-by: Tina Ruchandani ruchandani.tina@gmail.com
Hi Tina!
Please CC patches for anything under drivers/media and/or include/media to the linux-media mailinglist. That's where the experts on these drivers are and they will be able to tell you if it is OK or not.
I suspect that in both cases it may well be a bug.
Regards,
Hans
drivers/media/dvb-core/dvb_frontend.c | 46 ++++++++++++----------------------- drivers/media/dvb-core/dvb_frontend.h | 3 +-- drivers/media/dvb-frontends/stv0299.c | 15 ++++++------ 3 files changed, 24 insertions(+), 40 deletions(-)
diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c index 882ca41..48c3daf 100644 --- a/drivers/media/dvb-core/dvb_frontend.c +++ b/drivers/media/dvb-core/dvb_frontend.c @@ -40,6 +40,7 @@ #include <linux/freezer.h> #include <linux/jiffies.h> #include <linux/kthread.h> +#include <linux/ktime.h> #include <asm/processor.h> #include "dvb_frontend.h" @@ -889,42 +890,25 @@ static void dvb_frontend_stop(struct dvb_frontend *fe) fepriv->thread); } -s32 timeval_usec_diff(struct timeval lasttime, struct timeval curtime) -{
- return ((curtime.tv_usec < lasttime.tv_usec) ?
1000000 - lasttime.tv_usec + curtime.tv_usec :curtime.tv_usec - lasttime.tv_usec);-} -EXPORT_SYMBOL(timeval_usec_diff);
-static inline void timeval_usec_add(struct timeval *curtime, u32 add_usec) -{
- curtime->tv_usec += add_usec;
- if (curtime->tv_usec >= 1000000) {
curtime->tv_usec -= 1000000;curtime->tv_sec++;- }
-}
/*
- Sleep until gettimeofday() > waketime + add_usec
- This needs to be as precise as possible, but as the delay is
- usually between 2ms and 32ms, it is done using a scheduled msleep
- followed by usleep (normally a busy-wait loop) for the remainder
*/ -void dvb_frontend_sleep_until(struct timeval *waketime, u32 add_usec) +void dvb_frontend_sleep_until(ktime_t waketime, u32 add_usec) {
- struct timeval lasttime;
- ktime_t lasttime; s32 delta, newdelta;
- timeval_usec_add(waketime, add_usec);
- ktime_add_us(waketime, add_usec);
- do_gettimeofday(&lasttime);
- delta = timeval_usec_diff(lasttime, *waketime);
- lasttime = ktime_get_real();
- delta = ktime_to_us(ktime_sub(lasttime, waketime)); if (delta > 2500) { msleep((delta - 1500) / 1000);
do_gettimeofday(&lasttime);newdelta = timeval_usec_diff(lasttime, *waketime);
lasttime = ktime_get_real();newdelta = ktime_to_us(ktime_sub(lasttime, waketime));@@ -2458,24 +2442,24 @@ static int dvb_frontend_ioctl_legacy(struct file *file, * include the initialization or start bit */ unsigned long swcmd = ((unsigned long) parg) << 1;
struct timeval nexttime;struct timeval tv[10];
ktime_t nexttime;ktime_t tv[10]; int i; u8 last = 1; if (dvb_frontend_debug) printk("%s switch command: 0x%04lx\n", __func__, swcmd);
do_gettimeofday(&nexttime);
nexttime = ktime_get_real(); if (dvb_frontend_debug) tv[0] = nexttime; /* before sending a command, initialize by sending * a 32ms 18V to the switch */ fe->ops.set_voltage(fe, SEC_VOLTAGE_18);
dvb_frontend_sleep_until(&nexttime, 32000);
dvb_frontend_sleep_until(nexttime, 32000);for (i = 0; i < 9; i++) { if (dvb_frontend_debug)
do_gettimeofday(&tv[i + 1]);
tv[i+1] = ktime_get_real(); if ((swcmd & 0x01) != last) { /* set voltage to (last ? 13V : 18V) */ fe->ops.set_voltage(fe, (last) ? SEC_VOLTAGE_13 : SEC_VOLTAGE_18);@@ -2483,13 +2467,13 @@ static int dvb_frontend_ioctl_legacy(struct file *file, } swcmd = swcmd >> 1; if (i != 8)
dvb_frontend_sleep_until(&nexttime, 8000);
dvb_frontend_sleep_until(nexttime, 8000); } if (dvb_frontend_debug) { printk("%s(%d): switch delay (should be 32k followed by all 8k\n", __func__, fe->dvb->num); for (i = 1; i < 10; i++)
printk("%d: %d\n", i, timeval_usec_diff(tv[i-1] , tv[i]));
printk("%d: %d\n", i, (int) ktime_to_us(ktime_sub(tv[i-1], tv[i]))); } err = 0; fepriv->state = FESTATE_DISEQC;diff --git a/drivers/media/dvb-core/dvb_frontend.h b/drivers/media/dvb-core/dvb_frontend.h index 816269e..5ddd55f 100644 --- a/drivers/media/dvb-core/dvb_frontend.h +++ b/drivers/media/dvb-core/dvb_frontend.h @@ -439,7 +439,6 @@ extern void dvb_frontend_reinitialise(struct dvb_frontend *fe); extern int dvb_frontend_suspend(struct dvb_frontend *fe); extern int dvb_frontend_resume(struct dvb_frontend *fe); -extern void dvb_frontend_sleep_until(struct timeval *waketime, u32 add_usec); -extern s32 timeval_usec_diff(struct timeval lasttime, struct timeval curtime); +extern void dvb_frontend_sleep_until(ktime_t waketime, u32 add_usec); #endif diff --git a/drivers/media/dvb-frontends/stv0299.c b/drivers/media/dvb-frontends/stv0299.c index b57ecf4..8d30865 100644 --- a/drivers/media/dvb-frontends/stv0299.c +++ b/drivers/media/dvb-frontends/stv0299.c @@ -44,6 +44,7 @@ #include <linux/init.h> #include <linux/kernel.h> +#include <linux/ktime.h> #include <linux/module.h> #include <linux/string.h> #include <linux/slab.h> @@ -404,8 +405,8 @@ static int stv0299_send_legacy_dish_cmd (struct dvb_frontend* fe, unsigned long u8 lv_mask = 0x40; u8 last = 1; int i;
- struct timeval nexttime;
- struct timeval tv[10];
- ktime_t nexttime;
- ktime_t tv[10];
reg0x08 = stv0299_readreg (state, 0x08); reg0x0c = stv0299_readreg (state, 0x0c); @@ -418,16 +419,16 @@ static int stv0299_send_legacy_dish_cmd (struct dvb_frontend* fe, unsigned long if (debug_legacy_dish_switch) printk ("%s switch command: 0x%04lx\n",__func__, cmd);
- do_gettimeofday (&nexttime);
- nexttime = ktime_get_real(); if (debug_legacy_dish_switch) tv[0] = nexttime; stv0299_writeregI (state, 0x0c, reg0x0c | 0x50); /* set LNB to 18V */
- dvb_frontend_sleep_until(&nexttime, 32000);
- dvb_frontend_sleep_until(nexttime, 32000);
for (i=0; i<9; i++) { if (debug_legacy_dish_switch)
do_gettimeofday (&tv[i+1]);
tv[i+1] = ktime_get_real();@@ -437,13 +438,13 @@ static int stv0299_send_legacy_dish_cmd (struct dvb_frontend* fe, unsigned long cmd = cmd >> 1; if (i != 8)
dvb_frontend_sleep_until(&nexttime, 8000);
dvb_frontend_sleep_until(nexttime, 8000);
printk ("%d: %d\n", i, timeval_usec_diff(tv[i-1] , tv[i]));
printk("%d: %d\n", i, (int) ktime_to_us(ktime_sub(tv[i-1], tv[i])));return 0;
On Monday 11 May 2015 08:07:51 Tina Ruchandani wrote:
struct timeval uses a 32-bit seconds representation which will overflow in the year 2038 and beyond. This patch replaces the usage of struct timeval with ktime_t which is a 64-bit timestamp and is year 2038 safe. This patch is part of a larger attempt to remove all instances of 32-bit timekeeping variables (timeval, timespec, time_t) which are not year 2038 safe, from the kernel. The patch is a work-in-progress - correctness of the following changes is unclear: (a) Usage of timeval_usec_diff - The function seems to subtract usec values without caring about the difference of the seconds field. There may be an implicit assumption in the original code that the time delta is always of the order of microseconds.
Right, in particular, it does handle the seconds field overflowing once, but it breaks when the difference is larger than one second.
The patch replaces the usage of timeval_usec_diff with ktime_to_us(ktime_sub()) which computes the real timestamp difference, not just the difference in the usec field.
As in the other patches, ktime_us_delta() would look nicer, but it's equally correct.
- Sleep until gettimeofday() > waketime + add_usec
- This needs to be as precise as possible, but as the delay is
- usually between 2ms and 32ms, it is done using a scheduled msleep
- followed by usleep (normally a busy-wait loop) for the remainder
*/ -void dvb_frontend_sleep_until(struct timeval *waketime, u32 add_usec) +void dvb_frontend_sleep_until(ktime_t waketime, u32 add_usec) {
- struct timeval lasttime;
- ktime_t lasttime; s32 delta, newdelta;
- timeval_usec_add(waketime, add_usec);
- ktime_add_us(waketime, add_usec);
- do_gettimeofday(&lasttime);
- delta = timeval_usec_diff(lasttime, *waketime);
- lasttime = ktime_get_real();
- delta = ktime_to_us(ktime_sub(lasttime, waketime)); if (delta > 2500) { msleep((delta - 1500) / 1000);
do_gettimeofday(&lasttime);newdelta = timeval_usec_diff(lasttime, *waketime);
lasttime = ktime_get_real();newdelta = ktime_to_us(ktime_sub(lasttime, waketime));
There is a bug you introduced: The original function added add_usec to the value pointed to by waketime. Changing this from a pointer to a scalar argument means that the value is only updated in the dvb_frontend_sleep_until() function, but not inside of the caller.
In order to preserve the behavior, you have to leave waketime as a pointer.
Arnd
-
Arnd Bergmann -
Hans Verkuil -
Tina Ruchandani