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

[Pingbo Wen <pingbo.wen@linaro.org>]

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
>