Re: [PATCH] ktime: Fix ktime_divns to do signed division

[Trevor Cordes]

On 2015-05-01 John Stultz wrote:
> It was noted that the 32bit implementation of ktime_divns
> was doing unsgined division adn didn't properly handle
> negative values.
> 
> This patch fixes the problem by checking and preserving
> the sign bit, and then reapplying it if appropriate
> after the division.

I worked your new patch into my test system into Fedora 21's latest
3.19.5-200.fc21 (they haven't switched to 4.0 yet).  I had to add in
Nicolas' 8b618628 commit as well to get this new patch to go into
3.19.5.

After rebuild and reboot I can confirm this patch fixes my bug: irsend
doesn't hang lircd.  I'll report if anything else weird goes on, but
this all seems pretty straightforward so I doubt I'll have any problems.

> I'll send it out here shortly. If you could give it a spin at your
> leisure, and if it works give me a Tested-by: tag I'd appreciate it!

I'm not quite sure how to give a Tested-by, but from the minimal docs I
found on the net, I am trying below (after your Signed-off-by tag).  If
I need to do something else, please point me in the general direction
of instructions.

> Great work again on chasing this down, and thanks for helping with
> debugging and validating the fix!

Thanks!  I'm really glad this is getting worked out and am happy to
help.  It's a big step forward for me to move past simple bugzilla onto
kernel bisects.  I plan on giving a presentation about all this at my
local Linux user group.  It'll be nice to report the happy ending!  You
guys have been great.  I guess kernel bugzilla isn't the place to get
help: it's here on the LKML.

P.S. Here's the kernel bz link, we/I can close it with the results once
this is all done:
https://bugzilla.kernel.org/show_bug.cgi?id=95431

> Cc: Nicolas Pitre <nicolas.pitre@xxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Josh Boyer <jwboyer@xxxxxxxxxx>
> Cc: One Thousand Gnomes <gnomes@xxxxxxxxxxxxxxxxxxx>
> Reported-by: Trevor Cordes <trevor@xxxxxxxxxxxxx>
> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
Tested-by: Trevor Cordes <trevor@xxxxxxxxxxxxx> (runtime test on i686)
> ---
>  include/linux/ktime.h | 12 ++++++++++--
>  kernel/time/hrtimer.c | 11 +++++++++--
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/ktime.h b/include/linux/ktime.h
> index 5fc3d10..d947263 100644
> --- a/include/linux/ktime.h
> +++ b/include/linux/ktime.h
> @@ -166,12 +166,20 @@ static inline bool ktime_before(const ktime_t
> cmp1, const ktime_t cmp2) }
>  
>  #if BITS_PER_LONG < 64
> -extern u64 __ktime_divns(const ktime_t kt, s64 div);
> +extern s64 __ktime_divns(const ktime_t kt, s64 div);
>  static inline u64 ktime_divns(const ktime_t kt, s64 div)
>  {
>  	if (__builtin_constant_p(div) && !(div >> 32)) {
> -		u64 ns = kt.tv64;
> +		s64 ns = kt.tv64;
> +		int neg = 0;
> +
> +		if (ns < 0) {
> +			neg = 1;
> +			ns = -ns;
> +		}
>  		do_div(ns, div);
> +		if (neg)
> +			ns = -ns;
>  		return ns;
>  	} else {
>  		return __ktime_divns(kt, div);
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index 76d4bd9..4c1b294 100644
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -266,12 +266,17 @@ lock_hrtimer_base(const struct hrtimer *timer,
> unsigned long *flags) /*
>   * Divide a ktime value by a nanosecond value
>   */
> -u64 __ktime_divns(const ktime_t kt, s64 div)
> +s64 __ktime_divns(const ktime_t kt, s64 div)
>  {
> -	u64 dclc;
> +	s64 dclc;
>  	int sft = 0;
> +	int neg = 0;
>  
>  	dclc = ktime_to_ns(kt);
> +	if (dclc < 0) {
> +		neg = 1;
> +		dclc = -dclc;
> +	}
>  	/* Make sure the divisor is less than 2^32: */
>  	while (div >> 32) {
>  		sft++;
> @@ -279,6 +284,8 @@ u64 __ktime_divns(const ktime_t kt, s64 div)
>  	}
>  	dclc >>= sft;
>  	do_div(dclc, (unsigned long) div);
> +	if (neg)
> +		dclc = -dclc;
>  
>  	return dclc;
>  }

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