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

2015-10-23 Thread Amitoj Kaur Chawla 
This patch replaces timeval with timespec64 as 32 bit 'struct timeval'
will not give current time beyond 2038.

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

This patch also alters the format strings in sprintf() for now.tv_sec
and now.tv_nsec to incorporate 'long long' on 32 bit architectures and
leading zeroes respectively.

Signed-off-by: Amitoj Kaur Chawla 
Reviewed-by: Arnd Bergmann 
---
Changes in v2:
-change format string of now.tv_sec to '%llu'
-change format string of now.tv_nsec to '%.08lu'
Changes in v3:
-Replace tv_usec with tv_nsec, error made in v2
-Build tested

 drivers/misc/ibmasm/ibmasm.h | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/ibmasm/ibmasm.h b/drivers/misc/ibmasm/ibmasm.h
index 9b08344..82380ae 100644
--- a/drivers/misc/ibmasm/ibmasm.h
+++ b/drivers/misc/ibmasm/ibmasm.h
@@ -34,6 +34,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* Driver identification */
 #define DRIVER_NAME"ibmasm"
@@ -53,9 +54,10 @@ extern int ibmasm_debug;
 
 static inline char *get_timestamp(char *buf)
 {
-   struct timeval now;
-   do_gettimeofday();
-   sprintf(buf, "%lu.%lu", now.tv_sec, now.tv_usec);
+   struct timespec64 now;
+   ktime_get_real_ts64();
+   sprintf(buf, "%llu.%.08lu", (long long)now.tv_sec, 
+   now.tv_nsec / NSEC_PER_USEC);
return buf;
 }
 
-- 
1.9.1

___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH V2] hp_sdc: fixed y2038 problem

2015-10-23 Thread Pingbo Wen 


On Friday, October 23, 2015 05:25 PM, Arnd Bergmann wrote:
> On Friday 23 October 2015 16:53:26 WEN Pingbo wrote:
>> 1. Converting timeval to ktime_t, and there is no need to handle sec and
>> usec separately
>>
>> 2. hp_sdc.rtv is only used for time diff, monotonic time is better here
>>
>> Signed-off-by: WEN Pingbo 
>> ---
> 
> This version is still correct and looks better than the first version, but
> I'd still like you to improve some details:
> 
> - read some other changelogs and follow the common style. In particular,
>   in the subject line say /what/ you do ("e.g. use ktime_get instead of
>   do_gettimeofday",  or "avoid using struct timespec") instead of /why/,
>   but then explain in the changelog text what is wrong with the current
>   version and why it gets changed like this.
> 
> - Below the '---', add a short list of things you have changed since
>   the previous versions. This part will not get copied into the git
>   history.
> 

Ok, I will fix this in the next version.

>> -do_gettimeofday();
>> -if (tv.tv_sec > hp_sdc.rtv.tv_sec)
>> -tv.tv_usec += USEC_PER_SEC;
>> -
>> -if (tv.tv_usec - hp_sdc.rtv.tv_usec > HP_SDC_MAX_REG_DELAY) {
>> +if (time_diff.tv64 > HP_SDC_MAX_REG_DELAY) {
>>  hp_sdc_transaction *curr;
>>  uint8_t tmp;
>>  
>> -printk(KERN_WARNING PREFIX "read timeout (%ius)!\n",
>> -   (int)(tv.tv_usec - hp_sdc.rtv.tv_usec));
>> +printk(KERN_WARNING PREFIX "read timeout (%llins)!\n",
>> +   time_diff.tv64);
>>  curr->idx += hp_sdc.rqty;
>>  hp_sdc.rqty = 0;
>>  tmp = curr->seq[curr->actidx];
> 
> A tiny style comment here: please use ktime_to_ns() to extract the
> nanoseconds out of the ktime_t type rather than accessing the tv64
> member directly.

Same here.

Thank you
Pingbo

> 
>   Arnd
> 
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

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

2015-10-23 Thread Arnd Bergmann 
On Friday 23 October 2015 17:52:06 Amitoj Kaur Chawla wrote:
> This patch replaces timeval with timespec64 as 32 bit 'struct timeval'
> will not give current time beyond 2038.
> 
> The patch changes the code to use ktime_get_real_ts64() which returns
> a 'struct timespec64' instead of do_gettimeofday() which returns a
> 'struct timeval'
> 
> This patch also alters the format strings in sprintf() for now.tv_sec and
> now.tv_nsec to incorporate 'long long' on 32 bit architectures and
> leading zeroes respectively.
> 
> Signed-off-by: Amitoj Kaur Chawla 

Reviewed-by: Arnd Bergmann 
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

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

2015-10-23 Thread Amitoj Kaur Chawla 
On Fri, Oct 23, 2015 at 7:17 PM, Arnd Bergmann  wrote:
> On Friday 23 October 2015 15:39:49 Arnd Bergmann wrote:
>> On Friday 23 October 2015 17:52:06 Amitoj Kaur Chawla wrote:
>> > This patch replaces timeval with timespec64 as 32 bit 'struct timeval'
>> > will not give current time beyond 2038.
>> >
>> > The patch changes the code to use ktime_get_real_ts64() which returns
>> > a 'struct timespec64' instead of do_gettimeofday() which returns a
>> > 'struct timeval'
>> >
>> > This patch also alters the format strings in sprintf() for now.tv_sec and
>> > now.tv_nsec to incorporate 'long long' on 32 bit architectures and
>> > leading zeroes respectively.
>> >
>> > Signed-off-by: Amitoj Kaur Chawla 
>>
>> Reviewed-by: Arnd Bergmann 
>
> I was going to suggest to send this to the maintainers now, but after
> checking the MAINTAINERS file myself, I see that this would be Greg and me ;-)
>
> Greg, can you pick this up into the char-misc tree?
>
> Arnd

Hi Arnd,

I made a slight error while sending v2 which I caught after figuring
out of my issue of compiling the module.

I'm sending a v3 after build testing it.

Should I cc you and Greg separately or let it be?

-- 
Amitoj
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

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

2015-10-23 Thread Amitoj Kaur Chawla 
On Fri, Oct 23, 2015 at 5:55 PM, Julia Lawall  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  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
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH V2] hil_mlc: convert timeval to ktime_t

2015-10-23 Thread Arnd Bergmann 
On Friday 23 October 2015 20:32:50 Pingbo Wen wrote:
> >> -do_gettimeofday();
> >> -tv.tv_usec += USEC_PER_SEC * (tv.tv_sec - 
> >> mlc->instart.tv_sec);
> >> -tv.tv_usec -= mlc->instart.tv_usec;
> >> -if (tv.tv_usec >= mlc->intimeout) goto sched;
> >> -tv.tv_usec = (mlc->intimeout - tv.tv_usec) * HZ / 
> >> USEC_PER_SEC;
> >> -if (!tv.tv_usec) goto sched;
> >> -mod_timer(_mlcs_kicker, jiffies + tv.tv_usec);
> >> +if (tmp.tv64 >= (mlc->intimeout * NSEC_PER_USEC))
> >> +goto sched;
> >> +tmp.tv64 = mlc->intimeout * NSEC_PER_USEC - tmp.tv64;
> >> +if (tmp.tv64 < NSEC_PER_USEC)
> >> +goto sched;
> >> +mod_timer(_mlcs_kicker,
> >> +jiffies + nsecs_to_jiffies(tmp.tv64));
> >>  break;
> >>  sched:
> >>  tasklet_schedule(_mlcs_tasklet);
> > 
> > If I read this right, the code is executed one for each input event such
> > as a keypress or mouse movement. In the latter case, doing 
> > nsecs_to_jiffies()
> > here is actually a bit expensive, and I stil think it can be avoided
> > by just using jiffies.
> > 
> > For the (tmp.tv64 < NSEC_PER_USEC) part, did you just do that because
> > I said this, or did you actually prove that it is required? I'm still
> > confused about what the driver is trying to achieve here.
> 
> More explanation here:)
> the judgement here is to prevent mod_timer with zero delta. I can not
> make sure whether the module have nanosecond precise, so just keep same.

Ok, I guess I was misreading the original code. What it actually does
is to check the remaining time in jiffies, not in microseconds, so the
algorithm is:

if (already expired)
schedule tasklet
else {
convert to jiffies
if (expired just now)
schedule tasklet
else
schedule tasklet from timer
}

So the entire code is meant to guarantee that the tasklet is getting
scheduled, and the first two cases are just an optimization to avoid
going through the timer. I checked the code for mod_timer to verify
that mod_timer with an argument in the past will just cause the
handler to be called at the next tick, so we don't really need the
middle case, and the logic becomes really simple if you use jiffies
instead of ktime_t.

Arnd
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH] hil_mlc: convert timeval to timespec64

2015-10-23 Thread Arnd Bergmann 
On Friday 23 October 2015 17:12:38 Pingbo Wen wrote:
> On Monday, October 19, 2015 04:58 PM, Arnd Bergmann wrote:
> >> -do_gettimeofday();
> >> -tv.tv_usec += USEC_PER_SEC * (tv.tv_sec - 
> >> mlc->instart.tv_sec);
> >> -tv.tv_usec -= mlc->instart.tv_usec;
> >> -if (tv.tv_usec >= mlc->intimeout) goto sched;
> >> -tv.tv_usec = (mlc->intimeout - tv.tv_usec) * HZ / 
> >> USEC_PER_SEC;
> >> -if (!tv.tv_usec) goto sched;
> >> -mod_timer(_mlcs_kicker, jiffies + tv.tv_usec);
> >> +ktime_get_ts64();
> >> +ts64.tv_nsec += NSEC_PER_SEC *
> >> +(ts64.tv_sec - mlc->instart.tv_sec);
> >> +ts64.tv_nsec -= mlc->instart.tv_nsec;
> > 
> > tv_nsec will overflow here for any timeout over 4.3 seconds, where it
> > used to overflow after 4294 seconds. This is almost certainly a bug.
> > 
> > You could work around that by using ktime_get_ns() to get a nanosecond
> > value right away, but a 64-bit number is more expensive to convert to
> > jiffies.
> 
> You are right, I didn't notice that tv_nsec is a 32bit variable. Maybe
> we should use ktime_t here, so that handling sec and nsec separately is
> needless.
> 
> Using jiffies here will need to take more codes to handle jiffies overflow
> carefully. I think coverting 64bit number to jiffies is the price we must 
> take, if we use 64bit version here.

Handling the jiffies overflow is trivially done through the time_before()
and time_after() helpers, like


start = jiffies;
...
now = jiffies;
timeout = start + HZ * timeout_usec / USEC_PER_SEC;
if (time_after(now, start + timeout_jiffies)
timeout();
else
mod_timer(timer, start + timeout_jiffies);

The time_after function works because unsigned overflow is well-defined
in C (unlike signed overflow).

As a side-note, please take the time to delete any lines from the original
mail that you are not referencing when you reply.

Arnd
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

[Y2038] [PATCH V2] hil_mlc: convert timeval to ktime_t

2015-10-23 Thread WEN Pingbo 
Using struct timeval will cause time overflow in 2038, replacing it with
ktime_t. And we don't need to handle sec and nsec separately.

Since mlc->lcv_t is only interested in seconds, directly using
time64_t here.

And monotonic time is better here, since the original driver don't care
the wall time.

In addition, the original driver try to covert usec to jiffies manually in
hilse_donode(). This is not a universal and safe way, using
nsecs_to_jiffies() to fix that.

Signed-off-by: WEN Pingbo 
---
 drivers/input/serio/hil_mlc.c| 28 +---
 drivers/input/serio/hp_sdc_mlc.c |  8 
 include/linux/hil_mlc.h  |  4 ++--
 3 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/input/serio/hil_mlc.c b/drivers/input/serio/hil_mlc.c
index 65605e4..2746509 100644
--- a/drivers/input/serio/hil_mlc.c
+++ b/drivers/input/serio/hil_mlc.c
@@ -274,14 +274,12 @@ static int hilse_match(hil_mlc *mlc, int unused)
 /* An LCV used to prevent runaway loops, forces 5 second sleep when reset. */
 static int hilse_init_lcv(hil_mlc *mlc, int unused)
 {
-   struct timeval tv;
+   time64_t now = ktime_get_seconds();
 
-   do_gettimeofday();
-
-   if (mlc->lcv && (tv.tv_sec - mlc->lcv_tv.tv_sec) < 5)
+   if (mlc->lcv && (now - mlc->lcv_t) < 5)
return -1;
 
-   mlc->lcv_tv = tv;
+   mlc->lcv_t = now;
mlc->lcv = 0;
 
return 0;
@@ -605,7 +603,7 @@ static inline void hilse_setup_input(hil_mlc *mlc, const 
struct hilse_node *node
}
mlc->istarted = 1;
mlc->intimeout = node->arg;
-   do_gettimeofday(&(mlc->instart));
+   mlc->instart = ktime_get();
mlc->icount = 15;
memset(mlc->ipacket, 0, 16 * sizeof(hil_packet));
BUG_ON(down_trylock(>isem));
@@ -710,7 +708,7 @@ static int hilse_donode(hil_mlc *mlc)
break;
}
mlc->ostarted = 0;
-   do_gettimeofday(&(mlc->instart));
+   mlc->instart = ktime_get();
write_unlock_irqrestore(>lock, flags);
nextidx = HILSEN_NEXT;
break;
@@ -731,18 +729,18 @@ static int hilse_donode(hil_mlc *mlc)
 #endif
 
while (nextidx & HILSEN_SCHED) {
-   struct timeval tv;
+   ktime_t tmp = ktime_sub(ktime_get(), mlc->instart);
 
if (!sched_long)
goto sched;
 
-   do_gettimeofday();
-   tv.tv_usec += USEC_PER_SEC * (tv.tv_sec - mlc->instart.tv_sec);
-   tv.tv_usec -= mlc->instart.tv_usec;
-   if (tv.tv_usec >= mlc->intimeout) goto sched;
-   tv.tv_usec = (mlc->intimeout - tv.tv_usec) * HZ / USEC_PER_SEC;
-   if (!tv.tv_usec) goto sched;
-   mod_timer(_mlcs_kicker, jiffies + tv.tv_usec);
+   if (tmp.tv64 >= (mlc->intimeout * NSEC_PER_USEC))
+   goto sched;
+   tmp.tv64 = mlc->intimeout * NSEC_PER_USEC - tmp.tv64;
+   if (tmp.tv64 < NSEC_PER_USEC)
+   goto sched;
+   mod_timer(_mlcs_kicker,
+   jiffies + nsecs_to_jiffies(tmp.tv64));
break;
sched:
tasklet_schedule(_mlcs_tasklet);
diff --git a/drivers/input/serio/hp_sdc_mlc.c b/drivers/input/serio/hp_sdc_mlc.c
index d50f067..0a27b89 100644
--- a/drivers/input/serio/hp_sdc_mlc.c
+++ b/drivers/input/serio/hp_sdc_mlc.c
@@ -149,7 +149,8 @@ static int hp_sdc_mlc_in(hil_mlc *mlc, suseconds_t timeout)
 
/* Try to down the semaphore */
if (down_trylock(>isem)) {
-   struct timeval tv;
+   ktime_t tmp = ktime_sub(ktime_get(), mlc->instart);
+
if (priv->emtestmode) {
mlc->ipacket[0] =
HIL_ERR_INT | (mlc->opacket &
@@ -160,9 +161,8 @@ static int hp_sdc_mlc_in(hil_mlc *mlc, suseconds_t timeout)
/* printk(KERN_DEBUG PREFIX ">[%x]\n", 
mlc->ipacket[0]); */
goto wasup;
}
-   do_gettimeofday();
-   tv.tv_usec += USEC_PER_SEC * (tv.tv_sec - mlc->instart.tv_sec);
-   if (tv.tv_usec - mlc->instart.tv_usec > mlc->intimeout) {
+
+   if (tmp.tv64 > mlc->intimeout * NSEC_PER_USEC) {
/*  printk("!%i %i",
tv.tv_usec - mlc->instart.tv_usec,
mlc->intimeout);
diff --git a/include/linux/hil_mlc.h b/include/linux/hil_mlc.h
index 394a840..ec6 100644
--- a/include/linux/hil_mlc.h
+++ b/include/linux/hil_mlc.h
@@ -144,12 +144,12 @@ struct hil_mlc {
hil_packet  ipacket[16];
hil_packet  imatch;
int icount;
-   struct timeval  instart;
+   ktime_t instart;
suseconds_t

Re: [Y2038] [PATCH V3] hp_sdc: convert struct timeval to ktime_t

2015-10-23 Thread Arnd Bergmann 
On Friday 23 October 2015 20:19:46 Pingbo Wen wrote:
> 
> > Also, we don't normally have enumerated lists in a changelog, just use
> > normal text. The best changelogs typically have three paragraphs:
> > 
> > The first paragraph describes what the driver currently does. For really
> > obvious cases, this can be combined with the second paragraph.
> > 
> > The second paragraph explains why that is bad. This is where you can
> > mention the monotonic time vs real time issue and say whether we
> > just want the timeval removed to fix the kernel in general or whether
> > this particular driver is broken.
> > 
> > The third paragraph explains what the patch does to resolve the issue
> > described in the second one. This is also where you can list other
> > approaches that would have solved the problem, and why you picked on
> > over the others.
> 
> Do we really need this in ChangeLog? Commit msg already states this. I think
> the purpose of ChangeLog is let people know the main difference of two
> version patch at a glance, and the ‘what’ and ‘why’ should be placed in
> commit msg.
> 

I was using the terms changelog and commit message interchangeably, sorry
for being unclear. I meant the part above the --- line. The revision
history you have below the --- line looks good here.

Arnd
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH V2] hil_mlc: convert timeval to ktime_t

2015-10-23 Thread Pingbo Wen 

> 在 2015年10月23日，17:55，Arnd Bergmann  写道：
> 
> On Friday 23 October 2015 17:24:59 WEN Pingbo wrote:
>> Using struct timeval will cause time overflow in 2038, replacing it with
>> ktime_t. And we don't need to handle sec and nsec separately.
>> 
> 
> This part looks good now, but as I commented in version 1, it should
> really be a separate patch rather than combined with the rest that
> is dealing with another use of timeval.

Ok, I will split it in next version.

>> -do_gettimeofday();
>> -tv.tv_usec += USEC_PER_SEC * (tv.tv_sec - mlc->instart.tv_sec);
>> -tv.tv_usec -= mlc->instart.tv_usec;
>> -if (tv.tv_usec >= mlc->intimeout) goto sched;
>> -tv.tv_usec = (mlc->intimeout - tv.tv_usec) * HZ / USEC_PER_SEC;
>> -if (!tv.tv_usec) goto sched;
>> -mod_timer(_mlcs_kicker, jiffies + tv.tv_usec);
>> +if (tmp.tv64 >= (mlc->intimeout * NSEC_PER_USEC))
>> +goto sched;
>> +tmp.tv64 = mlc->intimeout * NSEC_PER_USEC - tmp.tv64;
>> +if (tmp.tv64 < NSEC_PER_USEC)
>> +goto sched;
>> +mod_timer(_mlcs_kicker,
>> +jiffies + nsecs_to_jiffies(tmp.tv64));
>>  break;
>>  sched:
>>  tasklet_schedule(_mlcs_tasklet);
> 
> If I read this right, the code is executed one for each input event such
> as a keypress or mouse movement. In the latter case, doing nsecs_to_jiffies()
> here is actually a bit expensive, and I stil think it can be avoided
> by just using jiffies.
> 
> For the (tmp.tv64 < NSEC_PER_USEC) part, did you just do that because
> I said this, or did you actually prove that it is required? I'm still
> confused about what the driver is trying to achieve here.

More explanation here:)
the judgement here is to prevent mod_timer with zero delta. I can not make sure 
whether the module
have nanosecond precise, so just keep same.

> 
>> diff --git a/drivers/input/serio/hp_sdc_mlc.c 
>> b/drivers/input/serio/hp_sdc_mlc.c
>> index d50f067..0a27b89 100644
>> --- a/drivers/input/serio/hp_sdc_mlc.c
>> +++ b/drivers/input/serio/hp_sdc_mlc.c
>> @@ -149,7 +149,8 @@ static int hp_sdc_mlc_in(hil_mlc *mlc, suseconds_t 
>> timeout)
>> 
>>  /* Try to down the semaphore */
>>  if (down_trylock(>isem)) {
>> -struct timeval tv;
>> +ktime_t tmp = ktime_sub(ktime_get(), mlc->instart);
>> +
>>  if (priv->emtestmode) {
>>  mlc->ipacket[0] =
>>  HIL_ERR_INT | (mlc->opacket &
> 
> Maybe give the variable a more useful name than 'tmp'?
> 
> You could also remove the variable entirely if you slightly restructure
> the condition below.

I see, thanks for point out.

Pingbo

___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH V2] hil_mlc: convert timeval to ktime_t

2015-10-23 Thread Arnd Bergmann 
On Friday 23 October 2015 17:24:59 WEN Pingbo wrote:
> Using struct timeval will cause time overflow in 2038, replacing it with
> ktime_t. And we don't need to handle sec and nsec separately.
> 
> Since mlc->lcv_t is only interested in seconds, directly using
> time64_t here.
> 
> And monotonic time is better here, since the original driver don't care
> the wall time.
> 
> In addition, the original driver try to covert usec to jiffies manually in
> hilse_donode(). This is not a universal and safe way, using
> nsecs_to_jiffies() to fix that.
> 
> Signed-off-by: WEN Pingbo 

The changelog text looks good.

> +++ b/drivers/input/serio/hil_mlc.c
> @@ -274,14 +274,12 @@ static int hilse_match(hil_mlc *mlc, int unused)
>  /* An LCV used to prevent runaway loops, forces 5 second sleep when reset. */
>  static int hilse_init_lcv(hil_mlc *mlc, int unused)
>  {
> - struct timeval tv;
> + time64_t now = ktime_get_seconds();
>  
> - do_gettimeofday();
> -
> - if (mlc->lcv && (tv.tv_sec - mlc->lcv_tv.tv_sec) < 5)
> + if (mlc->lcv && (now - mlc->lcv_t) < 5)
>   return -1;
>  
> - mlc->lcv_tv = tv;
> + mlc->lcv_t = now;
>   mlc->lcv = 0;
>  
>   return 0;

This part looks good now, but as I commented in version 1, it should
really be a separate patch rather than combined with the rest that
is dealing with another use of timeval.
>  
> - do_gettimeofday();
> - tv.tv_usec += USEC_PER_SEC * (tv.tv_sec - mlc->instart.tv_sec);
> - tv.tv_usec -= mlc->instart.tv_usec;
> - if (tv.tv_usec >= mlc->intimeout) goto sched;
> - tv.tv_usec = (mlc->intimeout - tv.tv_usec) * HZ / USEC_PER_SEC;
> - if (!tv.tv_usec) goto sched;
> - mod_timer(_mlcs_kicker, jiffies + tv.tv_usec);
> + if (tmp.tv64 >= (mlc->intimeout * NSEC_PER_USEC))
> + goto sched;
> + tmp.tv64 = mlc->intimeout * NSEC_PER_USEC - tmp.tv64;
> + if (tmp.tv64 < NSEC_PER_USEC)
> + goto sched;
> + mod_timer(_mlcs_kicker,
> + jiffies + nsecs_to_jiffies(tmp.tv64));
>   break;
>   sched:
>   tasklet_schedule(_mlcs_tasklet);

If I read this right, the code is executed one for each input event such
as a keypress or mouse movement. In the latter case, doing nsecs_to_jiffies()
here is actually a bit expensive, and I stil think it can be avoided
by just using jiffies.

For the (tmp.tv64 < NSEC_PER_USEC) part, did you just do that because
I said this, or did you actually prove that it is required? I'm still
confused about what the driver is trying to achieve here.

> diff --git a/drivers/input/serio/hp_sdc_mlc.c 
> b/drivers/input/serio/hp_sdc_mlc.c
> index d50f067..0a27b89 100644
> --- a/drivers/input/serio/hp_sdc_mlc.c
> +++ b/drivers/input/serio/hp_sdc_mlc.c
> @@ -149,7 +149,8 @@ static int hp_sdc_mlc_in(hil_mlc *mlc, suseconds_t 
> timeout)
>  
>   /* Try to down the semaphore */
>   if (down_trylock(>isem)) {
> - struct timeval tv;
> + ktime_t tmp = ktime_sub(ktime_get(), mlc->instart);
> +
>   if (priv->emtestmode) {
>   mlc->ipacket[0] =
>   HIL_ERR_INT | (mlc->opacket &

Maybe give the variable a more useful name than 'tmp'?

You could also remove the variable entirely if you slightly restructure
the condition below.

> @@ -160,9 +161,8 @@ static int hp_sdc_mlc_in(hil_mlc *mlc, suseconds_t 
> timeout)
>   /* printk(KERN_DEBUG PREFIX ">[%x]\n", 
> mlc->ipacket[0]); */
>   goto wasup;
>   }
> - do_gettimeofday();
> - tv.tv_usec += USEC_PER_SEC * (tv.tv_sec - mlc->instart.tv_sec);
> - if (tv.tv_usec - mlc->instart.tv_usec > mlc->intimeout) {
> +
> + if (tmp.tv64 > mlc->intimeout * NSEC_PER_USEC) {
>   /*  printk("!%i %i",
>   tv.tv_usec - mlc->instart.tv_usec,
>   mlc->intimeout);

As I mentioned, better use ktime_to_ns() instead of accessing the tv64 member
directly, but that is just a small style question.

Arnd
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

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

2015-10-23 Thread 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
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

[Y2038] [PATCH V3 2/2] hil_mlc: convert timeval to jiffies

2015-10-23 Thread WEN Pingbo 
struct timeval is not y2038 safe, and what mlc->instart do is
scheduling a task in a fixed timeout, so jiffies is the
simplest choice here.

In hilse_donode(), the expires in mod_timer equals

jiffies + intimeout - (now - instart)

If we use jiffies in 'now', the expires equals

instart + intimeout

So, all we need to do is that making sure expires is a future
timestamp before passed it to mod_timer

Signed-off-by: WEN Pingbo 
---

Version 2:
Using ktime_t to fix y2038 problem
Version 3:
Convert it to jiffies

 drivers/input/serio/hil_mlc.c| 17 +++--
 drivers/input/serio/hp_sdc_mlc.c |  7 +++
 include/linux/hil_mlc.h  |  2 +-
 3 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/input/serio/hil_mlc.c b/drivers/input/serio/hil_mlc.c
index fb297aa..5428098 100644
--- a/drivers/input/serio/hil_mlc.c
+++ b/drivers/input/serio/hil_mlc.c
@@ -603,7 +603,7 @@ static inline void hilse_setup_input(hil_mlc *mlc, const 
struct hilse_node *node
}
mlc->istarted = 1;
mlc->intimeout = node->arg;
-   do_gettimeofday(&(mlc->instart));
+   mlc->instart = jiffies;
mlc->icount = 15;
memset(mlc->ipacket, 0, 16 * sizeof(hil_packet));
BUG_ON(down_trylock(>isem));
@@ -708,7 +708,7 @@ static int hilse_donode(hil_mlc *mlc)
break;
}
mlc->ostarted = 0;
-   do_gettimeofday(&(mlc->instart));
+   mlc->instart = jiffies;
write_unlock_irqrestore(>lock, flags);
nextidx = HILSEN_NEXT;
break;
@@ -729,18 +729,15 @@ static int hilse_donode(hil_mlc *mlc)
 #endif
 
while (nextidx & HILSEN_SCHED) {
-   struct timeval tv;
+   unsigned long expires = mlc->instart +
+   usecs_to_jiffies(mlc->intimeout);
 
if (!sched_long)
goto sched;
 
-   do_gettimeofday();
-   tv.tv_usec += USEC_PER_SEC * (tv.tv_sec - mlc->instart.tv_sec);
-   tv.tv_usec -= mlc->instart.tv_usec;
-   if (tv.tv_usec >= mlc->intimeout) goto sched;
-   tv.tv_usec = (mlc->intimeout - tv.tv_usec) * HZ / USEC_PER_SEC;
-   if (!tv.tv_usec) goto sched;
-   mod_timer(_mlcs_kicker, jiffies + tv.tv_usec);
+   if (time_after_eq(jiffies, expires))
+   goto sched;
+   mod_timer(_mlcs_kicker, expires);
break;
sched:
tasklet_schedule(_mlcs_tasklet);
diff --git a/drivers/input/serio/hp_sdc_mlc.c b/drivers/input/serio/hp_sdc_mlc.c
index d50f067..b91d5bb 100644
--- a/drivers/input/serio/hp_sdc_mlc.c
+++ b/drivers/input/serio/hp_sdc_mlc.c
@@ -149,7 +149,6 @@ static int hp_sdc_mlc_in(hil_mlc *mlc, suseconds_t timeout)
 
/* Try to down the semaphore */
if (down_trylock(>isem)) {
-   struct timeval tv;
if (priv->emtestmode) {
mlc->ipacket[0] =
HIL_ERR_INT | (mlc->opacket &
@@ -160,9 +159,9 @@ static int hp_sdc_mlc_in(hil_mlc *mlc, suseconds_t timeout)
/* printk(KERN_DEBUG PREFIX ">[%x]\n", 
mlc->ipacket[0]); */
goto wasup;
}
-   do_gettimeofday();
-   tv.tv_usec += USEC_PER_SEC * (tv.tv_sec - mlc->instart.tv_sec);
-   if (tv.tv_usec - mlc->instart.tv_usec > mlc->intimeout) {
+
+   if (time_after(jiffies,
+   mlc->instart + usecs_to_jiffies(mlc->intimeout))) {
/*  printk("!%i %i",
tv.tv_usec - mlc->instart.tv_usec,
mlc->intimeout);
diff --git a/include/linux/hil_mlc.h b/include/linux/hil_mlc.h
index 29bb5e3..feb167d 100644
--- a/include/linux/hil_mlc.h
+++ b/include/linux/hil_mlc.h
@@ -144,7 +144,7 @@ struct hil_mlc {
hil_packet  ipacket[16];
hil_packet  imatch;
int icount;
-   struct timeval  instart;
+   unsigned long   instart; /* in jiffies */
suseconds_t intimeout;
 
int ddi;/* Last operational device id */
-- 
1.9.1

___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [Outreachy kernel] Re: [PATCH] char: ipmi: ipmi_ssif: Replace timeval with timespec64

2015-10-23 Thread Arnd Bergmann 
On Saturday 24 October 2015 00:58:12 Amitoj Kaur Chawla wrote:
> On Sat, Oct 24, 2015 at 12:53 AM, Arnd Bergmann  wrote:
> > On Saturday 24 October 2015 00:39:22 Amitoj Kaur Chawla wrote:
> >> This patch replaces timeval with timespec64 as 32 bit 'struct timeval'
> >> will not give current time beyond 2038.
> >>
> >> The patch changes the code to use ktime_get_real_ts64() which returns
> >> a 'struct timespec64' instead of do_gettimeofday() which returns a
> >> 'struct timeval'
> >>
> >> This patch also alters the format strings in pr_info() for now.tv_sec
> >> and now.tv_nsec to incorporate 'long long' on 32 bit architectures and
> >> leading zeroes respectively.
> >>
> >> Signed-off-by: Amitoj Kaur Chawla 
> >> ---
> >
> > The patch looks correct, but I think that this time the format string
> > was actually ok already for the microsecond value. Just leave it at "%6.6ld"
> > and send it again with the maintainers on Cc.
> >
> 
> Oh okay can I ask the reason that the change was required there but not here?
> 
> 

The other one was "%lu", which has no leading zeroes, while this one
was "%6.6ld", which does.

I keep getting confused by printf format strings and sometimes have
to try these out myself, but my understanding is that these all
behave the same way:

 "%06ld"
 "%.6ld"
 "%6.6ld"
 "%6.06ld"

Arnd
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH] char: ipmi: ipmi_ssif: Replace timeval with timespec64

2015-10-23 Thread Arnd Bergmann 
On Saturday 24 October 2015 00:39:22 Amitoj Kaur Chawla wrote:
> This patch replaces timeval with timespec64 as 32 bit 'struct timeval'
> will not give current time beyond 2038.
> 
> The patch changes the code to use ktime_get_real_ts64() which returns
> a 'struct timespec64' instead of do_gettimeofday() which returns a
> 'struct timeval'
> 
> This patch also alters the format strings in pr_info() for now.tv_sec
> and now.tv_nsec to incorporate 'long long' on 32 bit architectures and
> leading zeroes respectively.
> 
> Signed-off-by: Amitoj Kaur Chawla 
> ---

The patch looks correct, but I think that this time the format string
was actually ok already for the microsecond value. Just leave it at "%6.6ld"
and send it again with the maintainers on Cc.

>  drivers/char/ipmi/ipmi_ssif.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index 877205d..926add4 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -52,6 +52,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define PFX "ipmi_ssif: "
>  #define DEVICE_NAME "ipmi_ssif"
> @@ -1041,12 +1042,12 @@ static void sender(void*send_info,
>   start_next_msg(ssif_info, flags);
>  
>   if (ssif_info->ssif_debug & SSIF_DEBUG_TIMING) {
> - struct timeval t;
> + struct timespec64 t;
>  
> - do_gettimeofday();
> - pr_info("**Enqueue %02x %02x: %ld.%6.6ld\n",
> + ktime_get_real_ts64();
> + pr_info("**Enqueue %02x %02x: %lld.%6.06ld\n",
>  msg->data[0], msg->data[1],
> -(long) t.tv_sec, (long) t.tv_usec);
> +(long long) t.tv_sec, (long) t.tv_nsec / NSEC_PER_USEC);
>   }
>  }
>  
> 

___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [PATCH v2] char: ipmi: ipmi_ssif: Replace timeval with timespec64

2015-10-23 Thread Arnd Bergmann 
On Saturday 24 October 2015 01:21:04 Amitoj Kaur Chawla wrote:
> This patch replaces timeval with timespec64 as 32 bit 'struct timeval'
> will not give current time beyond 2038.
> 
> The patch changes the code to use ktime_get_real_ts64() which returns
> a 'struct timespec64' instead of do_gettimeofday() which returns a
> 'struct timeval'
> 
> This patch also alters the format string in pr_info() for now.tv_sec
> to incorporate 'long long' on 32 bit architectures.
> 
> Signed-off-by: Amitoj Kaur Chawla 
> 

Reviewed-by: Arnd Bergmann 
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [Outreachy kernel] Re: [PATCH] char: ipmi: ipmi_ssif: Replace timeval with timespec64

2015-10-23 Thread Amitoj Kaur Chawla 
On Sat, Oct 24, 2015 at 1:04 AM, Arnd Bergmann  wrote:
> On Saturday 24 October 2015 00:58:12 Amitoj Kaur Chawla wrote:
>> On Sat, Oct 24, 2015 at 12:53 AM, Arnd Bergmann  wrote:
>> > On Saturday 24 October 2015 00:39:22 Amitoj Kaur Chawla wrote:
>> >> This patch replaces timeval with timespec64 as 32 bit 'struct timeval'
>> >> will not give current time beyond 2038.
>> >>
>> >> The patch changes the code to use ktime_get_real_ts64() which returns
>> >> a 'struct timespec64' instead of do_gettimeofday() which returns a
>> >> 'struct timeval'
>> >>
>> >> This patch also alters the format strings in pr_info() for now.tv_sec
>> >> and now.tv_nsec to incorporate 'long long' on 32 bit architectures and
>> >> leading zeroes respectively.
>> >>
>> >> Signed-off-by: Amitoj Kaur Chawla 
>> >> ---
>> >
>> > The patch looks correct, but I think that this time the format string
>> > was actually ok already for the microsecond value. Just leave it at 
>> > "%6.6ld"
>> > and send it again with the maintainers on Cc.
>> >
>>
>> Oh okay can I ask the reason that the change was required there but not here?
>>
>>
>
> The other one was "%lu", which has no leading zeroes, while this one
> was "%6.6ld", which does.
>
> I keep getting confused by printf format strings and sometimes have
> to try these out myself, but my understanding is that these all
> behave the same way:
>
>  "%06ld"
>  "%.6ld"
>  "%6.6ld"
>  "%6.06ld"
>
> Arnd

Oh, yes in the previous change I had tried for "%lu" myself, with
several examples. This time I didn't, not paying attention to the
change. My bad.

-- 
Amitoj
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

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

2015-10-23 Thread Arnd Bergmann 
On Friday 23 October 2015 19:28:00 Amitoj Kaur Chawla wrote:
> On Fri, Oct 23, 2015 at 7:17 PM, Arnd Bergmann  wrote:
> > On Friday 23 October 2015 15:39:49 Arnd Bergmann wrote:
> >> On Friday 23 October 2015 17:52:06 Amitoj Kaur Chawla wrote:
> >> > This patch replaces timeval with timespec64 as 32 bit 'struct timeval'
> >> > will not give current time beyond 2038.
> >> >
> >> > The patch changes the code to use ktime_get_real_ts64() which returns
> >> > a 'struct timespec64' instead of do_gettimeofday() which returns a
> >> > 'struct timeval'
> >> >
> >> > This patch also alters the format strings in sprintf() for now.tv_sec and
> >> > now.tv_nsec to incorporate 'long long' on 32 bit architectures and
> >> > leading zeroes respectively.
> >> >
> >> > Signed-off-by: Amitoj Kaur Chawla 
> >>
> >> Reviewed-by: Arnd Bergmann 
> >
> > I was going to suggest to send this to the maintainers now, but after
> > checking the MAINTAINERS file myself, I see that this would be Greg and me 
> >
> > Greg, can you pick this up into the char-misc tree?
> >
> > Arnd
> 
> Hi Arnd,
> 
> I made a slight error while sending v2 which I caught after figuring
> out of my issue of compiling the module.
> 
> I'm sending a v3 after build testing it.
> 
> Should I cc you and Greg separately or let it be?

In general, it helps to Cc maintainers. We are good at ignoring duplicate
mails.

Arnd
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

[Y2038] [PATCH] usb: usbtest: Add new ioctl interface

2015-10-23 Thread Deepa Dinamani 
On Thu, Oct 22, 2015 at 1:45 PM, Arnd Bergmann  wrote:

> On Thursday 22 October 2015 09:43:25 Deepa Dinamani wrote:
> > On Thu, Oct 22, 2015 at 2:36 AM, Arnd Bergmann  wrote:
> > > On Wednesday 21 October 2015 18:03:02 Deepa Dinamani wrote:
> > > > On Wed, Oct 21, 2015 at 4:17 PM, Arnd Bergmann 
> wrote:
> > > Yes, and that is the right idea. I suspect we will have to provide
> > > different definitions for kernel and user space though, in one
> > > form or another. You are right that we want to remove 'timeval'
> > > from the kernel, and in order to keep user space unchanged, this
> > > means defining the structure like
> > >
> > > struct usbtest_param_32 {
> > > /* inputs */
> > > __u32test_num;   /* 0..(TEST_CASES-1) */
> > > __u32iterations;
> > > ...
> > > __s32 duration_sec;
> > > __s32 duration_usec;
> > > };
> > >
> > > which is a well-defined binary version of what 32-bit user space
> > > sees today. We also need the 64-bit version of that, for both
> > > normal 64-bit tasks and future 32-bit tasks that are built with
> > > the old structure definition.
> > >
> > > Optionally, we can introduce a better interface with a new command
> > > number as your patch does, but that should be a separate patch,
> > > and we have to see if the USB maintainers want to do this or not.
> > >
> >
> > There are two problems with the original ioctl interface:
> >
> > 1. Because of the use of timeval, compatibility is broken between 32 bit
> > and 64 bit binaries.
> >
> > This has nothing to do with y2038 problem at all.
> > This is the case with all interfaces using timeval itself and has nothing
> > to do with this one
> > particular bad interface design.
>
> Right.
>
> > The struct you suggested above will work to map to two separate ioctls.
> > But, if this is a generic problem, shouldn't the above solution be in
> some
> > common file?
> > For instance we could have this:
> >
> > struct timeval_32 {
> > __s32 duration_sec;
> > __s32 duration_usec;
> > };
>
> This is essentially 'struct compat_timeval', which I intend to keep
> around in the kernel for backwards compatibility and will make available
> to 32-bit kernels that currently can't use it. The patches still need
> a few more review rounds though.
>
> > And, a similar struct for timeval_64.
> >
> > This would also mean adding api's to fill the structures defined above.
> > Basically an entire layer.
>
> We should really see how many drivers need this. I have shown that the
> core kernel does not need it with my patches, as all system calls that
> use timeval are already deprecated. I have a list of drivers that need
> to be converted at
>
>
> https://docs.google.com/spreadsheets/d/1HCYwHXxs48TsTb6IGUduNjQnmfRvMPzCN6T_0YiQwis/edit#gid=346406462
>
> on the second sheet. All the ones that have an ABI exposed to user space
> are marked 'uapi' or 'ioctl', and we could check which ones of these
> would be helped by having a generic set of helpers for timeval_64.
>

Ok. I can do this. I can get back to you early next week.

>
> > This is not necessary for this driver as the struct's are not exposed.
> > My guess is also that there aren't many applications using this because
> of
> > the way it needs redeclaring everything in the application.
>
> Agreed. My guess is that there is only one application using it at all,
> but it's hard to know for sure. If we could prove that there is only one,
> and we can change it to use a new ioctl command if that is present in some
> header file, all this could be simplified.
>

Ok. I can contact the usb linux mailing list if they know of any other or
just to fish around for their opinion.
I'll keep you in the cc. Let me know if you would prefer the whole 2038/
outreachy mailing list instead.

>
> > Since the original implementation is broken already, my first preference
> > was to fix the interface with the new interface itself.
>
> I wouldn't call the original implementation broken, except for the
> 64-bit compat problem. What makes it broken is that the ioctl data
> structure changes in user space when that changes the defintion of
> 'struct timeval', but since the data returned here is a difference
> of two times, the current 32-bit tv_sec variable is actually good
> enough.
>
>
I don't think seconds granularity is sufficient here for usb operations.
This page shows some tests which run for less than a second:
http://blackfin.uclinux.org/doku.php?id=linux-kernel:usb-gadget:zero


> > My intention was to not fix the broken interface, but to replace or
> provide
> > a new interface.
> >
> > Wouldn't the following be better?
> >
> > #ifdef CONFIG_COMPAT
> >   old ioctl(code = 100)
> >use old timeval struct
> >   #if CONFIG_COMPAT_TIME
> >  use compat_timeval instead of timeval

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

2015-10-23 Thread 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
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038