[PATCH] hil_mlc: convert timeval to timespec64

[PATCH] hil_mlc: convert timeval to timespec64

[WEN Pingbo]

Using struct timeval will cause time overflow in 2038, replacing it with
a 64bit version.

In addition, the origin 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 <pingbo.wen@linaro.org>
---
 drivers/input/serio/hil_mlc.c    | 31 +++++++++++++++++--------------
 drivers/input/serio/hp_sdc_mlc.c | 10 ++++++----
 include/linux/hil_mlc.h          |  4 ++--
 3 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/drivers/input/serio/hil_mlc.c b/drivers/input/serio/hil_mlc.c
index 65605e4..4e3b926 100644
--- a/drivers/input/serio/hil_mlc.c
+++ b/drivers/input/serio/hil_mlc.c
@@ -274,14 +274,14 @@ 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;
+	struct timespec64 ts64;
 
-	do_gettimeofday(&tv);
+	ktime_get_ts64(&ts64);
 
-	if (mlc->lcv && (tv.tv_sec - mlc->lcv_tv.tv_sec) < 5)
+	if (mlc->lcv && (ts64.tv_sec - mlc->lcv_ts64.tv_sec) < 5)
 		return -1;
 
-	mlc->lcv_tv = tv;
+	mlc->lcv_ts64 = ts64;
 	mlc->lcv = 0;
 
 	return 0;
@@ -605,7 +605,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));
+	ktime_get_ts64(&(mlc->instart));
 	mlc->icount = 15;
 	memset(mlc->ipacket, 0, 16 * sizeof(hil_packet));
 	BUG_ON(down_trylock(&mlc->isem));
@@ -710,7 +710,7 @@ static int hilse_donode(hil_mlc *mlc)
 			break;
 		}
 		mlc->ostarted = 0;
-		do_gettimeofday(&(mlc->instart));
+		ktime_get_ts64(&(mlc->instart));
 		write_unlock_irqrestore(&mlc->lock, flags);
 		nextidx = HILSEN_NEXT;
 		break;
@@ -731,18 +731,21 @@ static int hilse_donode(hil_mlc *mlc)
 #endif
 
 	while (nextidx & HILSEN_SCHED) {
-		struct timeval tv;
+		struct timespec64 ts64;
 
 		if (!sched_long)
 			goto sched;
 
-		do_gettimeofday(&tv);
-		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(&hil_mlcs_kicker, jiffies + tv.tv_usec);
+		ktime_get_ts64(&ts64);
+		ts64.tv_nsec += NSEC_PER_SEC *
+			(ts64.tv_sec - mlc->instart.tv_sec);
+		ts64.tv_nsec -= mlc->instart.tv_nsec;
+		if (ts64.tv_nsec >= (mlc->intimeout * NSEC_PER_USEC))
+			goto sched;
+		ts64.tv_nsec = mlc->intimeout * NSEC_PER_USEC - ts64.tv_nsec;
+		if (!ts64.tv_nsec) goto sched;
+		mod_timer(&hil_mlcs_kicker,
+				jiffies + nsecs_to_jiffies(ts64.tv_nsec));
 		break;
 	sched:
 		tasklet_schedule(&hil_mlcs_tasklet);
diff --git a/drivers/input/serio/hp_sdc_mlc.c b/drivers/input/serio/hp_sdc_mlc.c
index d50f067..369885d 100644
--- a/drivers/input/serio/hp_sdc_mlc.c
+++ b/drivers/input/serio/hp_sdc_mlc.c
@@ -149,7 +149,7 @@ static int hp_sdc_mlc_in(hil_mlc *mlc, suseconds_t timeout)
 
 	/* Try to down the semaphore */
 	if (down_trylock(&mlc->isem)) {
-		struct timeval tv;
+		struct timespec64 ts64;
 		if (priv->emtestmode) {
 			mlc->ipacket[0] =
 				HIL_ERR_INT | (mlc->opacket &
@@ -160,9 +160,11 @@ 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.tv_usec += USEC_PER_SEC * (tv.tv_sec - mlc->instart.tv_sec);
-		if (tv.tv_usec - mlc->instart.tv_usec > mlc->intimeout) {
+		ktime_get_ts64(&ts64);
+		ts64.tv_nsec += NSEC_PER_SEC *
+			(ts64.tv_sec - mlc->instart.tv_sec);
+		if (ts64.tv_nsec - mlc->instart.tv_nsec > 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..a6b62fb 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;
+	struct timespec64	instart;
 	suseconds_t		intimeout;
 
 	int			ddi;	/* Last operational device id */
 	int			lcv;	/* LCV to throttle loops */
-	struct timeval		lcv_tv; /* Time loop was started */
+	struct timespec64	lcv_ts64; /* Time loop was started */
 
 	int			di_map[7]; /* Maps below items to live devs */
 	struct hil_mlc_devinfo	di[HIL_MLC_DEVMEM];
-- 
1.9.1

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

[Arnd Bergmann]

On Sunday 18 October 2015 17:45:19 WEN Pingbo wrote:
> Using struct timeval will cause time overflow in 2038, replacing it with
> a 64bit version.
> 
> In addition, the origin 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 <pingbo.wen@linaro.org>

You should mention somewhere that you are also converting from real
time to monotonic time, and why this is done.

> ---
>  drivers/input/serio/hil_mlc.c    | 31 +++++++++++++++++--------------
>  drivers/input/serio/hp_sdc_mlc.c | 10 ++++++----
>  include/linux/hil_mlc.h          |  4 ++--
>  3 files changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/input/serio/hil_mlc.c b/drivers/input/serio/hil_mlc.c
> index 65605e4..4e3b926 100644
> --- a/drivers/input/serio/hil_mlc.c
> +++ b/drivers/input/serio/hil_mlc.c
> @@ -274,14 +274,14 @@ 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;
> +	struct timespec64 ts64;
>  
> -	do_gettimeofday(&tv);
> +	ktime_get_ts64(&ts64);
>  
> -	if (mlc->lcv && (tv.tv_sec - mlc->lcv_tv.tv_sec) < 5)
> +	if (mlc->lcv && (ts64.tv_sec - mlc->lcv_ts64.tv_sec) < 5)
>  		return -1;
>  
> -	mlc->lcv_tv = tv;
> +	mlc->lcv_ts64 = ts64;
>  	mlc->lcv = 0;

No need to rename the two variables here. Also, it seems we never access the
tv_nsec portion at all, so this could use the simpler ktime_get_seconds()
or even 'jiffies' instead.

> @@ -605,7 +605,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));
> +	ktime_get_ts64(&(mlc->instart));
>  	mlc->icount = 15;
>  	memset(mlc->ipacket, 0, 16 * sizeof(hil_packet));
>  	BUG_ON(down_trylock(&mlc->isem));

This looks unrelated to the change above, so I would suggest making separate patches.

> @@ -710,7 +710,7 @@ static int hilse_donode(hil_mlc *mlc)
>  			break;
>  		}
>  		mlc->ostarted = 0;
> -		do_gettimeofday(&(mlc->instart));
> +		ktime_get_ts64(&(mlc->instart));
>  		write_unlock_irqrestore(&mlc->lock, flags);
>  		nextidx = HILSEN_NEXT;
>  		break;
> @@ -731,18 +731,21 @@ static int hilse_donode(hil_mlc *mlc)
>  #endif
>  
>  	while (nextidx & HILSEN_SCHED) {
> -		struct timeval tv;
> +		struct timespec64 ts64;
>  
>  		if (!sched_long)
>  			goto sched;
>  
> -		do_gettimeofday(&tv);
> -		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(&hil_mlcs_kicker, jiffies + tv.tv_usec);
> +		ktime_get_ts64(&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.

> +		if (ts64.tv_nsec >= (mlc->intimeout * NSEC_PER_USEC))
> +			goto sched;
> +		ts64.tv_nsec = mlc->intimeout * NSEC_PER_USEC - ts64.tv_nsec;
> +		if (!ts64.tv_nsec) goto sched;

As you are modifying the line, you should also fix the coding style to
write

		if (!ts64.tv_nsec)
			goto sched;

I also notice that you modify the behavior here, by changing from
microsecond to nanosecond resolution, the equivalent of the original
would have been

		if (ts64.tv_nsec < NSECS_PER_USEC)


Your current version looks like it will practically never be true (meaning
you hit the exact nanosecond). Is this conditional actually needed at all
then? If it is, what is the intention and what should it be?

> +		mod_timer(&hil_mlcs_kicker,
> +				jiffies + nsecs_to_jiffies(ts64.tv_nsec));
>  		break;
>  	sched:
>  		tasklet_schedule(&hil_mlcs_tasklet);

This part seems like it would be easier to just use jiffies instead
of timspec64, to avoid having to convert it back.

> @@ -160,9 +160,11 @@ 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.tv_usec += USEC_PER_SEC * (tv.tv_sec - mlc->instart.tv_sec);
> -		if (tv.tv_usec - mlc->instart.tv_usec > mlc->intimeout) {
> +		ktime_get_ts64(&ts64);
> +		ts64.tv_nsec += NSEC_PER_SEC *
> +			(ts64.tv_sec - mlc->instart.tv_sec);
> +		if (ts64.tv_nsec - mlc->instart.tv_nsec > mlc->intimeout *
> +				NSEC_PER_USEC) {
>  			/*	printk("!%i %i",
>  				tv.tv_usec - mlc->instart.tv_usec,
>  				mlc->intimeout);

same here.

	Arnd

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

[Pingbo Wen]

On Monday, October 19, 2015 04:58 PM, Arnd Bergmann wrote:
> On Sunday 18 October 2015 17:45:19 WEN Pingbo wrote:
>> Using struct timeval will cause time overflow in 2038, replacing it with
>> a 64bit version.
>>
>> In addition, the origin 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 <pingbo.wen@linaro.org>
> 
> You should mention somewhere that you are also converting from real
> time to monotonic time, and why this is done.
> 
>> ---
>>  drivers/input/serio/hil_mlc.c    | 31 +++++++++++++++++--------------
>>  drivers/input/serio/hp_sdc_mlc.c | 10 ++++++----
>>  include/linux/hil_mlc.h          |  4 ++--
>>  3 files changed, 25 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/input/serio/hil_mlc.c b/drivers/input/serio/hil_mlc.c
>> index 65605e4..4e3b926 100644
>> --- a/drivers/input/serio/hil_mlc.c
>> +++ b/drivers/input/serio/hil_mlc.c
>> @@ -274,14 +274,14 @@ 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;
>> +	struct timespec64 ts64;
>>  
>> -	do_gettimeofday(&tv);
>> +	ktime_get_ts64(&ts64);
>>  
>> -	if (mlc->lcv && (tv.tv_sec - mlc->lcv_tv.tv_sec) < 5)
>> +	if (mlc->lcv && (ts64.tv_sec - mlc->lcv_ts64.tv_sec) < 5)
>>  		return -1;
>>  
>> -	mlc->lcv_tv = tv;
>> +	mlc->lcv_ts64 = ts64;
>>  	mlc->lcv = 0;
> 
> No need to rename the two variables here. Also, it seems we never access the
> tv_nsec portion at all, so this could use the simpler ktime_get_seconds()
> or even 'jiffies' instead.
> 
>> @@ -605,7 +605,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));
>> +	ktime_get_ts64(&(mlc->instart));
>>  	mlc->icount = 15;
>>  	memset(mlc->ipacket, 0, 16 * sizeof(hil_packet));
>>  	BUG_ON(down_trylock(&mlc->isem));
> 
> This looks unrelated to the change above, so I would suggest making separate patches.
> 
>> @@ -710,7 +710,7 @@ static int hilse_donode(hil_mlc *mlc)
>>  			break;
>>  		}
>>  		mlc->ostarted = 0;
>> -		do_gettimeofday(&(mlc->instart));
>> +		ktime_get_ts64(&(mlc->instart));
>>  		write_unlock_irqrestore(&mlc->lock, flags);
>>  		nextidx = HILSEN_NEXT;
>>  		break;
>> @@ -731,18 +731,21 @@ static int hilse_donode(hil_mlc *mlc)
>>  #endif
>>  
>>  	while (nextidx & HILSEN_SCHED) {
>> -		struct timeval tv;
>> +		struct timespec64 ts64;
>>  
>>  		if (!sched_long)
>>  			goto sched;
>>  
>> -		do_gettimeofday(&tv);
>> -		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(&hil_mlcs_kicker, jiffies + tv.tv_usec);
>> +		ktime_get_ts64(&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.

> 
>> +		if (ts64.tv_nsec >= (mlc->intimeout * NSEC_PER_USEC))
>> +			goto sched;
>> +		ts64.tv_nsec = mlc->intimeout * NSEC_PER_USEC - ts64.tv_nsec;
>> +		if (!ts64.tv_nsec) goto sched;
> 
> As you are modifying the line, you should also fix the coding style to
> write
> 
> 		if (!ts64.tv_nsec)
> 			goto sched;
> 
> I also notice that you modify the behavior here, by changing from
> microsecond to nanosecond resolution, the equivalent of the original
> would have been
> 
> 		if (ts64.tv_nsec < NSECS_PER_USEC)
> 
> 
> Your current version looks like it will practically never be true (meaning
> you hit the exact nanosecond). Is this conditional actually needed at all
> then? If it is, what is the intention and what should it be?
> 

Yes, compare to NSEC_PER_USEC is more safe, since we use nanosecond here. 

>> +		mod_timer(&hil_mlcs_kicker,
>> +				jiffies + nsecs_to_jiffies(ts64.tv_nsec));
>>  		break;
>>  	sched:
>>  		tasklet_schedule(&hil_mlcs_tasklet);
> 
> This part seems like it would be easier to just use jiffies instead
> of timspec64, to avoid having to convert it back.
> 
>> @@ -160,9 +160,11 @@ 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.tv_usec += USEC_PER_SEC * (tv.tv_sec - mlc->instart.tv_sec);
>> -		if (tv.tv_usec - mlc->instart.tv_usec > mlc->intimeout) {
>> +		ktime_get_ts64(&ts64);
>> +		ts64.tv_nsec += NSEC_PER_SEC *
>> +			(ts64.tv_sec - mlc->instart.tv_sec);
>> +		if (ts64.tv_nsec - mlc->instart.tv_nsec > mlc->intimeout *
>> +				NSEC_PER_USEC) {
>>  			/*	printk("!%i %i",
>>  				tv.tv_usec - mlc->instart.tv_usec,
>>  				mlc->intimeout);
> 
> same here.
> 
> 	Arnd
> 

[PATCH V2] hil_mlc: convert timeval to ktime_t

[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 <pingbo.wen@linaro.org>
---
 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(&tv);
-
-	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(&mlc->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(&mlc->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.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(&hil_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(&hil_mlcs_kicker,
+				jiffies + nsecs_to_jiffies(tmp.tv64));
 		break;
 	sched:
 		tasklet_schedule(&hil_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(&mlc->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.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..2222ec6 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		intimeout;
 
 	int			ddi;	/* Last operational device id */
 	int			lcv;	/* LCV to throttle loops */
-	struct timeval		lcv_tv; /* Time loop was started */
+	time64_t		lcv_t; /* Time loop was started */
 
 	int			di_map[7]; /* Maps below items to live devs */
 	struct hil_mlc_devinfo	di[HIL_MLC_DEVMEM];
-- 
1.9.1

[PATCH V2] hil_mlc: convert timeval to ktime_t

[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 <pingbo.wen@linaro.org>
---
 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(&tv);
-
-	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(&mlc->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(&mlc->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.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(&hil_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(&hil_mlcs_kicker,
+				jiffies + nsecs_to_jiffies(tmp.tv64));
 		break;
 	sched:
 		tasklet_schedule(&hil_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(&mlc->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.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..2222ec6 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		intimeout;
 
 	int			ddi;	/* Last operational device id */
 	int			lcv;	/* LCV to throttle loops */
-	struct timeval		lcv_tv; /* Time loop was started */
+	time64_t		lcv_t; /* Time loop was started */
 
 	int			di_map[7]; /* Maps below items to live devs */
 	struct hil_mlc_devinfo	di[HIL_MLC_DEVMEM];
-- 
1.9.1

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

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

[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.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(&hil_mlcs_kicker, jiffies + tv.tv_usec);
> >> +            ktime_get_ts64(&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

Re: [PATCH] hil_mlc: convert timeval to timespec64

[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.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(&hil_mlcs_kicker, jiffies + tv.tv_usec);
> >> +            ktime_get_ts64(&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

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

[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 <pingbo.wen@linaro.org>

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(&tv);
> -
> -	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.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(&hil_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(&hil_mlcs_kicker,
> +				jiffies + nsecs_to_jiffies(tmp.tv64));
>  		break;
>  	sched:
>  		tasklet_schedule(&hil_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(&mlc->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.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

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

[Pingbo Wen]

> 在 2015年10月23日，17:55，Arnd Bergmann <arnd@arndb.de> 写道：
> 
> 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.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(&hil_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(&hil_mlcs_kicker,
>> +				jiffies + nsecs_to_jiffies(tmp.tv64));
>> 		break;
>> 	sched:
>> 		tasklet_schedule(&hil_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(&mlc->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

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

[Pingbo Wen]

> 在 2015年10月23日，17:45，Arnd Bergmann <arnd@arndb.de> 写道：
> 
> 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);
>>>> -        
> 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).
> 

Make sense, I will try this in next version.

Thanks,
Pingbo

Re: [PATCH] hil_mlc: convert timeval to timespec64

[Pingbo Wen]

> 在 2015年10月23日，17:45，Arnd Bergmann <arnd@arndb.de> 写道：
> 
> 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);
>>>> -        
> 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).
> 

Make sense, I will try this in next version.

Thanks,
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

[Arnd Bergmann]

On Friday 23 October 2015 20:32:50 Pingbo Wen wrote:
> >> -            do_gettimeofday(&tv);
> >> -            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(&hil_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(&hil_mlcs_kicker,
> >> +                            jiffies + nsecs_to_jiffies(tmp.tv64));
> >>              break;
> >>      sched:
> >>              tasklet_schedule(&hil_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

[PATCH V3 1/2] hil_mlc: convert timeval to time64_t

[WEN Pingbo]

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

Signed-off-by: WEN Pingbo <pingbo.wen@linaro.org>
---

Version 2:
	Convert timeval to ktime_t
Version 3:
	Convert it to time64_t, since it's a better choice

 drivers/input/serio/hil_mlc.c | 8 +++-----
 include/linux/hil_mlc.h       | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/input/serio/hil_mlc.c b/drivers/input/serio/hil_mlc.c
index 65605e4..fb297aa 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(&tv);
-
-	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;
diff --git a/include/linux/hil_mlc.h b/include/linux/hil_mlc.h
index 394a840..29bb5e3 100644
--- a/include/linux/hil_mlc.h
+++ b/include/linux/hil_mlc.h
@@ -149,7 +149,7 @@ struct hil_mlc {
 
 	int			ddi;	/* Last operational device id */
 	int			lcv;	/* LCV to throttle loops */
-	struct timeval		lcv_tv; /* Time loop was started */
+	time64_t		lcv_t; /* Time loop was started */
 
 	int			di_map[7]; /* Maps below items to live devs */
 	struct hil_mlc_devinfo	di[HIL_MLC_DEVMEM];
-- 
1.9.1

[PATCH V3 1/2] hil_mlc: convert timeval to time64_t

[WEN Pingbo]

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

Signed-off-by: WEN Pingbo <pingbo.wen@linaro.org>
---

Version 2:
	Convert timeval to ktime_t
Version 3:
	Convert it to time64_t, since it's a better choice

 drivers/input/serio/hil_mlc.c | 8 +++-----
 include/linux/hil_mlc.h       | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/input/serio/hil_mlc.c b/drivers/input/serio/hil_mlc.c
index 65605e4..fb297aa 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(&tv);
-
-	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;
diff --git a/include/linux/hil_mlc.h b/include/linux/hil_mlc.h
index 394a840..29bb5e3 100644
--- a/include/linux/hil_mlc.h
+++ b/include/linux/hil_mlc.h
@@ -149,7 +149,7 @@ struct hil_mlc {
 
 	int			ddi;	/* Last operational device id */
 	int			lcv;	/* LCV to throttle loops */
-	struct timeval		lcv_tv; /* Time loop was started */
+	time64_t		lcv_t; /* Time loop was started */
 
 	int			di_map[7]; /* Maps below items to live devs */
 	struct hil_mlc_devinfo	di[HIL_MLC_DEVMEM];
-- 
1.9.1

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

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

[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 <pingbo.wen@linaro.org>
---

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(&mlc->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(&mlc->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.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(&hil_mlcs_kicker, jiffies + tv.tv_usec);
+		if (time_after_eq(jiffies, expires))
+			goto sched;
+		mod_timer(&hil_mlcs_kicker, expires);
 		break;
 	sched:
 		tasklet_schedule(&hil_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(&mlc->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.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

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

[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 <pingbo.wen@linaro.org>
---

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(&mlc->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(&mlc->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.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(&hil_mlcs_kicker, jiffies + tv.tv_usec);
+		if (time_after_eq(jiffies, expires))
+			goto sched;
+		mod_timer(&hil_mlcs_kicker, expires);
 		break;
 	sched:
 		tasklet_schedule(&hil_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(&mlc->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.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