Hello, Arnd Thank you for your comments first.
> 在 2015年9月14日，16:19，Arnd Bergmann <[email protected]> 写道： > > On Sunday 13 September 2015 18:35:40 WEN Pingbo wrote: >> - what is the time_t/timeval/timespec type used for in this driver >> the timeval in dummy_g_get_frame is used for getting frame number in every >> ms. >> >> - is this broken in 2038 or not >> No. The millisecond of the last second will be normal if tv_sec is >> overflowed. But for consistency purpose, and avoiding further risks, I >> have replaced timeval with timespec64. > > Hi Pingbo, > > The patch and your findings so far look good. As a changelog comment > however, the text needs improvement and should not be written in > question/answer style. Instead, try to come up with complete sentences > that explain the current state of the driver, why we want to change > it and how your patch addresses the problem. > > It's quite likely that I will keep commenting mostly on your > changelog texts, but they are immensely important to get right > if you want to get patches merged. Basically you need to "sell" your > patch to the maintainer and explain why they really want to apply > it, when it's easier for them to not touch the driver to avoid > introducing a regression when they find an excuse not to apply the > patch. ;-) I just copied the IRC text here;-) I will rearrange the commit msg, and send it upstream. Hope I can “sell” it out. > >> - does the driver use monotonic or real time >> Real time. But only microsecond part is used. >> >> - if it uses real time, should it use monotonic time instead >> Monotonic time will be ok if it can meet the precise requirement(us). > > Your patch picks the easy approach by leaving the driver to use real > time. This clearly has a lower risk of regressions, which is good, but > you should come up with an argument on which of the two is the better > choice here. > Yes, I just follow the old way to avoid additional risks. If using monotonic time here, maybe we can replace the whole function as: return jiffies % HZ; But we need some tricks here to make sure the return value is correct, if HZ macro is greater than 1000 in some platform. > As a background: > > - The precision of real time and monotonic time is identical > > - Both the microsecond/nanosecond and the second portion are different, > as monotonic time starts at the moment that the machine is booted, > and there is a constant offset between the two. > > - Whenever a time value has to be synchronized between different machines, > we have to use real time (this does not seem to be the case here). > > - "real" time can jump forward or backward in some cases. If any of these > would cause problems in the driver, you should use monotonic time instead: > - root calls "settimeofday" > - ntp sets the time because of an overly large offset to the > official time > - a "leap second" occurs > > - both real and monotonic time can be accelerated or slowed down by ntp, > in order to stay in sync with the network time. If you instead need the > time to tick in sync with the CPU or bus clock generator, there is > "monotonic_raw" time. We rarely need this. > > - If there are multiple code locations that access the same time fields, > they obviously need to use the same time base. > > Arnd > >> Signed-off-by: WEN Pingbo <[email protected]> >> Cc: Y2038 <[email protected]> >> --- >> drivers/usb/gadget/udc/dummy_hcd.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c >> b/drivers/usb/gadget/udc/dummy_hcd.c >> index 1379ad4..7be721dad 100644 >> --- a/drivers/usb/gadget/udc/dummy_hcd.c >> +++ b/drivers/usb/gadget/udc/dummy_hcd.c >> @@ -833,10 +833,10 @@ static const struct usb_ep_ops dummy_ep_ops = { >> /* there are both host and device side versions of this call ... */ >> static int dummy_g_get_frame(struct usb_gadget *_gadget) >> { >> - struct timeval tv; >> + struct timespec64 tv; >> >> - do_gettimeofday(&tv); >> - return tv.tv_usec / 1000; >> + getnstimeofday64(&tv); >> + return tv.tv_nsec / 1000000L; >> } >> >> static int dummy_wakeup(struct usb_gadget *_gadget) >> > _______________________________________________ Y2038 mailing list [email protected] https://lists.linaro.org/mailman/listinfo/y2038
