[PATCH] misc: ibmasm: Replace timeval with timespec64

[PATCH] misc: ibmasm: Replace timeval with timespec64

[Amitoj Kaur Chawla]

This patch replaces timeval with timespec64 as 32 bit 'struct timeval'
will not give current time beyond 2038.

This also changes the code to use ktime_get_real_ts64() which returns
a 'struct timespec64' instead of do_gettimeofday() which returns a
'struct timeval'

Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
---
 drivers/misc/ibmasm/ibmasm.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/ibmasm/ibmasm.h b/drivers/misc/ibmasm/ibmasm.h
index 9b08344..0c7bb6d 100644
--- a/drivers/misc/ibmasm/ibmasm.h
+++ b/drivers/misc/ibmasm/ibmasm.h
@@ -34,6 +34,7 @@
 #include <linux/kref.h>
 #include <linux/device.h>
 #include <linux/input.h>
+#include <linux/time64.h>
 
 /* Driver identification */
 #define DRIVER_NAME	"ibmasm"
@@ -53,9 +54,9 @@ extern int ibmasm_debug;
 
 static inline char *get_timestamp(char *buf)
 {
-	struct timeval now;
-	do_gettimeofday(&now);
-	sprintf(buf, "%lu.%lu", now.tv_sec, now.tv_usec);
+	struct timespec64 now;
+	ktime_get_real_ts64(&now);
+	sprintf(buf, "%lu.%lu", now.tv_sec, now.tv_nsec / NSEC_PER_USEC);
 	return buf;
 }
 
-- 
1.9.1

Re: [Y2038] [PATCH] misc: ibmasm: Replace timeval with timespec64

[Arnd Bergmann]

On Thursday 22 October 2015 23:09:20 Amitoj Kaur Chawla wrote:
> This patch replaces timeval with timespec64 as 32 bit 'struct timeval'
> will not give current time beyond 2038.
> 
> This also changes the code to use ktime_get_real_ts64() which returns
> a 'struct timespec64' instead of do_gettimeofday() which returns a
> 'struct timeval'
> 
> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>

Looks almost right, except for one bit:

> ---
>  drivers/misc/ibmasm/ibmasm.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/misc/ibmasm/ibmasm.h b/drivers/misc/ibmasm/ibmasm.h
> index 9b08344..0c7bb6d 100644
> --- a/drivers/misc/ibmasm/ibmasm.h
> +++ b/drivers/misc/ibmasm/ibmasm.h
> @@ -34,6 +34,7 @@
>  #include <linux/kref.h>
>  #include <linux/device.h>
>  #include <linux/input.h>
> +#include <linux/time64.h>
>  
>  /* Driver identification */
>  #define DRIVER_NAME    "ibmasm"
> @@ -53,9 +54,9 @@ extern int ibmasm_debug;
>  
>  static inline char *get_timestamp(char *buf)
>  {
> -       struct timeval now;
> -       do_gettimeofday(&now);
> -       sprintf(buf, "%lu.%lu", now.tv_sec, now.tv_usec);
> +       struct timespec64 now;
> +       ktime_get_real_ts64(&now);
> +       sprintf(buf, "%lu.%lu", now.tv_sec, now.tv_nsec / NSEC_PER_USEC);
>         return buf;
> 

now.tv_sec is using a 'time_t' a.k.a. 'long' on 64-bit architectures, but
a 'time64_t' a.k.a. 'long long' on 32-bit architectures. This is a bit
nonintuitive, but please change the format string to %llu for the first
argument and add a corresponding cast to 's64' or 'long long', so we
actually print the correct data and do not get a compiler warning.

The tv_nsec argument is always 'long', on both 32-bit and 64-bit
architectures, so there is no warning and we correctly print the
number. However, there is a preexisting bug in the format string
that you should fix while you are here: %lu drops leading zeroes,
which results in something like "12345678.1234" where we want to
see "1234567.001234". Please change the format string to add the
correct number of leading zeroes here.

As you seem to generally be doing well with the trivial patches, please
move on to the 'simple' category next. I should also add a few more
'medium' level tasks but need to pick them carefully first.

	Arnd

Re: [Y2038] [PATCH] misc: ibmasm: Replace timeval with timespec64

[Amitoj Kaur Chawla]

On Fri, Oct 23, 2015 at 12:45 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 22 October 2015 23:09:20 Amitoj Kaur Chawla wrote:
>> This patch replaces timeval with timespec64 as 32 bit 'struct timeval'
>> will not give current time beyond 2038.
>>
>> This also changes the code to use ktime_get_real_ts64() which returns
>> a 'struct timespec64' instead of do_gettimeofday() which returns a
>> 'struct timeval'
>>
>> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
>
> Looks almost right, except for one bit:
>
>> ---
>>  drivers/misc/ibmasm/ibmasm.h | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/misc/ibmasm/ibmasm.h b/drivers/misc/ibmasm/ibmasm.h
>> index 9b08344..0c7bb6d 100644
>> --- a/drivers/misc/ibmasm/ibmasm.h
>> +++ b/drivers/misc/ibmasm/ibmasm.h
>> @@ -34,6 +34,7 @@
>>  #include <linux/kref.h>
>>  #include <linux/device.h>
>>  #include <linux/input.h>
>> +#include <linux/time64.h>
>>
>>  /* Driver identification */
>>  #define DRIVER_NAME    "ibmasm"
>> @@ -53,9 +54,9 @@ extern int ibmasm_debug;
>>
>>  static inline char *get_timestamp(char *buf)
>>  {
>> -       struct timeval now;
>> -       do_gettimeofday(&now);
>> -       sprintf(buf, "%lu.%lu", now.tv_sec, now.tv_usec);
>> +       struct timespec64 now;
>> +       ktime_get_real_ts64(&now);
>> +       sprintf(buf, "%lu.%lu", now.tv_sec, now.tv_nsec / NSEC_PER_USEC);
>>         return buf;
>>
>
> now.tv_sec is using a 'time_t' a.k.a. 'long' on 64-bit architectures, but
> a 'time64_t' a.k.a. 'long long' on 32-bit architectures. This is a bit
> nonintuitive, but please change the format string to %llu for the first
> argument and add a corresponding cast to 's64' or 'long long', so we
> actually print the correct data and do not get a compiler warning.
>
> The tv_nsec argument is always 'long', on both 32-bit and 64-bit
> architectures, so there is no warning and we correctly print the
> number. However, there is a preexisting bug in the format string
> that you should fix while you are here: %lu drops leading zeroes,
> which results in something like "12345678.1234" where we want to
> see "1234567.001234". Please change the format string to add the
> correct number of leading zeroes here.
>

Just to clarify, I should change this to
   sprintf(buf, "%llu.%8.8lu", now.tv_sec, now.tv_nsec / NSEC_PER_USEC);

> As you seem to generally be doing well with the trivial patches, please
> move on to the 'simple' category next. I should also add a few more
> 'medium' level tasks but need to pick them carefully first.
>

Okay! Next patches will be the simple tasks. Also, I couldn't find the
tutorial you mentioned for compiling, can you help me out some there?

Thanks,
-- 
Amitoj

Re: [Y2038] [PATCH] misc: ibmasm: Replace timeval with timespec64

[Amitoj Kaur Chawla]

On Fri, Oct 23, 2015 at 7:18 AM, Amitoj Kaur Chawla
<amitoj1606@gmail.com> wrote:
> On Fri, Oct 23, 2015 at 12:45 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Thursday 22 October 2015 23:09:20 Amitoj Kaur Chawla wrote:
>>> This patch replaces timeval with timespec64 as 32 bit 'struct timeval'
>>> will not give current time beyond 2038.
>>>
>>> This also changes the code to use ktime_get_real_ts64() which returns
>>> a 'struct timespec64' instead of do_gettimeofday() which returns a
>>> 'struct timeval'
>>>
>>> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
>>
>> Looks almost right, except for one bit:
>>
>>> ---
>>>  drivers/misc/ibmasm/ibmasm.h | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/misc/ibmasm/ibmasm.h b/drivers/misc/ibmasm/ibmasm.h
>>> index 9b08344..0c7bb6d 100644
>>> --- a/drivers/misc/ibmasm/ibmasm.h
>>> +++ b/drivers/misc/ibmasm/ibmasm.h
>>> @@ -34,6 +34,7 @@
>>>  #include <linux/kref.h>
>>>  #include <linux/device.h>
>>>  #include <linux/input.h>
>>> +#include <linux/time64.h>
>>>
>>>  /* Driver identification */
>>>  #define DRIVER_NAME    "ibmasm"
>>> @@ -53,9 +54,9 @@ extern int ibmasm_debug;
>>>
>>>  static inline char *get_timestamp(char *buf)
>>>  {
>>> -       struct timeval now;
>>> -       do_gettimeofday(&now);
>>> -       sprintf(buf, "%lu.%lu", now.tv_sec, now.tv_usec);
>>> +       struct timespec64 now;
>>> +       ktime_get_real_ts64(&now);
>>> +       sprintf(buf, "%lu.%lu", now.tv_sec, now.tv_nsec / NSEC_PER_USEC);
>>>         return buf;
>>>
>>
>> now.tv_sec is using a 'time_t' a.k.a. 'long' on 64-bit architectures, but
>> a 'time64_t' a.k.a. 'long long' on 32-bit architectures. This is a bit
>> nonintuitive, but please change the format string to %llu for the first
>> argument and add a corresponding cast to 's64' or 'long long', so we
>> actually print the correct data and do not get a compiler warning.
>>
>> The tv_nsec argument is always 'long', on both 32-bit and 64-bit
>> architectures, so there is no warning and we correctly print the
>> number. However, there is a preexisting bug in the format string
>> that you should fix while you are here: %lu drops leading zeroes,
>> which results in something like "12345678.1234" where we want to
>> see "1234567.001234". Please change the format string to add the
>> correct number of leading zeroes here.
>>
>
> Just to clarify, I should change this to
>    sprintf(buf, "%llu.%8.8lu", now.tv_sec, now.tv_nsec / NSEC_PER_USEC);
>

Sorry, it should be changed to
    sprintf(buf, "%llu.%.08lu", (long long)now.tv_sec, now.tv_nsec /
NSEC_PER_USEC);

>> As you seem to generally be doing well with the trivial patches, please
>> move on to the 'simple' category next. I should also add a few more
>> 'medium' level tasks but need to pick them carefully first.
>>
>
> Okay! Next patches will be the simple tasks. Also, I couldn't find the
> tutorial you mentioned for compiling, can you help me out some there?
>
> Thanks,
> --
> Amitoj



-- 
Amitoj

Re: [Y2038] [PATCH] misc: ibmasm: Replace timeval with timespec64

[Arnd Bergmann]

On Friday 23 October 2015 08:55:22 Amitoj Kaur Chawla wrote:
> >
> > Just to clarify, I should change this to
> >    sprintf(buf, "%llu.%8.8lu", now.tv_sec, now.tv_nsec / NSEC_PER_USEC);
> >
> 
> Sorry, it should be changed to
>     sprintf(buf, "%llu.%.08lu", (long long)now.tv_sec, now.tv_nsec /
> NSEC_PER_USEC);
> 

Yes, the last version of that looks correct to me.

	Arnd

Re: [Outreachy kernel] Re: [Y2038] [PATCH] misc: ibmasm: Replace timeval with timespec64

[Ksenija Stanojević]

Hi Amitoj,

On Fri, Oct 23, 2015 at 3:48 AM, Amitoj Kaur Chawla
<amitoj1606@gmail.com> wrote:
> On Fri, Oct 23, 2015 at 12:45 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Thursday 22 October 2015 23:09:20 Amitoj Kaur Chawla wrote:
>>> This patch replaces timeval with timespec64 as 32 bit 'struct timeval'
>>> will not give current time beyond 2038.
>>>
>>> This also changes the code to use ktime_get_real_ts64() which returns
>>> a 'struct timespec64' instead of do_gettimeofday() which returns a
>>> 'struct timeval'
>>>
>>> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
>>
>> Looks almost right, except for one bit:
>>
>>> ---
>>>  drivers/misc/ibmasm/ibmasm.h | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/misc/ibmasm/ibmasm.h b/drivers/misc/ibmasm/ibmasm.h
>>> index 9b08344..0c7bb6d 100644
>>> --- a/drivers/misc/ibmasm/ibmasm.h
>>> +++ b/drivers/misc/ibmasm/ibmasm.h
>>> @@ -34,6 +34,7 @@
>>>  #include <linux/kref.h>
>>>  #include <linux/device.h>
>>>  #include <linux/input.h>
>>> +#include <linux/time64.h>
>>>
>>>  /* Driver identification */
>>>  #define DRIVER_NAME    "ibmasm"
>>> @@ -53,9 +54,9 @@ extern int ibmasm_debug;
>>>
>>>  static inline char *get_timestamp(char *buf)
>>>  {
>>> -       struct timeval now;
>>> -       do_gettimeofday(&now);
>>> -       sprintf(buf, "%lu.%lu", now.tv_sec, now.tv_usec);
>>> +       struct timespec64 now;
>>> +       ktime_get_real_ts64(&now);
>>> +       sprintf(buf, "%lu.%lu", now.tv_sec, now.tv_nsec / NSEC_PER_USEC);
>>>         return buf;
>>>
>>
>> now.tv_sec is using a 'time_t' a.k.a. 'long' on 64-bit architectures, but
>> a 'time64_t' a.k.a. 'long long' on 32-bit architectures. This is a bit
>> nonintuitive, but please change the format string to %llu for the first
>> argument and add a corresponding cast to 's64' or 'long long', so we
>> actually print the correct data and do not get a compiler warning.
>>
>> The tv_nsec argument is always 'long', on both 32-bit and 64-bit
>> architectures, so there is no warning and we correctly print the
>> number. However, there is a preexisting bug in the format string
>> that you should fix while you are here: %lu drops leading zeroes,
>> which results in something like "12345678.1234" where we want to
>> see "1234567.001234". Please change the format string to add the
>> correct number of leading zeroes here.
>>
>
> Just to clarify, I should change this to
>    sprintf(buf, "%llu.%8.8lu", now.tv_sec, now.tv_nsec / NSEC_PER_USEC);
>
>> As you seem to generally be doing well with the trivial patches, please
>> move on to the 'simple' category next. I should also add a few more
>> 'medium' level tasks but need to pick them carefully first.
>>
>
> Okay! Next patches will be the simple tasks. Also, I couldn't find the
> tutorial you mentioned for compiling, can you help me out some there?

I think the tutorial mentioned is kernel newbies First patch tutorial and under
section Make a driver change:
http://kernelnewbies.org/Outreachyfirstpatch#head-816e6eef0008340eab7d542c3f6923a1f896478d
there is instruction how to compile your changes:

Compile your changes:

Recompile your kernel, by running make (with an optional -jN flag):
make -j2

Re: [Outreachy kernel] Re: [Y2038] [PATCH] misc: ibmasm: Replace timeval with timespec64

[Amitoj Kaur Chawla]

>> Okay! Next patches will be the simple tasks. Also, I couldn't find the
>> tutorial you mentioned for compiling, can you help me out some there?
>
> I think the tutorial mentioned is kernel newbies First patch tutorial and under
> section Make a driver change:
> http://kernelnewbies.org/Outreachyfirstpatch#head-816e6eef0008340eab7d542c3f6923a1f896478d
> there is instruction how to compile your changes:
>
> Compile your changes:
>
> Recompile your kernel, by running make (with an optional -jN flag):
> make -j2

Hi Ksenija,

Thanks for the help. I'm already doing this, I was under the
impression that Arnd was mentioning something other than this. I could
be wrong.

-- 
Amitoj

Re: [Outreachy kernel] Re: [Y2038] [PATCH] misc: ibmasm: Replace timeval with timespec64

[Julia Lawall]

On Fri, 23 Oct 2015, Amitoj Kaur Chawla wrote:

> >> Okay! Next patches will be the simple tasks. Also, I couldn't find the
> >> tutorial you mentioned for compiling, can you help me out some there?
> >
> > I think the tutorial mentioned is kernel newbies First patch tutorial and under
> > section Make a driver change:
> > http://kernelnewbies.org/Outreachyfirstpatch#head-816e6eef0008340eab7d542c3f6923a1f896478d
> > there is instruction how to compile your changes:
> >
> > Compile your changes:
> >
> > Recompile your kernel, by running make (with an optional -jN flag):
> > make -j2
>
> Hi Ksenija,
>
> Thanks for the help. I'm already doing this, I was under the
> impression that Arnd was mentioning something other than this. I could
> be wrong.

Are you sure that you have a .o file for the code you are working on?  And
if you do, are you compiling the whole kernel?  Maybe the problem is
related to linking?

julia

Re: [Outreachy kernel] Re: [Y2038] [PATCH] misc: ibmasm: Replace timeval with timespec64

[Amitoj Kaur Chawla]

[-- Attachment #1: Type: text/plain, Size: 1583 bytes --]

On Fri, Oct 23, 2015 at 5:55 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Fri, 23 Oct 2015, Amitoj Kaur Chawla wrote:
>
>> >> Okay! Next patches will be the simple tasks. Also, I couldn't find the
>> >> tutorial you mentioned for compiling, can you help me out some there?
>> >
>> > I think the tutorial mentioned is kernel newbies First patch tutorial
and under
>> > section Make a driver change:
>> >
http://kernelnewbies.org/Outreachyfirstpatch#head-816e6eef0008340eab7d542c3f6923a1f896478d
>> > there is instruction how to compile your changes:
>> >
>> > Compile your changes:
>> >
>> > Recompile your kernel, by running make (with an optional -jN flag):
>> > make -j2
>>
>> Hi Ksenija,
>>
>> Thanks for the help. I'm already doing this, I was under the
>> impression that Arnd was mentioning something other than this. I could
>> be wrong.
>
> Are you sure that you have a .o file for the code you are working on?  And
> if you do, are you compiling the whole kernel?  Maybe the problem is
> related to linking?
>
> julia

Yep, found the issue.

For reference of other applicants in case they get stuck in a similar
issue, I'll clarify what happened.

I had initially modified only staging drivers to be compilable as a module.
But here I was working on other drivers. Hence the issue.

The Outreachyfirstpatch <http://kernelnewbies.org/Outreachyfirstpatch> page
specifies the process for staging drivers. For other drivers, the Kconfig
file can be referred to for finding the driver to make it compilable as a
module.


Thank you Julia for helping out!


-- 
Amitoj

[-- Attachment #2: Type: text/html, Size: 2120 bytes --]