> 在 2015年9月14日，16:37，Arnd Bergmann <[email protected]> 写道：
>
> On Sunday 13 September 2015 19:00:11 WEN Pingbo wrote:
>> proc_show only output string, so we can keep userspace untouched
>> while replacing timeval with timespec64 in kernel.
>>
>> Not all timer in i8042 have y2038 risk(handshake, match timer, etc),
>> Replacements in those timer are just for consistency.
>
> The patch looks mostly ok, a few details here:
>
> - Please start the changelog with an explanation of what the driver does
> and why that is a problem, e.g.
>
> "The SDC real time clock driver internally uses an 'struct timeval' to
> represent times, which will overflow on 32-bit machines in 2038.”
>
Ok, I will rearrange it later. Thanks.
>> @@ -209,15 +209,15 @@ static inline int hp_sdc_rtc_read_rt(struct timeval
>> *res) {
>> tenms = (uint32_t)raw & 0xffffff;
>> days = (unsigned int)(raw >> 24) & 0xffff;
>>
>> - res->tv_usec = (suseconds_t)(tenms % 100) * 10000;
>> - res->tv_sec = (time_t)(tenms / 100) + days * 86400;
>> + res->tv_nsec = (long)(tenms % 100) * 10000 * 1000;
>
> For clarity, I would suggest changing the code to use the USEC_PER_SEC
> macro. We don't have a global SECONDS_PER_DAY macro unfortunately. If
> you are really keen, you could introduce one in include/linux/time64.h,
> otherwise just leave it as it is.
That make sense.
>
>> + res->tv_sec = (time64_t)(tenms / 100) + days * 86400;
>
> If I read this right, you still have a bug here: "days" is an "unsigned int"
> variable, and multiplying that with 86400 will overflow in 2106. We
> don't care that much about this overflow yet, but there is no reason
> to not get it right here if it's cheap.
>
> If you move the type cast, it will be fine.
Yes, we need one more type cast here.
>
>> @@ -452,36 +452,36 @@ static int hp_sdc_rtc_proc_show(struct seq_file *m,
>> void *v)
>> if (hp_sdc_rtc_read_rt(&tv)) {
>> seq_puts(m, "i8042 rtc\t: READ FAILED!\n");
>> } else {
>> - seq_printf(m, "i8042 rtc\t: %ld.%02d seconds\n",
>> - tv.tv_sec, (int)tv.tv_usec/1000);
>> + seq_printf(m, "i8042 rtc\t: %lld.%02ld seconds\n",
>> + tv.tv_sec, (long)tv.tv_nsec/1000000L);
>> }
>>
>
> The change from (int) to (long) is not necessary here, because the nanoseconds
> always fit in a 32bit int type.
>
> However, the tv_sec variable is currently defined in a slightly awkward way,
> and is 'long' on 64-bit architectures but 'long long' on 32-bit architectures.
>
> This means you get a compile warning for a type mismatch on 64-bit compilers
> that print a 'long' argument with %lld. The best workaround I've come up
> with is to add a cast to 'long long' or 's64', which is defined as 'long long'
> on all architectures.
>
> To avoid problems like this, it's useful to build test with both 32-bit and
> 64-bit compilers, and to list in the changelog text what kind of testing
> you have done to verify the driver.
I didn’t notice that. It seems hp_sdc_rtc only available in m68k & hppa arch. I
am still on the way to compile it correctly.
>
> Arnd
_______________________________________________
Y2038 mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/y2038
