* Re: [PATCH V2] dummy_hcd: replace timeval with timespec64
2015-09-17 3:09 ` [PATCH V2] dummy_hcd: replace timeval with timespec64 WEN Pingbo
@ 2015-09-17 3:13 ` Pingbo Wen
2015-09-17 8:12 ` Arnd Bergmann
2015-09-17 9:59 ` [PATCH V2] " Peter Stuge
2 siblings, 0 replies; 6+ messages in thread
From: Pingbo Wen @ 2015-09-17 3:13 UTC (permalink / raw)
To: linux-usb; +Cc: Y2038, linux-kernel, Arnd Bergmann, Felipe Balbi, stern
add Alan Stern
On Thursday, September 17, 2015 11:09 AM, WEN Pingbo wrote:
> The millisecond of the last second will be normal if tv_sec is
> overflowed. But for y2038 consistency and demonstration purpose,
> and avoiding further risks, we need to remove 'timeval' in this
> driver, to avoid similair problems.
>
> V2 Updates:
> - using monotonic time here by replacing getnstimeofday() with
> ktime_get_ts64(), to avoid leap second issues. The frame time in USB
> is always 1ms, no matter what speed, so ktime_get_ts64() have enough
> resolution to cover this.
> - using NSEC_PER_MSEC instead of hard code.
>
> Signed-off-by: Pingbo Wen <pingbo.wen@linaro.org>
> Cc: Y2038 <y2038@lists.linaro.org>
> Cc: linux-kernel@vger.kernel.org
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Felipe Balbi <balbi@ti.com>
> Signed-off-by: WEN Pingbo <pingbo.wen@linaro.org>
> ---
> 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..6d1ed35 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;
> + ktime_get_ts64(&tv);
> + return tv.tv_nsec / NSEC_PER_MSEC;
> }
>
> static int dummy_wakeup(struct usb_gadget *_gadget)
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH V2] dummy_hcd: replace timeval with timespec64
2015-09-17 3:09 ` [PATCH V2] dummy_hcd: replace timeval with timespec64 WEN Pingbo
2015-09-17 3:13 ` Pingbo Wen
@ 2015-09-17 8:12 ` Arnd Bergmann
2015-09-18 2:51 ` [PATCH V3] " WEN Pingbo
2015-09-17 9:59 ` [PATCH V2] " Peter Stuge
2 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2015-09-17 8:12 UTC (permalink / raw)
To: WEN Pingbo; +Cc: linux-usb, Y2038, linux-kernel, Felipe Balbi
On Thursday 17 September 2015 11:09:49 WEN Pingbo wrote:
> The millisecond of the last second will be normal if tv_sec is
> overflowed. But for y2038 consistency and demonstration purpose,
> and avoiding further risks, we need to remove 'timeval' in this
> driver, to avoid similair problems.
>
> V2 Updates:
> - using monotonic time here by replacing getnstimeofday() with
> ktime_get_ts64(), to avoid leap second issues. The frame time in USB
> is always 1ms, no matter what speed, so ktime_get_ts64() have enough
> resolution to cover this.
> - using NSEC_PER_MSEC instead of hard code.
>
> Signed-off-by: Pingbo Wen <pingbo.wen@linaro.org>
> Cc: Y2038 <y2038@lists.linaro.org>
> Cc: linux-kernel@vger.kernel.org
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Felipe Balbi <balbi@ti.com>
> Signed-off-by: WEN Pingbo <pingbo.wen@linaro.org>
>
The patch looks good to me now,
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Regarding the changelog, I notice you have a duplicate Signed-off-by
line, one of the two should be removed. Also, The "V2 update" portion
should be split into the part that you want in the changelog, without
the '-' bullet points (in this case, the explanation for monontonic
time), and the purely informational part that makes sense for the
review but not for the git history (what changes were done against
previous versions of the patch), which should go below the '---'
line under the Signed-off-by chain, so it gets removed when imported
to git.
If Felipe decides to do these changes himself when he applies the
patch, that's fine, otherwise please send the patch again as 'V3'
tomorrow.
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V3] dummy_hcd: replace timeval with timespec64
2015-09-17 8:12 ` Arnd Bergmann
@ 2015-09-18 2:51 ` WEN Pingbo
0 siblings, 0 replies; 6+ messages in thread
From: WEN Pingbo @ 2015-09-18 2:51 UTC (permalink / raw)
To: linux-usb; +Cc: y2038, linux-kernel, arnd, balbi, peter, stern, WEN Pingbo
The millisecond of the last second will be normal if tv_sec is
overflowed. But for y2038 consistency and demonstration purpose,
and avoiding further risks, we need to remove 'timeval' in this
driver, to avoid similair problems.
Signed-off-by: Pingbo Wen <pingbo.wen@linaro.org>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
---
V3 Updates:
- using ts64 variable name to avoid confusion
V2 Updates:
- using monotonic time here by replacing getnstimeofday() with
ktime_get_ts64(), to avoid leap second issues. The frame time in USB
is always 1ms, no matter what speed, so ktime_get_ts64() have enough
resolution to cover this.
- using NSEC_PER_MSEC instead of hard code.
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..2ac9a13 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 ts64;
- do_gettimeofday(&tv);
- return tv.tv_usec / 1000;
+ ktime_get_ts64(&ts64);
+ return ts64.tv_nsec / NSEC_PER_MSEC;
}
static int dummy_wakeup(struct usb_gadget *_gadget)
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V2] dummy_hcd: replace timeval with timespec64
2015-09-17 3:09 ` [PATCH V2] dummy_hcd: replace timeval with timespec64 WEN Pingbo
2015-09-17 3:13 ` Pingbo Wen
2015-09-17 8:12 ` Arnd Bergmann
@ 2015-09-17 9:59 ` Peter Stuge
2015-09-17 11:42 ` Pingbo Wen
2 siblings, 1 reply; 6+ messages in thread
From: Peter Stuge @ 2015-09-17 9:59 UTC (permalink / raw)
To: WEN Pingbo; +Cc: linux-usb, Y2038, linux-kernel, Arnd Bergmann, Felipe Balbi
WEN Pingbo wrote:
> +++ 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;
tv is very often used for timeval structs.
I suggest also changing the variable name, for example to ts, to
avoid some possible confusion.
//Peter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH V2] dummy_hcd: replace timeval with timespec64
2015-09-17 9:59 ` [PATCH V2] " Peter Stuge
@ 2015-09-17 11:42 ` Pingbo Wen
0 siblings, 0 replies; 6+ messages in thread
From: Pingbo Wen @ 2015-09-17 11:42 UTC (permalink / raw)
To: peter; +Cc: linux-usb, Y2038, linux-kernel, Arnd Bergmann, Felipe Balbi
On Thursday, September 17, 2015 05:59 PM, Peter Stuge wrote:
> WEN Pingbo wrote:
>> +++ 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;
>
> tv is very often used for timeval structs.
>
> I suggest also changing the variable name, for example to ts, to
> avoid some possible confusion.
>
Hi Peter,
Thanks for pointing it out, I will fix it in next version.
Pingbo
^ permalink raw reply [flat|nested] 6+ messages in thread