On Tue, Jan 05, 2016 at 08:27:09AM -0800, Deepa Dinamani wrote:
On Mon, Jan 4, 2016 at 11:27 AM, Alison Schofield amsfield22@gmail.com wrote:
resending for review...
On Fri, Nov 20, 2015 at 10:23:17AM -0800, Alison Schofield wrote:
divamnt stores a start_time at init and uses it to calculate elapsed time. These operations are all internal to divamnt.
Address struct timeval overflow on 32-bit systems by replacing: struct timeval with struct timespec64; do_gettimeofday() with getnstimeofday64(); the calculations that yield a safe and normalized elapsed time with timespec64_sub() which provides same.
Signed-off-by: Alison Schofield amsfield22@gmail.com
drivers/isdn/hardware/eicon/divamnt.c | 35
+++++++++++------------------------
1 file changed, 11 insertions(+), 24 deletions(-)
diff --git a/drivers/isdn/hardware/eicon/divamnt.c
b/drivers/isdn/hardware/eicon/divamnt.c
index 48db08d..7ac057e 100644 --- a/drivers/isdn/hardware/eicon/divamnt.c +++ b/drivers/isdn/hardware/eicon/divamnt.c @@ -45,7 +45,7 @@ char *DRIVERRELEASE_MNT = "2.0";
static wait_queue_head_t msgwaitq; static unsigned long opened; -static struct timeval start_time; +static struct timespec64 start_time;
extern int mntfunc_init(int *, void **, unsigned long); extern void mntfunc_finit(void); @@ -88,28 +88,15 @@ int diva_os_copy_from_user(void *os_handle, void
*dst, const void __user *src,
*/ void diva_os_get_time(dword *sec, dword *usec) {
struct timeval tv;do_gettimeofday(&tv);if (tv.tv_sec > start_time.tv_sec) {if (start_time.tv_usec > tv.tv_usec) {tv.tv_sec--;tv.tv_usec += 1000000;}*sec = (dword) (tv.tv_sec - start_time.tv_sec);*usec = (dword) (tv.tv_usec - start_time.tv_usec);} else if (tv.tv_sec == start_time.tv_sec) {*sec = 0;if (start_time.tv_usec < tv.tv_usec) {*usec = (dword) (tv.tv_usec - start_time.tv_usec);} else {*usec = 0;}} else {*sec = (dword) tv.tv_sec;*usec = (dword) tv.tv_usec;}timespec64_sub(lhs, rhs) assumes lhs > rhs always.
But, the original code doesn't seem to think this is always the case because of the else condition. The first 2 cases where start_time < curr_time, start_time == curr_time are handled in the change. But, think you have missed the case when start_time > curr_time?
struct timespec64 curr_time;struct timespec64 delta;getnstimeofday64(&curr_time);delta = timespec64_sub(curr_time, start_time);*sec = (dword) delta.tv_sec;*usec = (dword) (delta.tv_nsec / NSEC_PER_USEC);}
Here dword is defined as u32. Since this is only a difference in times, this might be warranted. But, the else part above might not fit in u32 because now you use 64 bit time. Maybe commit text should mention why this is still valid?
Since we are only concerned about difference monotonic times also should be okay.
Deepa, Thanks for the review. Your feedback led me to...
use monotonic time: The driver wants separate secs and usecs fields AND is only looking at delta so use ktime_get_ts64 to get that timespec64 struct. This removes the risk of start time being greater than current time.
uncover a small code cleanup: In further walkthrough of callers of diva_os_get_time() found redundant secs and usec constants declared and initialized using diva_os_get_time. I removed that code.
See what you think in version2. alison