On Tue, Jan 05, 2016 at 08:27:09AM -0800, Deepa Dinamani wrote:
> On Mon, Jan 4, 2016 at 11:27 AM, Alison Schofield <[email protected]>
> 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 <[email protected]>
> >> ---
> >> 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 <https://lists.linaro.org/mailman/listinfo/y2038>
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
_______________________________________________
Y2038 mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/y2038
