在 2015年9月14日，16:37，Arnd Bergmann arnd@arndb.de 写道：
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