diff mbox

[RFC,v2,03/11] time: Add rtc_tm_to_time64() safe version(using time64_t)

Message ID 1414667745-7703-4-git-send-email-pang.xunlei@linaro.org
State New
Headers show

Commit Message

pang.xunlei Oct. 30, 2014, 11:15 a.m. UTC
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(-)

Comments

Thomas Gleixner Oct. 30, 2014, 1:55 p.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
pang.xunlei Oct. 30, 2014, 4:30 p.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Thomas Gleixner Oct. 30, 2014, 8:49 p.m. UTC | #3
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






--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
diff mbox

Patch

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