| Message ID | 1414667745-7703-4-git-send-email-pang.xunlei@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 30 Oct 2014, pang.xunlei wrote: Same $subject issue. > As part of addressing 2038 saftey for in-kernel uses, this patch > adds safe rtc_tm_to_time64() using time64_t. After this patch, > rtc_tm_to_time() should be replaced by rtc_tm_to_time64() one by > one. Eventually, rtc_tm_to_time() will be removed from the kernel > when it has no users. > > Signed-off-by: pang.xunlei <pang.xunlei@linaro.org> > --- > drivers/rtc/rtc-lib.c | 15 ++++++++++++++- > include/linux/rtc.h | 1 + > 2 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/rtc/rtc-lib.c b/drivers/rtc/rtc-lib.c > index c4cf057..6948cbd 100644 > --- a/drivers/rtc/rtc-lib.c > +++ b/drivers/rtc/rtc-lib.c > @@ -110,10 +110,23 @@ EXPORT_SYMBOL(rtc_valid_tm); > > /* > * Convert Gregorian date to seconds since 01-01-1970 00:00:00. > + * Safe version for 2038 safety. Docbook comment missing. See the previous replies. > + */ > +int rtc_tm_to_time64(struct rtc_time *tm, time64_t *time) What's the point of the return value? Just because rtc_tm_to_time() has one? Yes it does, but that does not mean that we blindly copy the existing nonsense when we implement a replacement interface. What's wrong with changing it to: time64_t rtc_tm_to_time64(struct rtc_time *tm) { return mktime64(......); } > +/* > + * Convert Gregorian date to seconds since 01-01-1970 00:00:00. > + * TODO: [2038 safety] should be replaced by rtc_tm_to_time64. Groan. > */ > int rtc_tm_to_time(struct rtc_time *tm, unsigned long *time) > { > - *time = mktime(tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday, > + *time = (unsigned long) mktime64(tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday, > tm->tm_hour, tm->tm_min, tm->tm_sec); And that would become *time = rtc_time_to_time64(tm); > return 0; > } Please stop doing purely mechanical changes. Just blindly copying stuff and make it take time64_t instead of unsigned long can be done with a script as well. Thanks, tglx
On 30 October 2014 21:55, Thomas Gleixner <tglx@linutronix.de> wrote: > On Thu, 30 Oct 2014, pang.xunlei wrote: > > Same $subject issue. > >> As part of addressing 2038 saftey for in-kernel uses, this patch >> adds safe rtc_tm_to_time64() using time64_t. After this patch, >> rtc_tm_to_time() should be replaced by rtc_tm_to_time64() one by >> one. Eventually, rtc_tm_to_time() will be removed from the kernel >> when it has no users. >> >> Signed-off-by: pang.xunlei <pang.xunlei@linaro.org> >> --- >> drivers/rtc/rtc-lib.c | 15 ++++++++++++++- >> include/linux/rtc.h | 1 + >> 2 files changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/rtc/rtc-lib.c b/drivers/rtc/rtc-lib.c >> index c4cf057..6948cbd 100644 >> --- a/drivers/rtc/rtc-lib.c >> +++ b/drivers/rtc/rtc-lib.c >> @@ -110,10 +110,23 @@ EXPORT_SYMBOL(rtc_valid_tm); >> >> /* >> * Convert Gregorian date to seconds since 01-01-1970 00:00:00. >> + * Safe version for 2038 safety. > > Docbook comment missing. See the previous replies. > >> + */ >> +int rtc_tm_to_time64(struct rtc_time *tm, time64_t *time) > > What's the point of the return value? Just because rtc_tm_to_time() > has one? > > Yes it does, but that does not mean that we blindly copy the existing > nonsense when we implement a replacement interface. > > What's wrong with changing it to: > > time64_t rtc_tm_to_time64(struct rtc_time *tm) > { > return mktime64(......); > } Thank you for reminding me of this, and also a valuable principle when doing things. I wanna change it to: void rtc_tm_to_time64(struct rtc_time *tm, time64_t *time) { time = mktime64(......); } Just keep it the similar format as rtc_time64_to_tm(), is this acceptable? > >> +/* >> + * Convert Gregorian date to seconds since 01-01-1970 00:00:00. >> + * TODO: [2038 safety] should be replaced by rtc_tm_to_time64. > > Groan. > >> */ >> int rtc_tm_to_time(struct rtc_time *tm, unsigned long *time) >> { >> - *time = mktime(tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday, >> + *time = (unsigned long) mktime64(tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday, >> tm->tm_hour, tm->tm_min, tm->tm_sec); > > And that would become > > *time = rtc_time_to_time64(tm); > >> return 0; >> } > > Please stop doing purely mechanical changes. Just blindly copying > stuff and make it take time64_t instead of unsigned long can be done > with a script as well. > > Thanks, > > tglx
On Fri, 31 Oct 2014, pang.xunlei wrote: > On 30 October 2014 21:55, Thomas Gleixner <tglx@linutronix.de> wrote: > > What's wrong with changing it to: > > > > time64_t rtc_tm_to_time64(struct rtc_time *tm) > > { > > return mktime64(......); > > } > > Thank you for reminding me of this, and also a valuable principle when > doing things. > > I wanna change it to: > void rtc_tm_to_time64(struct rtc_time *tm, time64_t *time) > { > time = mktime64(......); > } > > Just keep it the similar format as rtc_time64_to_tm(), is this acceptable? Why on earth want you do that? Just because it looks the same or what? Come on, it's not that hard and this is not the CS 'Java for dummies' course where you get an A for implementing 'looks the same' nonsense. For rtc_time64_to_tm() you really want to hand in the pointer for the result because returning the full structure is overkill as it involves heavy stack operations. But for rtc_tm_to_time64() this is a different story. All relevant 32bit architectures can deal with a u64 return value very well and in most cases it creates better code. Just do the simple experiment of implementing both, converting a dozen drivers and comparing the resulting assembler code on the call sites and the function itself. That should give you an objective decision criteria instead of "wanna change just because I feel it looks better". Another valuable principle of doing things to remember, right? Thanks, tglx
diff --git a/drivers/rtc/rtc-lib.c b/drivers/rtc/rtc-lib.c index c4cf057..6948cbd 100644 --- a/drivers/rtc/rtc-lib.c +++ b/drivers/rtc/rtc-lib.c @@ -110,10 +110,23 @@ EXPORT_SYMBOL(rtc_valid_tm); /* * Convert Gregorian date to seconds since 01-01-1970 00:00:00. + * Safe version for 2038 safety. + */ +int rtc_tm_to_time64(struct rtc_time *tm, time64_t *time) +{ + *time = mktime64(tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday, + tm->tm_hour, tm->tm_min, tm->tm_sec); + return 0; +} +EXPORT_SYMBOL(rtc_tm_to_time64); + +/* + * Convert Gregorian date to seconds since 01-01-1970 00:00:00. + * TODO: [2038 safety] should be replaced by rtc_tm_to_time64. */ int rtc_tm_to_time(struct rtc_time *tm, unsigned long *time) { - *time = mktime(tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday, + *time = (unsigned long) mktime64(tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday, tm->tm_hour, tm->tm_min, tm->tm_sec); return 0; } diff --git a/include/linux/rtc.h b/include/linux/rtc.h index c2c2897..2a6a9ce 100644 --- a/include/linux/rtc.h +++ b/include/linux/rtc.h @@ -19,6 +19,7 @@ extern int rtc_month_days(unsigned int month, unsigned int year); extern int rtc_year_days(unsigned int day, unsigned int month, unsigned int year); extern int rtc_valid_tm(struct rtc_time *tm); +extern int rtc_tm_to_time64(struct rtc_time *tm, time64_t *time); extern int rtc_tm_to_time(struct rtc_time *tm, unsigned long *time); extern void rtc_time_to_tm(unsigned long time, struct rtc_time *tm); ktime_t rtc_tm_to_ktime(struct rtc_time tm);
As part of addressing 2038 saftey for in-kernel uses, this patch adds safe rtc_tm_to_time64() using time64_t. After this patch, rtc_tm_to_time() should be replaced by rtc_tm_to_time64() one by one. Eventually, rtc_tm_to_time() will be removed from the kernel when it has no users. Signed-off-by: pang.xunlei <pang.xunlei@linaro.org> --- drivers/rtc/rtc-lib.c | 15 ++++++++++++++- include/linux/rtc.h | 1 + 2 files changed, 15 insertions(+), 1 deletion(-)