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

[Pingbo Wen <pingbo.wen@linaro.org>]

> 在 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