- 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.
- 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).
Signed-off-by: WEN Pingbo pingbo.wen@linaro.org Cc: Y2038 y2038@lists.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..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)
add Arnd.
On Sunday, September 13, 2015 06:35 PM, 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.
- 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).
Signed-off-by: WEN Pingbo pingbo.wen@linaro.org Cc: Y2038 y2038@lists.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..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)
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. ;-)
- 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.
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 pingbo.wen@linaro.org Cc: Y2038 y2038@lists.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..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)
Hello, Arnd
Thank you for your comments first.
在 2015年9月14日，16:19，Arnd Bergmann arnd@arndb.de 写道：
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 pingbo.wen@linaro.org Cc: Y2038 y2038@lists.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..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)
On Tuesday 15 September 2015 09:48:10 Pingbo Wen wrote:
- 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.
On most architectures, HZ is between 100 and 1000, I believe Alpha is the only exception that goes as high as 1200. However, there is also a jiffies_to_msecs() function that might do the exact right thing and is normally very efficient.
The main downsides are that it loses precision on architectures with HZ=100, and that it will jump once every 41 days on architectures with HZ=1000 when jiffies overflows, as ULONG_MAX is not a multiple of 1000.
Do you know the specific requirement on the USB frame numbers? I don't know enough about USB to tell if either of those matter.
Arnd
On Tuesday, September 15, 2015 02:38 PM, Arnd Bergmann wrote:
On Tuesday 15 September 2015 09:48:10 Pingbo Wen wrote:
- 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.
On most architectures, HZ is between 100 and 1000, I believe Alpha is the only exception that goes as high as 1200. However, there is also a jiffies_to_msecs() function that might do the exact right thing and is normally very efficient.
The main downsides are that it loses precision on architectures with HZ=100, and that it will jump once every 41 days on architectures with HZ=1000 when jiffies overflows, as ULONG_MAX is not a multiple of 1000.
Do you know the specific requirement on the USB frame numbers? I don't know enough about USB to tell if either of those matter.
Using jiffies_to_msecs() here is a nice way. According to USB 2.0 Spec, however, the frame time is 1ms in full / low-speed, and 125us in high-speed. Actually, most of HCD implement this in hardware, the driver just read a register and return its value. It's hard to cover all platform here if we only use kernel time wheel.
Pingbo
On Tuesday 15 September 2015 20:10:10 Pingbo Wen wrote:
On Tuesday, September 15, 2015 02:38 PM, Arnd Bergmann wrote:
On Tuesday 15 September 2015 09:48:10 Pingbo Wen wrote:
- 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.
On most architectures, HZ is between 100 and 1000, I believe Alpha is the only exception that goes as high as 1200. However, there is also a jiffies_to_msecs() function that might do the exact right thing and is normally very efficient.
The main downsides are that it loses precision on architectures with HZ=100, and that it will jump once every 41 days on architectures with HZ=1000 when jiffies overflows, as ULONG_MAX is not a multiple of 1000.
Do you know the specific requirement on the USB frame numbers? I don't know enough about USB to tell if either of those matter.
Using jiffies_to_msecs() here is a nice way. According to USB 2.0 Spec, however, the frame time is 1ms in full / low-speed, and 125us in high-speed. Actually, most of HCD implement this in hardware, the driver just read a register and return its value. It's hard to cover all platform here if we only use kernel time wheel.
ktime_get() should have a high enough resolution to cover the high-speed mode, but that would also mean changing the driver more fundamentally.
The dummy driver does support emulating both superspeed and highspeed USB, so that may indeed be the right solution. At this point, we probably need Felipe, Alan and the linux-usb list to make a decision, so I've added them to Cc.
My impression from your comment is that the driver should check the dummy_hcd_module_parameters variable for the device speed in order to pick the right time base for the frame number, and then calculate it using ktime_divns(ktime_get()) with an appropriate factor.
Arnd
On Tue, 15 Sep 2015, Arnd Bergmann wrote:
Do you know the specific requirement on the USB frame numbers? I don't know enough about USB to tell if either of those matter.
Using jiffies_to_msecs() here is a nice way. According to USB 2.0 Spec, however, the frame time is 1ms in full / low-speed, and 125us in high-speed.
That's not quite right. A frame is always 1 ms, no matter what the speed. In high-speed and SuperSpeed, each 125-us period is called a micro-frame.
Actually, most of HCD implement this in hardware, the driver just read a register and return its value. It's hard to cover all platform here if we only use kernel time wheel.
ktime_get() should have a high enough resolution to cover the high-speed mode, but that would also mean changing the driver more fundamentally.
The dummy driver does support emulating both superspeed and highspeed USB, so that may indeed be the right solution. At this point, we probably need Felipe, Alan and the linux-usb list to make a decision, so I've added them to Cc.
My impression from your comment is that the driver should check the dummy_hcd_module_parameters variable for the device speed in order to pick the right time base for the frame number, and then calculate it using ktime_divns(ktime_get()) with an appropriate factor.
The right place to get the speed would be hcd->speed (in dummy_h_get_frame) and _gadget->speed (in dummy_g_get_frame). The module parameters affect what speeds are allowed but they don't fully determine the actual speed. However this is moot, since the API always returns the frame number, not the micro-frame number.
I agree that the monotonic clock would be better than the real-time clock.
On the other hand, this probably doesn't matter much. Frame numbers generally aren't useful for anything other than isochronous transfers, and dummy-hcd doesn't support isochronous.
Alan Stern
On Tuesday 15 September 2015 10:40:24 Alan Stern wrote:
On Tue, 15 Sep 2015, Arnd Bergmann wrote:
Do you know the specific requirement on the USB frame numbers? I don't know enough about USB to tell if either of those matter.
Using jiffies_to_msecs() here is a nice way. According to USB 2.0 Spec, however, the frame time is 1ms in full / low-speed, and 125us in high-speed.
That's not quite right. A frame is always 1 ms, no matter what the speed. In high-speed and SuperSpeed, each 125-us period is called a micro-frame.
Actually, most of HCD implement this in hardware, the driver just read a register and return its value. It's hard to cover all platform here if we only use kernel time wheel.
ktime_get() should have a high enough resolution to cover the high-speed mode, but that would also mean changing the driver more fundamentally.
The dummy driver does support emulating both superspeed and highspeed USB, so that may indeed be the right solution. At this point, we probably need Felipe, Alan and the linux-usb list to make a decision, so I've added them to Cc.
My impression from your comment is that the driver should check the dummy_hcd_module_parameters variable for the device speed in order to pick the right time base for the frame number, and then calculate it using ktime_divns(ktime_get()) with an appropriate factor.
The right place to get the speed would be hcd->speed (in dummy_h_get_frame) and _gadget->speed (in dummy_g_get_frame). The module parameters affect what speeds are allowed but they don't fully determine the actual speed. However this is moot, since the API always returns the frame number, not the micro-frame number.
Ok, thanks for the clarification!
I agree that the monotonic clock would be better than the real-time clock.
On the other hand, this probably doesn't matter much. Frame numbers generally aren't useful for anything other than isochronous transfers, and dummy-hcd doesn't support isochronous.
Ok. I'd suggest that Pingbo uses ktime_get_ts64() for the next version then, and add an explanation why it is likely irrelevant. By now, the main advantage I see in using monotonic time is that it might save someone from having to look at this file when searching for possible leap second issues, but that's still better than nothing.
Arnd
On Tuesday, September 15, 2015 10:49 PM, Arnd Bergmann wrote:
On Tuesday 15 September 2015 10:40:24 Alan Stern wrote:
On Tue, 15 Sep 2015, Arnd Bergmann wrote:
Do you know the specific requirement on the USB frame numbers? I don't know enough about USB to tell if either of those matter.
Using jiffies_to_msecs() here is a nice way. According to USB 2.0 Spec, however, the frame time is 1ms in full / low-speed, and 125us in high-speed.
That's not quite right. A frame is always 1 ms, no matter what the speed. In high-speed and SuperSpeed, each 125-us period is called a micro-frame.
Actually, most of HCD implement this in hardware, the driver just read a register and return its value. It's hard to cover all platform here if we only use kernel time wheel.
ktime_get() should have a high enough resolution to cover the high-speed mode, but that would also mean changing the driver more fundamentally.
The dummy driver does support emulating both superspeed and highspeed USB, so that may indeed be the right solution. At this point, we probably need Felipe, Alan and the linux-usb list to make a decision, so I've added them to Cc.
My impression from your comment is that the driver should check the dummy_hcd_module_parameters variable for the device speed in order to pick the right time base for the frame number, and then calculate it using ktime_divns(ktime_get()) with an appropriate factor.
The right place to get the speed would be hcd->speed (in dummy_h_get_frame) and _gadget->speed (in dummy_g_get_frame). The module parameters affect what speeds are allowed but they don't fully determine the actual speed. However this is moot, since the API always returns the frame number, not the micro-frame number.
Ok, thanks for the clarification!
I agree that the monotonic clock would be better than the real-time clock.
On the other hand, this probably doesn't matter much. Frame numbers generally aren't useful for anything other than isochronous transfers, and dummy-hcd doesn't support isochronous.
Ok. I'd suggest that Pingbo uses ktime_get_ts64() for the next version then, and add an explanation why it is likely irrelevant. By now, the main advantage I see in using monotonic time is that it might save someone from having to look at this file when searching for possible leap second issues, but that's still better than nothing.
I will fix it in next version.
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.
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)
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)
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
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
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
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)
-
Alan Stern -
Arnd Bergmann -
Peter Stuge -
Pingbo Wen -
WEN Pingbo