[PATCH] dummy_hcd: replace timeval with timespec64

[PATCH] dummy_hcd: replace timeval with timespec64

[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 still need to fix it here,
to avoid similair problems.

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>
---
 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)
-- 
1.9.1

Re: [Y2038] [PATCH] dummy_hcd: replace timeval with timespec64

[Arnd Bergmann]

On Tuesday 15 September 2015 20:56:15 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 still need to fix it here,
> to avoid similair problems.
> 
> 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>

I just replied to the thread of the first mail, and it's good to see you had
the same thought about adding the usb folks to the Cc list.

When sending a new version of a patch you have already sent before, it's
common practice to use [PATCH v2] in the subject, so please do that for the
next version.

Your changelog text is better than the first version, but can still be improved.
I would at least mention that we want to remove all uses of 'timeval' from
device drivers in order to better scan for y2038 problems in the drivers that
still use them.

Also, you don't mention at all the discussion we had about real time vs.
monotonic time for this driver. I think using monotonic time (ktime_get_ts64())
would be more appropriate here, but whichever you choose, you should explain
in the changelog why you think that one is better than the other.
Most of the time, we end up changing from real time to monotonic time in the
same patch when converting a driver to avoid 32-bit time_t, so even you don't
change it, you should explain why not.

>  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;
>  }
>  

As in your other patch, I think the use of NSEC_PER_MSEC would make this
slightly more understandable.

	Arnd

Re: [Y2038] [PATCH] dummy_hcd: replace timeval with timespec64

[Pingbo Wen]

On Tuesday, September 15, 2015 09:14 PM, Arnd Bergmann wrote:
> On Tuesday 15 September 2015 20:56:15 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 still need to fix it here,
>> to avoid similair problems.
>>
>> 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>
> 
> I just replied to the thread of the first mail, and it's good to see you had
> the same thought about adding the usb folks to the Cc list.
> 
> When sending a new version of a patch you have already sent before, it's
> common practice to use [PATCH v2] in the subject, so please do that for the
> next version.
> 

Ok, I thought previous thread is just draft, so I renewed one. Losing some history,
sorry for that.

> Your changelog text is better than the first version, but can still be improved.
> I would at least mention that we want to remove all uses of 'timeval' from
> device drivers in order to better scan for y2038 problems in the drivers that
> still use them.
> 
> Also, you don't mention at all the discussion we had about real time vs.
> monotonic time for this driver. I think using monotonic time (ktime_get_ts64())
> would be more appropriate here, but whichever you choose, you should explain
> in the changelog why you think that one is better than the other.
> Most of the time, we end up changing from real time to monotonic time in the
> same patch when converting a driver to avoid 32-bit time_t, so even you don't
> change it, you should explain why not.

ktime_get_ts64() is another choice. As we discussed in previous thread, only using
jiffies can not keep the precise, and we should also handle the jiffies overflowing
case, so I kept the old implementation.

> 
>>  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;
>>  }
>>  
> 
> As in your other patch, I think the use of NSEC_PER_MSEC would make this
> slightly more understandable.
> 
> 	Arnd
> 

Re: [Y2038] [PATCH] dummy_hcd: replace timeval with timespec64

[Arnd Bergmann]

On Tuesday 15 September 2015 21:43:19 Pingbo Wen wrote:
> 
> On Tuesday, September 15, 2015 09:14 PM, Arnd Bergmann wrote:
> > On Tuesday 15 September 2015 20:56:15 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 still need to fix it here,
> >> to avoid similair problems.
> >>
> >> 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>
> > 
> > I just replied to the thread of the first mail, and it's good to see you had
> > the same thought about adding the usb folks to the Cc list.
> > 
> > When sending a new version of a patch you have already sent before, it's
> > common practice to use [PATCH v2] in the subject, so please do that for the
> > next version.
> > 
> 
> Ok, I thought previous thread is just draft, so I renewed one. Losing some history,
> sorry for that.

No, it's fine either way, I was just making sure you get the idea.

> 
> > Your changelog text is better than the first version, but can still be improved.
> > I would at least mention that we want to remove all uses of 'timeval' from
> > device drivers in order to better scan for y2038 problems in the drivers that
> > still use them.
> > 
> > Also, you don't mention at all the discussion we had about real time vs.
> > monotonic time for this driver. I think using monotonic time (ktime_get_ts64())
> > would be more appropriate here, but whichever you choose, you should explain
> > in the changelog why you think that one is better than the other.
> > Most of the time, we end up changing from real time to monotonic time in the
> > same patch when converting a driver to avoid 32-bit time_t, so even you don't
> > change it, you should explain why not.
> 
> ktime_get_ts64() is another choice. As we discussed in previous thread, only using
> jiffies can not keep the precise, and we should also handle the jiffies overflowing
> case, so I kept the old implementation.

Ok. As I said, it's most important to describe what the decision process was,
rather than the specific decision (real time has worked fine in the past,
so the change to monotonic is not critical here, just an additional improvement).

Let's wait for more comments to the question whether the resolution should be
changed for highspeed and superspeed mode, then you can post an updated version.

	Arnd