* Re: [PATCH] clockevents: Sanitize ticks to nsec conversion
@ 2013-09-19 14:30 ` Thomas Gleixner
0 siblings, 0 replies; 62+ messages in thread
From: Thomas Gleixner @ 2013-09-19 14:30 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Ludovic Desroches, Russell King - ARM Linux, Marc Kleine-Budde,
nicolas.ferre, LKML, Marc Pignat, john.stultz, kernel,
Ronald Wahl, LAK
Marc Kleine-Budde pointed out, that commit 77cc982 "clocksource: use
clockevents_config_and_register() where possible" caused a regression
for some of the converted subarchs.
The reason is, that the clockevents core code converts the minimal
hardware tick delta to a nanosecond value for core internal
usage. This conversion is affected by integer math rounding loss, so
the backwards conversion to hardware ticks will likely result in a
value which is less than the configured hardware limitation. The
affected subarchs used their own workaround (SIGH!) which got lost in
the conversion.
The solution for the issue at hand is simple: adding evt->mult - 1 to
the shifted value before the integer divison in the core conversion
function takes care of it. But this only works for the case where for
the scaled math mult/shift pair "mult <= 1 << shift" is true. For the
case where "mult > 1 << shift" we can apply the rounding add only for
the minimum delta value to make sure that the backward conversion is
not less than the given hardware limit. For the upper bound we need to
omit the rounding add, because the backwards conversion is always
larger than the original latch value. That would violate the upper
bound of the hardware device.
Though looking closer at the details of that function reveals another
bogosity: The upper bounds check is broken as well. Checking for a
resulting "clc" value greater than KTIME_MAX after the conversion is
pointless. The conversion does:
u64 clc = (latch << evt->shift) / evt->mult;
So there is no sanity check for (latch << evt->shift) exceeding the
64bit boundary. The latch argument is "unsigned long", so on a 64bit
arch the handed in argument could easily lead to an unnoticed shift
overflow. With the above rounding fix applied the calculation before
the divison is:
u64 clc = (latch << evt->shift) + evt->mult - 1;
So we need to make sure, that neither the shift nor the rounding add
is overflowing the u64 boundary.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: nicolas.ferre@atmel.com
Cc: Marc Pignat <marc.pignat@hevs.ch>
Cc: john.stultz@linaro.org
Cc: kernel@pengutronix.de
Cc: Ronald Wahl <ronald.wahl@raritan.com>
Cc: LAK <linux-arm-kernel@lists.infradead.org>
Cc: u.kleine-koenig@pengutronix.de
Cc: Ludovic Desroches <ludovic.desroches@atmel.com>
---
kernel/time/clockevents.c | 64 +++++++++++++++++++++++++++++++++++-----------
1 file changed, 49 insertions(+), 15 deletions(-)
Index: linux-2.6/kernel/time/clockevents.c
===================================================================
--- linux-2.6.orig/kernel/time/clockevents.c
+++ linux-2.6/kernel/time/clockevents.c
@@ -33,29 +33,63 @@ struct ce_unbind {
int res;
};
-/**
- * clockevents_delta2ns - Convert a latch value (device ticks) to nanoseconds
- * @latch: value to convert
- * @evt: pointer to clock event device descriptor
- *
- * Math helper, returns latch value converted to nanoseconds (bound checked)
- */
-u64 clockevent_delta2ns(unsigned long latch, struct clock_event_device *evt)
+static u64 cev_delta2ns(unsigned long latch, struct clock_event_device *evt,
+ bool ismax)
{
u64 clc = (u64) latch << evt->shift;
+ u64 rnd = (u64) evt->mult - 1;
if (unlikely(!evt->mult)) {
evt->mult = 1;
WARN_ON(1);
}
+ /*
+ * Upper bound sanity check. If the backwards conversion is
+ * not equal latch, we know that the above shift overflowed.
+ */
+ if (clc >> evt->shift) != (u64)latch)
+ clc = ~0ULL;
+
+ /*
+ * Scaled math oddities:
+ *
+ * For mult <= (1 << shift) we can safely add mult - 1 to
+ * prevent integer rounding loss. So the backwards conversion
+ * from nsec to device ticks will be correct.
+ *
+ * For mult > (1 << shift), i.e. device frequency is > 1GHz we
+ * need to be careful. Adding mult - 1 will result in a value
+ * which when converted back to device ticks will be larger
+ * than latch by (mult / (1 << shift)) - 1. For the min_delta
+ * calculation we still want to apply this in order to stay
+ * above the minimum device ticks limit. For the upper limit
+ * we would end up with a latch value larger than the upper
+ * limit of the device, so we omit the add to stay below the
+ * device upper boundary.
+ *
+ * Also omit the add if it would overflow the u64 boundary.
+ */
+ if ((~0ULL - clc > rnd) &&
+ (!ismax || evt->mult <= (1U << evt->shift)))
+ clc += rnd;
+
do_div(clc, evt->mult);
- if (clc < 1000)
- clc = 1000;
- if (clc > KTIME_MAX)
- clc = KTIME_MAX;
- return clc;
+ /* Deltas less than 1usec are pointless noise */
+ return clc > 1000 ? clc : 1000;
+}
+
+/**
+ * clockevents_delta2ns - Convert a latch value (device ticks) to nanoseconds
+ * @latch: value to convert
+ * @evt: pointer to clock event device descriptor
+ *
+ * Math helper, returns latch value converted to nanoseconds (bound checked)
+ */
+u64 clockevent_delta2ns(unsigned long latch, struct clock_event_device *evt)
+{
+ return cev_delta2ns(latch, evt, false);
}
EXPORT_SYMBOL_GPL(clockevent_delta2ns);
@@ -380,8 +414,8 @@ void clockevents_config(struct clock_eve
sec = 600;
clockevents_calc_mult_shift(dev, freq, sec);
- dev->min_delta_ns = clockevent_delta2ns(dev->min_delta_ticks, dev);
- dev->max_delta_ns = clockevent_delta2ns(dev->max_delta_ticks, dev);
+ dev->min_delta_ns = cev_delta2ns(dev->min_delta_ticks, dev, false);
+ dev->max_delta_ns = cev_delta2ns(dev->max_delta_ticks, dev, true);
}
/**
^ permalink raw reply [flat|nested] 62+ messages in thread* [PATCH] clockevents: Sanitize ticks to nsec conversion
2013-09-19 14:30 ` Thomas Gleixner
@ 2013-09-19 20:03 ` Uwe Kleine-König
-1 siblings, 0 replies; 62+ messages in thread
From: Uwe Kleine-König @ 2013-09-19 20:03 UTC (permalink / raw)
To: linux-arm-kernel
Hi Thomas,
On Thu, Sep 19, 2013 at 04:30:37PM +0200, Thomas Gleixner wrote:
> Marc Kleine-Budde pointed out, that commit 77cc982 "clocksource: use
> clockevents_config_and_register() where possible" caused a regression
> for some of the converted subarchs.
>
> The reason is, that the clockevents core code converts the minimal
> hardware tick delta to a nanosecond value for core internal
> usage. This conversion is affected by integer math rounding loss, so
> the backwards conversion to hardware ticks will likely result in a
> value which is less than the configured hardware limitation. The
> affected subarchs used their own workaround (SIGH!) which got lost in
> the conversion.
>
> The solution for the issue at hand is simple: adding evt->mult - 1 to
> the shifted value before the integer divison in the core conversion
> function takes care of it. But this only works for the case where for
> the scaled math mult/shift pair "mult <= 1 << shift" is true. For the
> case where "mult > 1 << shift" we can apply the rounding add only for
> the minimum delta value to make sure that the backward conversion is
> not less than the given hardware limit. For the upper bound we need to
> omit the rounding add, because the backwards conversion is always
> larger than the original latch value. That would violate the upper
> bound of the hardware device.
>
> Though looking closer at the details of that function reveals another
> bogosity: The upper bounds check is broken as well. Checking for a
> resulting "clc" value greater than KTIME_MAX after the conversion is
> pointless. The conversion does:
>
> u64 clc = (latch << evt->shift) / evt->mult;
>
> So there is no sanity check for (latch << evt->shift) exceeding the
> 64bit boundary. The latch argument is "unsigned long", so on a 64bit
> arch the handed in argument could easily lead to an unnoticed shift
> overflow. With the above rounding fix applied the calculation before
> the divison is:
>
> u64 clc = (latch << evt->shift) + evt->mult - 1;
>
> So we need to make sure, that neither the shift nor the rounding add
> is overflowing the u64 boundary.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: nicolas.ferre at atmel.com
> Cc: Marc Pignat <marc.pignat@hevs.ch>
> Cc: john.stultz at linaro.org
> Cc: kernel at pengutronix.de
> Cc: Ronald Wahl <ronald.wahl@raritan.com>
> Cc: LAK <linux-arm-kernel@lists.infradead.org>
> Cc: u.kleine-koenig at pengutronix.de
> Cc: Ludovic Desroches <ludovic.desroches@atmel.com>
>
> ---
> kernel/time/clockevents.c | 64 +++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 49 insertions(+), 15 deletions(-)
>
> Index: linux-2.6/kernel/time/clockevents.c
> ===================================================================
> --- linux-2.6.orig/kernel/time/clockevents.c
> +++ linux-2.6/kernel/time/clockevents.c
> @@ -33,29 +33,63 @@ struct ce_unbind {
> int res;
> };
>
> -/**
> - * clockevents_delta2ns - Convert a latch value (device ticks) to nanoseconds
> - * @latch: value to convert
> - * @evt: pointer to clock event device descriptor
> - *
> - * Math helper, returns latch value converted to nanoseconds (bound checked)
> - */
> -u64 clockevent_delta2ns(unsigned long latch, struct clock_event_device *evt)
> +static u64 cev_delta2ns(unsigned long latch, struct clock_event_device *evt,
> + bool ismax)
> {
> u64 clc = (u64) latch << evt->shift;
> + u64 rnd = (u64) evt->mult - 1;
>
> if (unlikely(!evt->mult)) {
> evt->mult = 1;
> WARN_ON(1);
> }
I suggest to move the assignment to rnd below this if block as it
changes mult.
>
> + /*
> + * Upper bound sanity check. If the backwards conversion is
> + * not equal latch, we know that the above shift overflowed.
> + */
> + if (clc >> evt->shift) != (u64)latch)
You didn't compile test, did you? Also the cast on the rhs isn't needed.
> + clc = ~0ULL;
> +
> + /*
> + * Scaled math oddities:
> + *
> + * For mult <= (1 << shift) we can safely add mult - 1 to
> + * prevent integer rounding loss. So the backwards conversion
It doesn't prevent inexactness to add mult - 1. It (only) asserts that
the ns2delta(delta2ns(latch)) >= latch instead of ... <= latch when not
doing it.
> + * from nsec to device ticks will be correct.
> + *
> + * For mult > (1 << shift), i.e. device frequency is > 1GHz we
> + * need to be careful. Adding mult - 1 will result in a value
> + * which when converted back to device ticks will be larger
s/will/can/
> + * than latch by (mult / (1 << shift)) - 1. For the min_delta
s/by/by up to/
> + * calculation we still want to apply this in order to stay
> + * above the minimum device ticks limit. For the upper limit
> + * we would end up with a latch value larger than the upper
> + * limit of the device, so we omit the add to stay below the
> + * device upper boundary.
> + *
> + * Also omit the add if it would overflow the u64 boundary.
> + */
> + if ((~0ULL - clc > rnd) &&
> + (!ismax || evt->mult <= (1U << evt->shift)))
> + clc += rnd;
I would expect that
if (!ismax)
if (~0ULL - clc > rnd)
clc += rnd;
else
clc = ~0ULL;
is enough (and a tad more exact in the presence of an overflow). I have
to think about that though.
> +
> do_div(clc, evt->mult);
> - if (clc < 1000)
> - clc = 1000;
> - if (clc > KTIME_MAX)
> - clc = KTIME_MAX;
>
> - return clc;
> + /* Deltas less than 1usec are pointless noise */
> + return clc > 1000 ? clc : 1000;
> +}
> +
> +/**
> + * clockevents_delta2ns - Convert a latch value (device ticks) to nanoseconds
> + * @latch: value to convert
> + * @evt: pointer to clock event device descriptor
> + *
> + * Math helper, returns latch value converted to nanoseconds (bound checked)
> + */
> +u64 clockevent_delta2ns(unsigned long latch, struct clock_event_device *evt)
> +{
> + return cev_delta2ns(latch, evt, false);
Hmm, I wonder if you need to be more clever in the general case. So you
still may return a value > max_delta_ticks here if latch is big enough.
But see below ...
> }
> EXPORT_SYMBOL_GPL(clockevent_delta2ns);
>
> @@ -380,8 +414,8 @@ void clockevents_config(struct clock_eve
> sec = 600;
>
> clockevents_calc_mult_shift(dev, freq, sec);
> - dev->min_delta_ns = clockevent_delta2ns(dev->min_delta_ticks, dev);
> - dev->max_delta_ns = clockevent_delta2ns(dev->max_delta_ticks, dev);
> + dev->min_delta_ns = cev_delta2ns(dev->min_delta_ticks, dev, false);
> + dev->max_delta_ns = cev_delta2ns(dev->max_delta_ticks, dev, true);
Another improvement that came to my mind just now. For min_delta_ns you
want to assert that it results in a value >= min_delta_ticks when
converted back. For max_delta_ns you want ... value <= max_delta_ticks.
What about the values in between? They for sure should land in
[min_delta_ticks ... max_delta_ticks] when converted back and ideally
should be most exact. The latter part would mean to add (rnd / 2)
instead of rnd. I don't know yet how that would behave@the borders of
the [min_delta_ns ... max_delta_ns] interval, but I think you still need
to special-case that.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH] clockevents: Sanitize ticks to nsec conversion
@ 2013-09-19 20:03 ` Uwe Kleine-König
0 siblings, 0 replies; 62+ messages in thread
From: Uwe Kleine-König @ 2013-09-19 20:03 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Ludovic Desroches, Russell King - ARM Linux, Marc Kleine-Budde,
nicolas.ferre, LKML, Marc Pignat, john.stultz, kernel,
Ronald Wahl, LAK
Hi Thomas,
On Thu, Sep 19, 2013 at 04:30:37PM +0200, Thomas Gleixner wrote:
> Marc Kleine-Budde pointed out, that commit 77cc982 "clocksource: use
> clockevents_config_and_register() where possible" caused a regression
> for some of the converted subarchs.
>
> The reason is, that the clockevents core code converts the minimal
> hardware tick delta to a nanosecond value for core internal
> usage. This conversion is affected by integer math rounding loss, so
> the backwards conversion to hardware ticks will likely result in a
> value which is less than the configured hardware limitation. The
> affected subarchs used their own workaround (SIGH!) which got lost in
> the conversion.
>
> The solution for the issue at hand is simple: adding evt->mult - 1 to
> the shifted value before the integer divison in the core conversion
> function takes care of it. But this only works for the case where for
> the scaled math mult/shift pair "mult <= 1 << shift" is true. For the
> case where "mult > 1 << shift" we can apply the rounding add only for
> the minimum delta value to make sure that the backward conversion is
> not less than the given hardware limit. For the upper bound we need to
> omit the rounding add, because the backwards conversion is always
> larger than the original latch value. That would violate the upper
> bound of the hardware device.
>
> Though looking closer at the details of that function reveals another
> bogosity: The upper bounds check is broken as well. Checking for a
> resulting "clc" value greater than KTIME_MAX after the conversion is
> pointless. The conversion does:
>
> u64 clc = (latch << evt->shift) / evt->mult;
>
> So there is no sanity check for (latch << evt->shift) exceeding the
> 64bit boundary. The latch argument is "unsigned long", so on a 64bit
> arch the handed in argument could easily lead to an unnoticed shift
> overflow. With the above rounding fix applied the calculation before
> the divison is:
>
> u64 clc = (latch << evt->shift) + evt->mult - 1;
>
> So we need to make sure, that neither the shift nor the rounding add
> is overflowing the u64 boundary.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: nicolas.ferre@atmel.com
> Cc: Marc Pignat <marc.pignat@hevs.ch>
> Cc: john.stultz@linaro.org
> Cc: kernel@pengutronix.de
> Cc: Ronald Wahl <ronald.wahl@raritan.com>
> Cc: LAK <linux-arm-kernel@lists.infradead.org>
> Cc: u.kleine-koenig@pengutronix.de
> Cc: Ludovic Desroches <ludovic.desroches@atmel.com>
>
> ---
> kernel/time/clockevents.c | 64 +++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 49 insertions(+), 15 deletions(-)
>
> Index: linux-2.6/kernel/time/clockevents.c
> ===================================================================
> --- linux-2.6.orig/kernel/time/clockevents.c
> +++ linux-2.6/kernel/time/clockevents.c
> @@ -33,29 +33,63 @@ struct ce_unbind {
> int res;
> };
>
> -/**
> - * clockevents_delta2ns - Convert a latch value (device ticks) to nanoseconds
> - * @latch: value to convert
> - * @evt: pointer to clock event device descriptor
> - *
> - * Math helper, returns latch value converted to nanoseconds (bound checked)
> - */
> -u64 clockevent_delta2ns(unsigned long latch, struct clock_event_device *evt)
> +static u64 cev_delta2ns(unsigned long latch, struct clock_event_device *evt,
> + bool ismax)
> {
> u64 clc = (u64) latch << evt->shift;
> + u64 rnd = (u64) evt->mult - 1;
>
> if (unlikely(!evt->mult)) {
> evt->mult = 1;
> WARN_ON(1);
> }
I suggest to move the assignment to rnd below this if block as it
changes mult.
>
> + /*
> + * Upper bound sanity check. If the backwards conversion is
> + * not equal latch, we know that the above shift overflowed.
> + */
> + if (clc >> evt->shift) != (u64)latch)
You didn't compile test, did you? Also the cast on the rhs isn't needed.
> + clc = ~0ULL;
> +
> + /*
> + * Scaled math oddities:
> + *
> + * For mult <= (1 << shift) we can safely add mult - 1 to
> + * prevent integer rounding loss. So the backwards conversion
It doesn't prevent inexactness to add mult - 1. It (only) asserts that
the ns2delta(delta2ns(latch)) >= latch instead of ... <= latch when not
doing it.
> + * from nsec to device ticks will be correct.
> + *
> + * For mult > (1 << shift), i.e. device frequency is > 1GHz we
> + * need to be careful. Adding mult - 1 will result in a value
> + * which when converted back to device ticks will be larger
s/will/can/
> + * than latch by (mult / (1 << shift)) - 1. For the min_delta
s/by/by up to/
> + * calculation we still want to apply this in order to stay
> + * above the minimum device ticks limit. For the upper limit
> + * we would end up with a latch value larger than the upper
> + * limit of the device, so we omit the add to stay below the
> + * device upper boundary.
> + *
> + * Also omit the add if it would overflow the u64 boundary.
> + */
> + if ((~0ULL - clc > rnd) &&
> + (!ismax || evt->mult <= (1U << evt->shift)))
> + clc += rnd;
I would expect that
if (!ismax)
if (~0ULL - clc > rnd)
clc += rnd;
else
clc = ~0ULL;
is enough (and a tad more exact in the presence of an overflow). I have
to think about that though.
> +
> do_div(clc, evt->mult);
> - if (clc < 1000)
> - clc = 1000;
> - if (clc > KTIME_MAX)
> - clc = KTIME_MAX;
>
> - return clc;
> + /* Deltas less than 1usec are pointless noise */
> + return clc > 1000 ? clc : 1000;
> +}
> +
> +/**
> + * clockevents_delta2ns - Convert a latch value (device ticks) to nanoseconds
> + * @latch: value to convert
> + * @evt: pointer to clock event device descriptor
> + *
> + * Math helper, returns latch value converted to nanoseconds (bound checked)
> + */
> +u64 clockevent_delta2ns(unsigned long latch, struct clock_event_device *evt)
> +{
> + return cev_delta2ns(latch, evt, false);
Hmm, I wonder if you need to be more clever in the general case. So you
still may return a value > max_delta_ticks here if latch is big enough.
But see below ...
> }
> EXPORT_SYMBOL_GPL(clockevent_delta2ns);
>
> @@ -380,8 +414,8 @@ void clockevents_config(struct clock_eve
> sec = 600;
>
> clockevents_calc_mult_shift(dev, freq, sec);
> - dev->min_delta_ns = clockevent_delta2ns(dev->min_delta_ticks, dev);
> - dev->max_delta_ns = clockevent_delta2ns(dev->max_delta_ticks, dev);
> + dev->min_delta_ns = cev_delta2ns(dev->min_delta_ticks, dev, false);
> + dev->max_delta_ns = cev_delta2ns(dev->max_delta_ticks, dev, true);
Another improvement that came to my mind just now. For min_delta_ns you
want to assert that it results in a value >= min_delta_ticks when
converted back. For max_delta_ns you want ... value <= max_delta_ticks.
What about the values in between? They for sure should land in
[min_delta_ticks ... max_delta_ticks] when converted back and ideally
should be most exact. The latter part would mean to add (rnd / 2)
instead of rnd. I don't know yet how that would behave at the borders of
the [min_delta_ns ... max_delta_ns] interval, but I think you still need
to special-case that.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 62+ messages in thread* [PATCH] clockevents: Sanitize ticks to nsec conversion
2013-09-19 20:03 ` Uwe Kleine-König
@ 2013-09-20 9:56 ` Thomas Gleixner
-1 siblings, 0 replies; 62+ messages in thread
From: Thomas Gleixner @ 2013-09-20 9:56 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 19 Sep 2013, Uwe Kleine-K?nig wrote:
> > + u64 rnd = (u64) evt->mult - 1;
> >
> > if (unlikely(!evt->mult)) {
> > evt->mult = 1;
> > WARN_ON(1);
> > }
> I suggest to move the assignment to rnd below this if block as it
> changes mult.
True.
>
> + /*
> + * Upper bound sanity check. If the backwards conversion is
> + * not equal latch, we know that the above shift overflowed.
> + */
> + if (clc >> evt->shift) != (u64)latch)
You didn't compile test, did you? Also the cast on the rhs isn't needed.
I did. I just missed to refresh the patch before sending it :)
> > + * For mult <= (1 << shift) we can safely add mult - 1 to
> > + * prevent integer rounding loss. So the backwards conversion
> It doesn't prevent inexactness to add mult - 1. It (only) asserts that
> the ns2delta(delta2ns(latch)) >= latch instead of ... <= latch when not
> doing it.
For mult <= 1 << shift the conversion is always ending up with the
same latch value.
> > + * from nsec to device ticks will be correct.
> > + *
> > + * For mult > (1 << shift), i.e. device frequency is > 1GHz we
> > + * need to be careful. Adding mult - 1 will result in a value
> > + * which when converted back to device ticks will be larger
> s/will/can/
No, it will always be larger.
> > + * than latch by (mult / (1 << shift)) - 1. For the min_delta
> s/by/by up to/
>
> > + * calculation we still want to apply this in order to stay
> > + * above the minimum device ticks limit. For the upper limit
> > + * we would end up with a latch value larger than the upper
> > + * limit of the device, so we omit the add to stay below the
> > + * device upper boundary.
> > + *
> > + * Also omit the add if it would overflow the u64 boundary.
> > + */
> > + if ((~0ULL - clc > rnd) &&
> > + (!ismax || evt->mult <= (1U << evt->shift)))
> > + clc += rnd;
> I would expect that
>
> if (!ismax)
> if (~0ULL - clc > rnd)
> clc += rnd;
> else
> clc = ~0ULL;
>
> is enough (and a tad more exact in the presence of an overflow). I have
> to think about that though.
Errm.
1) We cannot add if we'd overflow
2) For mult <= 1 << shift it's always correct
3) for mult > 1 << shift we only apply it to the min value not the max
> > clockevents_calc_mult_shift(dev, freq, sec);
> > - dev->min_delta_ns = clockevent_delta2ns(dev->min_delta_ticks, dev);
> > - dev->max_delta_ns = clockevent_delta2ns(dev->max_delta_ticks, dev);
> > + dev->min_delta_ns = cev_delta2ns(dev->min_delta_ticks, dev, false);
> > + dev->max_delta_ns = cev_delta2ns(dev->max_delta_ticks, dev, true);
> Another improvement that came to my mind just now. For min_delta_ns you
> want to assert that it results in a value >= min_delta_ticks when
> converted back. For max_delta_ns you want ... value <= max_delta_ticks.
> What about the values in between? They for sure should land in
> [min_delta_ticks ... max_delta_ticks] when converted back and ideally
> should be most exact. The latter part would mean to add (rnd / 2)
> instead of rnd. I don't know yet how that would behave at the borders of
> the [min_delta_ns ... max_delta_ns] interval, but I think you still need
> to special-case that.
Again:
1) For mult <= 1 << shift the backwards conversion is always the same as
the input value.
2) For mult > 1 << shift the backwards conversion of the min value is
always > than the input value. And the backwards conversion of the
max value is always < than the input value.
The values between that are completely uninteresting as the
program_events code always converts from nsec to device ticks.
We clamp the delta between min_ns and max_ns. So due to the above any
min_ns <= delta <= max_ns
will after conversion fulfil
min_tick <= delta_tick <= max_tick
So what are you going to improve? Either the math works or it does not.
Thanks,
tglx
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH] clockevents: Sanitize ticks to nsec conversion
@ 2013-09-20 9:56 ` Thomas Gleixner
0 siblings, 0 replies; 62+ messages in thread
From: Thomas Gleixner @ 2013-09-20 9:56 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Ludovic Desroches, Russell King - ARM Linux, Marc Kleine-Budde,
nicolas.ferre, LKML, Marc Pignat, john.stultz, kernel,
Ronald Wahl, LAK
[-- Attachment #1: Type: TEXT/PLAIN, Size: 3908 bytes --]
On Thu, 19 Sep 2013, Uwe Kleine-König wrote:
> > + u64 rnd = (u64) evt->mult - 1;
> >
> > if (unlikely(!evt->mult)) {
> > evt->mult = 1;
> > WARN_ON(1);
> > }
> I suggest to move the assignment to rnd below this if block as it
> changes mult.
True.
>
> + /*
> + * Upper bound sanity check. If the backwards conversion is
> + * not equal latch, we know that the above shift overflowed.
> + */
> + if (clc >> evt->shift) != (u64)latch)
You didn't compile test, did you? Also the cast on the rhs isn't needed.
I did. I just missed to refresh the patch before sending it :)
> > + * For mult <= (1 << shift) we can safely add mult - 1 to
> > + * prevent integer rounding loss. So the backwards conversion
> It doesn't prevent inexactness to add mult - 1. It (only) asserts that
> the ns2delta(delta2ns(latch)) >= latch instead of ... <= latch when not
> doing it.
For mult <= 1 << shift the conversion is always ending up with the
same latch value.
> > + * from nsec to device ticks will be correct.
> > + *
> > + * For mult > (1 << shift), i.e. device frequency is > 1GHz we
> > + * need to be careful. Adding mult - 1 will result in a value
> > + * which when converted back to device ticks will be larger
> s/will/can/
No, it will always be larger.
> > + * than latch by (mult / (1 << shift)) - 1. For the min_delta
> s/by/by up to/
>
> > + * calculation we still want to apply this in order to stay
> > + * above the minimum device ticks limit. For the upper limit
> > + * we would end up with a latch value larger than the upper
> > + * limit of the device, so we omit the add to stay below the
> > + * device upper boundary.
> > + *
> > + * Also omit the add if it would overflow the u64 boundary.
> > + */
> > + if ((~0ULL - clc > rnd) &&
> > + (!ismax || evt->mult <= (1U << evt->shift)))
> > + clc += rnd;
> I would expect that
>
> if (!ismax)
> if (~0ULL - clc > rnd)
> clc += rnd;
> else
> clc = ~0ULL;
>
> is enough (and a tad more exact in the presence of an overflow). I have
> to think about that though.
Errm.
1) We cannot add if we'd overflow
2) For mult <= 1 << shift it's always correct
3) for mult > 1 << shift we only apply it to the min value not the max
> > clockevents_calc_mult_shift(dev, freq, sec);
> > - dev->min_delta_ns = clockevent_delta2ns(dev->min_delta_ticks, dev);
> > - dev->max_delta_ns = clockevent_delta2ns(dev->max_delta_ticks, dev);
> > + dev->min_delta_ns = cev_delta2ns(dev->min_delta_ticks, dev, false);
> > + dev->max_delta_ns = cev_delta2ns(dev->max_delta_ticks, dev, true);
> Another improvement that came to my mind just now. For min_delta_ns you
> want to assert that it results in a value >= min_delta_ticks when
> converted back. For max_delta_ns you want ... value <= max_delta_ticks.
> What about the values in between? They for sure should land in
> [min_delta_ticks ... max_delta_ticks] when converted back and ideally
> should be most exact. The latter part would mean to add (rnd / 2)
> instead of rnd. I don't know yet how that would behave at the borders of
> the [min_delta_ns ... max_delta_ns] interval, but I think you still need
> to special-case that.
Again:
1) For mult <= 1 << shift the backwards conversion is always the same as
the input value.
2) For mult > 1 << shift the backwards conversion of the min value is
always > than the input value. And the backwards conversion of the
max value is always < than the input value.
The values between that are completely uninteresting as the
program_events code always converts from nsec to device ticks.
We clamp the delta between min_ns and max_ns. So due to the above any
min_ns <= delta <= max_ns
will after conversion fulfil
min_tick <= delta_tick <= max_tick
So what are you going to improve? Either the math works or it does not.
Thanks,
tglx
^ permalink raw reply [flat|nested] 62+ messages in thread* [PATCH] clockevents: Sanitize ticks to nsec conversion
2013-09-20 9:56 ` Thomas Gleixner
@ 2013-09-20 20:41 ` Uwe Kleine-König
-1 siblings, 0 replies; 62+ messages in thread
From: Uwe Kleine-König @ 2013-09-20 20:41 UTC (permalink / raw)
To: linux-arm-kernel
Hi Thomas,
On Fri, Sep 20, 2013 at 11:56:27AM +0200, Thomas Gleixner wrote:
> On Thu, 19 Sep 2013, Uwe Kleine-K?nig wrote:
> > > + * For mult <= (1 << shift) we can safely add mult - 1 to
> > > + * prevent integer rounding loss. So the backwards conversion
> > It doesn't prevent inexactness to add mult - 1. It (only) asserts that
> > the ns2delta(delta2ns(latch)) >= latch instead of ... <= latch when not
> > doing it.
>
> For mult <= 1 << shift the conversion is always ending up with the
> same latch value.
Ah right, I missed that we're in the slow-clock-case.
> > > + * from nsec to device ticks will be correct.
> > > + *
> > > + * For mult > (1 << shift), i.e. device frequency is > 1GHz we
> > > + * need to be careful. Adding mult - 1 will result in a value
> > > + * which when converted back to device ticks will be larger
> > s/will/can/
>
> No, it will always be larger.
Hmm, consider a 1.25 GHz clock with shift = 2 and mult = 5. Then
ns2clc(clc2ns(1000)) = 1000. So it's not always larger!
In the fast-clock-case we have:
With x << shift = n * mult - k for k in [0 .. mult-1] and an integer n:
ns2clc(clc2ns(x))
= ns2clc(((x << shift) + mult - 1) / mult)
= ((((x << shift) + mult - 1) / mult) * mult) >> shift
= n * mult >> shift
= ((x << shift) + k) >> shift
= x + (k >> shift)
So ns2clc(clc2ns(x)) = x for all x > 0 that have
k = mult - ((x << shift) - 1) % mult - 1 < 1 << shift
So my correction still stands.
> > > + * than latch by (mult / (1 << shift)) - 1. For the min_delta
> > s/by/by up to/
>From the calculation above you can also see that this term is wrong. k
is smaller than mult (and there are values that realize k = mult - 1).
So the converted back value can be larger than latch by up to
(mult - 1) >> shift. This is zero for the slow-clock-case.
In the 1.25 GHz example above that means that the difference is up to 1,
not 0 as your term would imply. 1004 is an example where the conversion
to nano seconds and back to ticks results in a difference of 1.
> > > + * calculation we still want to apply this in order to stay
> > > + * above the minimum device ticks limit. For the upper limit
> > > + * we would end up with a latch value larger than the upper
> > > + * limit of the device, so we omit the add to stay below the
> > > + * device upper boundary.
> > > + *
> > > + * Also omit the add if it would overflow the u64 boundary.
> > > + */
> > > + if ((~0ULL - clc > rnd) &&
> > > + (!ismax || evt->mult <= (1U << evt->shift)))
> > > + clc += rnd;
> > I would expect that
> >
> > if (!ismax)
> > if (~0ULL - clc > rnd)
> > clc += rnd;
> > else
> > clc = ~0ULL;
> >
> > is enough (and a tad more exact in the presence of an overflow). I have
> > to think about that though.
>
> Errm.
>
> 1) We cannot add if we'd overflow
>
> 2) For mult <= 1 << shift it's always correct
>
> 3) for mult > 1 << shift we only apply it to the min value not the max
Yeah, I didn't say your code is wrong *here*. I just think that my
easier (and so probably faster) code is good enough.
> > > clockevents_calc_mult_shift(dev, freq, sec);
> > > - dev->min_delta_ns = clockevent_delta2ns(dev->min_delta_ticks, dev);
> > > - dev->max_delta_ns = clockevent_delta2ns(dev->max_delta_ticks, dev);
> > > + dev->min_delta_ns = cev_delta2ns(dev->min_delta_ticks, dev, false);
> > > + dev->max_delta_ns = cev_delta2ns(dev->max_delta_ticks, dev, true);
> > Another improvement that came to my mind just now. For min_delta_ns you
> > want to assert that it results in a value >= min_delta_ticks when
> > converted back. For max_delta_ns you want ... value <= max_delta_ticks.
> > What about the values in between? They for sure should land in
> > [min_delta_ticks ... max_delta_ticks] when converted back and ideally
> > should be most exact. The latter part would mean to add (rnd / 2)
> > instead of rnd. I don't know yet how that would behave at the borders of
> > the [min_delta_ns ... max_delta_ns] interval, but I think you still need
> > to special-case that.
>
> Again:
>
> 1) For mult <= 1 << shift the backwards conversion is always the same as
> the input value.
>
> 2) For mult > 1 << shift the backwards conversion of the min value is
> always > than the input value. And the backwards conversion of the
> max value is always < than the input value.
>
> The values between that are completely uninteresting as the
> program_events code always converts from nsec to device ticks.
>
> We clamp the delta between min_ns and max_ns. So due to the above any
>
> min_ns <= delta <= max_ns
>
> will after conversion fulfil
>
> min_tick <= delta_tick <= max_tick
>
> So what are you going to improve? Either the math works or it does not.
Right, my idea is nice, but useless.
So I suggest you resend your patch with the compile fix and the
corrected comment and I will think about my suggestion to simplify the
if condition independently as it's only a small runtime improvent and so
not important enough to stop the correctness issue your patch is fixing.
Best regards and thanks for the nice discussion,
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH] clockevents: Sanitize ticks to nsec conversion
@ 2013-09-20 20:41 ` Uwe Kleine-König
0 siblings, 0 replies; 62+ messages in thread
From: Uwe Kleine-König @ 2013-09-20 20:41 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Ludovic Desroches, Russell King - ARM Linux, Marc Kleine-Budde,
nicolas.ferre, LKML, Marc Pignat, john.stultz, kernel,
Ronald Wahl, LAK
Hi Thomas,
On Fri, Sep 20, 2013 at 11:56:27AM +0200, Thomas Gleixner wrote:
> On Thu, 19 Sep 2013, Uwe Kleine-König wrote:
> > > + * For mult <= (1 << shift) we can safely add mult - 1 to
> > > + * prevent integer rounding loss. So the backwards conversion
> > It doesn't prevent inexactness to add mult - 1. It (only) asserts that
> > the ns2delta(delta2ns(latch)) >= latch instead of ... <= latch when not
> > doing it.
>
> For mult <= 1 << shift the conversion is always ending up with the
> same latch value.
Ah right, I missed that we're in the slow-clock-case.
> > > + * from nsec to device ticks will be correct.
> > > + *
> > > + * For mult > (1 << shift), i.e. device frequency is > 1GHz we
> > > + * need to be careful. Adding mult - 1 will result in a value
> > > + * which when converted back to device ticks will be larger
> > s/will/can/
>
> No, it will always be larger.
Hmm, consider a 1.25 GHz clock with shift = 2 and mult = 5. Then
ns2clc(clc2ns(1000)) = 1000. So it's not always larger!
In the fast-clock-case we have:
With x << shift = n * mult - k for k in [0 .. mult-1] and an integer n:
ns2clc(clc2ns(x))
= ns2clc(((x << shift) + mult - 1) / mult)
= ((((x << shift) + mult - 1) / mult) * mult) >> shift
= n * mult >> shift
= ((x << shift) + k) >> shift
= x + (k >> shift)
So ns2clc(clc2ns(x)) = x for all x > 0 that have
k = mult - ((x << shift) - 1) % mult - 1 < 1 << shift
So my correction still stands.
> > > + * than latch by (mult / (1 << shift)) - 1. For the min_delta
> > s/by/by up to/
>From the calculation above you can also see that this term is wrong. k
is smaller than mult (and there are values that realize k = mult - 1).
So the converted back value can be larger than latch by up to
(mult - 1) >> shift. This is zero for the slow-clock-case.
In the 1.25 GHz example above that means that the difference is up to 1,
not 0 as your term would imply. 1004 is an example where the conversion
to nano seconds and back to ticks results in a difference of 1.
> > > + * calculation we still want to apply this in order to stay
> > > + * above the minimum device ticks limit. For the upper limit
> > > + * we would end up with a latch value larger than the upper
> > > + * limit of the device, so we omit the add to stay below the
> > > + * device upper boundary.
> > > + *
> > > + * Also omit the add if it would overflow the u64 boundary.
> > > + */
> > > + if ((~0ULL - clc > rnd) &&
> > > + (!ismax || evt->mult <= (1U << evt->shift)))
> > > + clc += rnd;
> > I would expect that
> >
> > if (!ismax)
> > if (~0ULL - clc > rnd)
> > clc += rnd;
> > else
> > clc = ~0ULL;
> >
> > is enough (and a tad more exact in the presence of an overflow). I have
> > to think about that though.
>
> Errm.
>
> 1) We cannot add if we'd overflow
>
> 2) For mult <= 1 << shift it's always correct
>
> 3) for mult > 1 << shift we only apply it to the min value not the max
Yeah, I didn't say your code is wrong *here*. I just think that my
easier (and so probably faster) code is good enough.
> > > clockevents_calc_mult_shift(dev, freq, sec);
> > > - dev->min_delta_ns = clockevent_delta2ns(dev->min_delta_ticks, dev);
> > > - dev->max_delta_ns = clockevent_delta2ns(dev->max_delta_ticks, dev);
> > > + dev->min_delta_ns = cev_delta2ns(dev->min_delta_ticks, dev, false);
> > > + dev->max_delta_ns = cev_delta2ns(dev->max_delta_ticks, dev, true);
> > Another improvement that came to my mind just now. For min_delta_ns you
> > want to assert that it results in a value >= min_delta_ticks when
> > converted back. For max_delta_ns you want ... value <= max_delta_ticks.
> > What about the values in between? They for sure should land in
> > [min_delta_ticks ... max_delta_ticks] when converted back and ideally
> > should be most exact. The latter part would mean to add (rnd / 2)
> > instead of rnd. I don't know yet how that would behave at the borders of
> > the [min_delta_ns ... max_delta_ns] interval, but I think you still need
> > to special-case that.
>
> Again:
>
> 1) For mult <= 1 << shift the backwards conversion is always the same as
> the input value.
>
> 2) For mult > 1 << shift the backwards conversion of the min value is
> always > than the input value. And the backwards conversion of the
> max value is always < than the input value.
>
> The values between that are completely uninteresting as the
> program_events code always converts from nsec to device ticks.
>
> We clamp the delta between min_ns and max_ns. So due to the above any
>
> min_ns <= delta <= max_ns
>
> will after conversion fulfil
>
> min_tick <= delta_tick <= max_tick
>
> So what are you going to improve? Either the math works or it does not.
Right, my idea is nice, but useless.
So I suggest you resend your patch with the compile fix and the
corrected comment and I will think about my suggestion to simplify the
if condition independently as it's only a small runtime improvent and so
not important enough to stop the correctness issue your patch is fixing.
Best regards and thanks for the nice discussion,
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH] clockevents: Sanitize ticks to nsec conversion
2013-09-20 20:41 ` Uwe Kleine-König
@ 2013-09-20 21:30 ` Thomas Gleixner
-1 siblings, 0 replies; 62+ messages in thread
From: Thomas Gleixner @ 2013-09-20 21:30 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 20 Sep 2013, Uwe Kleine-K?nig wrote:
> On Fri, Sep 20, 2013 at 11:56:27AM +0200, Thomas Gleixner wrote:
> > On Thu, 19 Sep 2013, Uwe Kleine-K?nig wrote:
> > > > + * from nsec to device ticks will be correct.
> > > > + *
> > > > + * For mult > (1 << shift), i.e. device frequency is > 1GHz we
> > > > + * need to be careful. Adding mult - 1 will result in a value
> > > > + * which when converted back to device ticks will be larger
> > > s/will/can/
> >
> > No, it will always be larger.
> Hmm, consider a 1.25 GHz clock with shift = 2 and mult = 5. Then
> ns2clc(clc2ns(1000)) = 1000. So it's not always larger!
> In the fast-clock-case we have:
> With x << shift = n * mult - k for k in [0 .. mult-1] and an integer n:
>
> ns2clc(clc2ns(x))
> = ns2clc(((x << shift) + mult - 1) / mult)
> = ((((x << shift) + mult - 1) / mult) * mult) >> shift
> = n * mult >> shift
> = ((x << shift) + k) >> shift
> = x + (k >> shift)
>
> So ns2clc(clc2ns(x)) = x for all x > 0 that have
>
> k = mult - ((x << shift) - 1) % mult - 1 < 1 << shift
>
> So my correction still stands.
Fair enough.
> > 1) We cannot add if we'd overflow
> >
> > 2) For mult <= 1 << shift it's always correct
> >
> > 3) for mult > 1 << shift we only apply it to the min value not the max
>
> Yeah, I didn't say your code is wrong *here*. I just think that my
> easier (and so probably faster) code is good enough.
Granted. I was stuck in the correctness discussion. So yeah, it does
not matter if we steal 30 usec of maximum idle sleep time from a 32kHz
clock. OTOH it does not matter much in the setup slow path to take
another conditional. :)
> Best regards and thanks for the nice discussion,
Ditto! You saved me from actually sitting down and using the pencil to
do the proper math.
Thanks,
tglx
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH] clockevents: Sanitize ticks to nsec conversion
@ 2013-09-20 21:30 ` Thomas Gleixner
0 siblings, 0 replies; 62+ messages in thread
From: Thomas Gleixner @ 2013-09-20 21:30 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Ludovic Desroches, Russell King - ARM Linux, Marc Kleine-Budde,
nicolas.ferre, LKML, Marc Pignat, john.stultz, kernel,
Ronald Wahl, LAK
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1803 bytes --]
On Fri, 20 Sep 2013, Uwe Kleine-König wrote:
> On Fri, Sep 20, 2013 at 11:56:27AM +0200, Thomas Gleixner wrote:
> > On Thu, 19 Sep 2013, Uwe Kleine-König wrote:
> > > > + * from nsec to device ticks will be correct.
> > > > + *
> > > > + * For mult > (1 << shift), i.e. device frequency is > 1GHz we
> > > > + * need to be careful. Adding mult - 1 will result in a value
> > > > + * which when converted back to device ticks will be larger
> > > s/will/can/
> >
> > No, it will always be larger.
> Hmm, consider a 1.25 GHz clock with shift = 2 and mult = 5. Then
> ns2clc(clc2ns(1000)) = 1000. So it's not always larger!
> In the fast-clock-case we have:
> With x << shift = n * mult - k for k in [0 .. mult-1] and an integer n:
>
> ns2clc(clc2ns(x))
> = ns2clc(((x << shift) + mult - 1) / mult)
> = ((((x << shift) + mult - 1) / mult) * mult) >> shift
> = n * mult >> shift
> = ((x << shift) + k) >> shift
> = x + (k >> shift)
>
> So ns2clc(clc2ns(x)) = x for all x > 0 that have
>
> k = mult - ((x << shift) - 1) % mult - 1 < 1 << shift
>
> So my correction still stands.
Fair enough.
> > 1) We cannot add if we'd overflow
> >
> > 2) For mult <= 1 << shift it's always correct
> >
> > 3) for mult > 1 << shift we only apply it to the min value not the max
>
> Yeah, I didn't say your code is wrong *here*. I just think that my
> easier (and so probably faster) code is good enough.
Granted. I was stuck in the correctness discussion. So yeah, it does
not matter if we steal 30 usec of maximum idle sleep time from a 32kHz
clock. OTOH it does not matter much in the setup slow path to take
another conditional. :)
> Best regards and thanks for the nice discussion,
Ditto! You saved me from actually sitting down and using the pencil to
do the proper math.
Thanks,
tglx
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v2] clockevents: Sanitize ticks to nsec conversion
2013-09-19 14:30 ` Thomas Gleixner
@ 2013-09-24 19:50 ` Uwe Kleine-König
-1 siblings, 0 replies; 62+ messages in thread
From: Uwe Kleine-König @ 2013-09-24 19:50 UTC (permalink / raw)
To: linux-arm-kernel
From: Thomas Gleixner <tglx@linutronix.de>
Marc Kleine-Budde pointed out, that commit 77cc982 "clocksource: use
clockevents_config_and_register() where possible" caused a regression
for some of the converted subarchs.
The reason is, that the clockevents core code converts the minimal
hardware tick delta to a nanosecond value for core internal
usage. This conversion is affected by integer math rounding loss, so
the backwards conversion to hardware ticks will likely result in a
value which is less than the configured hardware limitation. The
affected subarchs used their own workaround (SIGH!) which got lost in
the conversion.
The solution for the issue at hand is simple: adding evt->mult - 1 to
the shifted value before the integer divison in the core conversion
function takes care of it. But this only works for the case where for
the scaled math mult/shift pair "mult <= 1 << shift" is true. For the
case where "mult > 1 << shift" we can apply the rounding add only for
the minimum delta value to make sure that the backward conversion is
not less than the given hardware limit. For the upper bound we need to
omit the rounding add, because the backwards conversion is always
larger than the original latch value. That would violate the upper
bound of the hardware device.
Though looking closer@the details of that function reveals another
bogosity: The upper bounds check is broken as well. Checking for a
resulting "clc" value greater than KTIME_MAX after the conversion is
pointless. The conversion does:
u64 clc = (latch << evt->shift) / evt->mult;
So there is no sanity check for (latch << evt->shift) exceeding the
64bit boundary. The latch argument is "unsigned long", so on a 64bit
arch the handed in argument could easily lead to an unnoticed shift
overflow. With the above rounding fix applied the calculation before
the divison is:
u64 clc = (latch << evt->shift) + evt->mult - 1;
So we need to make sure, that neither the shift nor the rounding add
is overflowing the u64 boundary.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: nicolas.ferre at atmel.com
Cc: Marc Pignat <marc.pignat@hevs.ch>
Cc: john.stultz at linaro.org
Cc: kernel at pengutronix.de
Cc: Ronald Wahl <ronald.wahl@raritan.com>
Cc: LAK <linux-arm-kernel@lists.infradead.org>
Cc: Ludovic Desroches <ludovic.desroches@atmel.com>
[ukl: move assignment to rnd after eventually changing mult, fix build
issue and correct comment with the right math]
Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
kernel/time/clockevents.c | 65 ++++++++++++++++++++++++++++++++++++-----------
1 file changed, 50 insertions(+), 15 deletions(-)
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 38959c8..662c579 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -33,29 +33,64 @@ struct ce_unbind {
int res;
};
-/**
- * clockevents_delta2ns - Convert a latch value (device ticks) to nanoseconds
- * @latch: value to convert
- * @evt: pointer to clock event device descriptor
- *
- * Math helper, returns latch value converted to nanoseconds (bound checked)
- */
-u64 clockevent_delta2ns(unsigned long latch, struct clock_event_device *evt)
+static u64 cev_delta2ns(unsigned long latch, struct clock_event_device *evt,
+ bool ismax)
{
u64 clc = (u64) latch << evt->shift;
+ u64 rnd;
if (unlikely(!evt->mult)) {
evt->mult = 1;
WARN_ON(1);
}
+ rnd = (u64) evt->mult - 1;
+
+ /*
+ * Upper bound sanity check. If the backwards conversion is
+ * not equal latch, we know that the above shift overflowed.
+ */
+ if ((clc >> evt->shift) != (u64)latch)
+ clc = ~0ULL;
+
+ /*
+ * Scaled math oddities:
+ *
+ * For mult <= (1 << shift) we can safely add mult - 1 to
+ * prevent integer rounding loss. So the backwards conversion
+ * from nsec to device ticks will be correct.
+ *
+ * For mult > (1 << shift), i.e. device frequency is > 1GHz we
+ * need to be careful. Adding mult - 1 will result in a value
+ * which when converted back to device ticks can be larger
+ * than latch by up to (mult - 1) >> shift. For the min_delta
+ * calculation we still want to apply this in order to stay
+ * above the minimum device ticks limit. For the upper limit
+ * we would end up with a latch value larger than the upper
+ * limit of the device, so we omit the add to stay below the
+ * device upper boundary.
+ *
+ * Also omit the add if it would overflow the u64 boundary.
+ */
+ if ((~0ULL - clc > rnd) &&
+ (!ismax || evt->mult <= (1U << evt->shift)))
+ clc += rnd;
do_div(clc, evt->mult);
- if (clc < 1000)
- clc = 1000;
- if (clc > KTIME_MAX)
- clc = KTIME_MAX;
- return clc;
+ /* Deltas less than 1usec are pointless noise */
+ return clc > 1000 ? clc : 1000;
+}
+
+/**
+ * clockevents_delta2ns - Convert a latch value (device ticks) to nanoseconds
+ * @latch: value to convert
+ * @evt: pointer to clock event device descriptor
+ *
+ * Math helper, returns latch value converted to nanoseconds (bound checked)
+ */
+u64 clockevent_delta2ns(unsigned long latch, struct clock_event_device *evt)
+{
+ return cev_delta2ns(latch, evt, false);
}
EXPORT_SYMBOL_GPL(clockevent_delta2ns);
@@ -380,8 +415,8 @@ void clockevents_config(struct clock_event_device *dev, u32 freq)
sec = 600;
clockevents_calc_mult_shift(dev, freq, sec);
- dev->min_delta_ns = clockevent_delta2ns(dev->min_delta_ticks, dev);
- dev->max_delta_ns = clockevent_delta2ns(dev->max_delta_ticks, dev);
+ dev->min_delta_ns = cev_delta2ns(dev->min_delta_ticks, dev, false);
+ dev->max_delta_ns = cev_delta2ns(dev->max_delta_ticks, dev, true);
}
/**
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 62+ messages in thread* [PATCH v2] clockevents: Sanitize ticks to nsec conversion
@ 2013-09-24 19:50 ` Uwe Kleine-König
0 siblings, 0 replies; 62+ messages in thread
From: Uwe Kleine-König @ 2013-09-24 19:50 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel, Russell King - ARM Linux, Marc Kleine-Budde,
nicolas.ferre, Marc Pignat, john.stultz, kernel, Ronald Wahl, LAK,
Ludovic Desroches
From: Thomas Gleixner <tglx@linutronix.de>
Marc Kleine-Budde pointed out, that commit 77cc982 "clocksource: use
clockevents_config_and_register() where possible" caused a regression
for some of the converted subarchs.
The reason is, that the clockevents core code converts the minimal
hardware tick delta to a nanosecond value for core internal
usage. This conversion is affected by integer math rounding loss, so
the backwards conversion to hardware ticks will likely result in a
value which is less than the configured hardware limitation. The
affected subarchs used their own workaround (SIGH!) which got lost in
the conversion.
The solution for the issue at hand is simple: adding evt->mult - 1 to
the shifted value before the integer divison in the core conversion
function takes care of it. But this only works for the case where for
the scaled math mult/shift pair "mult <= 1 << shift" is true. For the
case where "mult > 1 << shift" we can apply the rounding add only for
the minimum delta value to make sure that the backward conversion is
not less than the given hardware limit. For the upper bound we need to
omit the rounding add, because the backwards conversion is always
larger than the original latch value. That would violate the upper
bound of the hardware device.
Though looking closer at the details of that function reveals another
bogosity: The upper bounds check is broken as well. Checking for a
resulting "clc" value greater than KTIME_MAX after the conversion is
pointless. The conversion does:
u64 clc = (latch << evt->shift) / evt->mult;
So there is no sanity check for (latch << evt->shift) exceeding the
64bit boundary. The latch argument is "unsigned long", so on a 64bit
arch the handed in argument could easily lead to an unnoticed shift
overflow. With the above rounding fix applied the calculation before
the divison is:
u64 clc = (latch << evt->shift) + evt->mult - 1;
So we need to make sure, that neither the shift nor the rounding add
is overflowing the u64 boundary.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: nicolas.ferre@atmel.com
Cc: Marc Pignat <marc.pignat@hevs.ch>
Cc: john.stultz@linaro.org
Cc: kernel@pengutronix.de
Cc: Ronald Wahl <ronald.wahl@raritan.com>
Cc: LAK <linux-arm-kernel@lists.infradead.org>
Cc: Ludovic Desroches <ludovic.desroches@atmel.com>
[ukl: move assignment to rnd after eventually changing mult, fix build
issue and correct comment with the right math]
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
kernel/time/clockevents.c | 65 ++++++++++++++++++++++++++++++++++++-----------
1 file changed, 50 insertions(+), 15 deletions(-)
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 38959c8..662c579 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -33,29 +33,64 @@ struct ce_unbind {
int res;
};
-/**
- * clockevents_delta2ns - Convert a latch value (device ticks) to nanoseconds
- * @latch: value to convert
- * @evt: pointer to clock event device descriptor
- *
- * Math helper, returns latch value converted to nanoseconds (bound checked)
- */
-u64 clockevent_delta2ns(unsigned long latch, struct clock_event_device *evt)
+static u64 cev_delta2ns(unsigned long latch, struct clock_event_device *evt,
+ bool ismax)
{
u64 clc = (u64) latch << evt->shift;
+ u64 rnd;
if (unlikely(!evt->mult)) {
evt->mult = 1;
WARN_ON(1);
}
+ rnd = (u64) evt->mult - 1;
+
+ /*
+ * Upper bound sanity check. If the backwards conversion is
+ * not equal latch, we know that the above shift overflowed.
+ */
+ if ((clc >> evt->shift) != (u64)latch)
+ clc = ~0ULL;
+
+ /*
+ * Scaled math oddities:
+ *
+ * For mult <= (1 << shift) we can safely add mult - 1 to
+ * prevent integer rounding loss. So the backwards conversion
+ * from nsec to device ticks will be correct.
+ *
+ * For mult > (1 << shift), i.e. device frequency is > 1GHz we
+ * need to be careful. Adding mult - 1 will result in a value
+ * which when converted back to device ticks can be larger
+ * than latch by up to (mult - 1) >> shift. For the min_delta
+ * calculation we still want to apply this in order to stay
+ * above the minimum device ticks limit. For the upper limit
+ * we would end up with a latch value larger than the upper
+ * limit of the device, so we omit the add to stay below the
+ * device upper boundary.
+ *
+ * Also omit the add if it would overflow the u64 boundary.
+ */
+ if ((~0ULL - clc > rnd) &&
+ (!ismax || evt->mult <= (1U << evt->shift)))
+ clc += rnd;
do_div(clc, evt->mult);
- if (clc < 1000)
- clc = 1000;
- if (clc > KTIME_MAX)
- clc = KTIME_MAX;
- return clc;
+ /* Deltas less than 1usec are pointless noise */
+ return clc > 1000 ? clc : 1000;
+}
+
+/**
+ * clockevents_delta2ns - Convert a latch value (device ticks) to nanoseconds
+ * @latch: value to convert
+ * @evt: pointer to clock event device descriptor
+ *
+ * Math helper, returns latch value converted to nanoseconds (bound checked)
+ */
+u64 clockevent_delta2ns(unsigned long latch, struct clock_event_device *evt)
+{
+ return cev_delta2ns(latch, evt, false);
}
EXPORT_SYMBOL_GPL(clockevent_delta2ns);
@@ -380,8 +415,8 @@ void clockevents_config(struct clock_event_device *dev, u32 freq)
sec = 600;
clockevents_calc_mult_shift(dev, freq, sec);
- dev->min_delta_ns = clockevent_delta2ns(dev->min_delta_ticks, dev);
- dev->max_delta_ns = clockevent_delta2ns(dev->max_delta_ticks, dev);
+ dev->min_delta_ns = cev_delta2ns(dev->min_delta_ticks, dev, false);
+ dev->max_delta_ns = cev_delta2ns(dev->max_delta_ticks, dev, true);
}
/**
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 62+ messages in thread* Timekeeping on at91rm9200 [Was: [PATCH v2] clockevents: Sanitize ticks to nsec conversion]
2013-09-24 19:50 ` Uwe Kleine-König
@ 2013-09-24 21:11 ` Uwe Kleine-König
-1 siblings, 0 replies; 62+ messages in thread
From: Uwe Kleine-König @ 2013-09-24 21:11 UTC (permalink / raw)
To: linux-arm-kernel
Hello Nicolas, hello Jean-Christophe,
I expect you should be able to revert
b7a8ca5 (ARM: at91: rm9200 fix time support)
which reverted
838a2ae (ARM: use clockevents_config_and_register() where possible)
for at91rm9200 on top of the patch this mail is a reply to.
Can you please test that?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 62+ messages in thread
* Timekeeping on at91rm9200 [Was: [PATCH v2] clockevents: Sanitize ticks to nsec conversion]
@ 2013-09-24 21:11 ` Uwe Kleine-König
0 siblings, 0 replies; 62+ messages in thread
From: Uwe Kleine-König @ 2013-09-24 21:11 UTC (permalink / raw)
To: Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD
Cc: Russell King - ARM Linux, Thomas Gleixner, linux-kernel,
Marc Pignat, Ludovic Desroches, john.stultz, kernel,
Marc Kleine-Budde, Ronald Wahl, LAK
Hello Nicolas, hello Jean-Christophe,
I expect you should be able to revert
b7a8ca5 (ARM: at91: rm9200 fix time support)
which reverted
838a2ae (ARM: use clockevents_config_and_register() where possible)
for at91rm9200 on top of the patch this mail is a reply to.
Can you please test that?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 62+ messages in thread
* Timekeeping on at91rm9200 [Was: [PATCH v2] clockevents: Sanitize ticks to nsec conversion]
2013-09-24 21:11 ` Uwe Kleine-König
@ 2013-10-04 10:00 ` Nicolas Ferre
-1 siblings, 0 replies; 62+ messages in thread
From: Nicolas Ferre @ 2013-10-04 10:00 UTC (permalink / raw)
To: linux-arm-kernel
On 24/09/2013 23:11, Uwe Kleine-K?nig :
> Hello Nicolas, hello Jean-Christophe,
>
> I expect you should be able to revert
>
> b7a8ca5 (ARM: at91: rm9200 fix time support)
>
> which reverted
>
> 838a2ae (ARM: use clockevents_config_and_register() where possible)
>
> for at91rm9200 on top of the patch this mail is a reply to.
>
> Can you please test that?
I have just tested that on at91rm9200-ek:
- revert the revert (!)
- experience the crash
- then apply the v2 patch
- and finally see it working nicely!
Thanks a lot for having corrected this. Now, do you want that I stack
this revert-of-revert on the AT91 git tree or you are able to apply it
through the clock git tree together with the patch that is fixing the
issue (it may prevent synchronization difficulties)? (BTW, I do not see
it yet in "next").
Anyway, if it is required, you can add my:
Tested-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Thanks, bye,
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: Timekeeping on at91rm9200 [Was: [PATCH v2] clockevents: Sanitize ticks to nsec conversion]
@ 2013-10-04 10:00 ` Nicolas Ferre
0 siblings, 0 replies; 62+ messages in thread
From: Nicolas Ferre @ 2013-10-04 10:00 UTC (permalink / raw)
To: Uwe Kleine-König, Jean-Christophe PLAGNIOL-VILLARD,
Thomas Gleixner, LAK
Cc: Russell King - ARM Linux, linux-kernel, Marc Pignat,
Ludovic Desroches, john.stultz, kernel, Marc Kleine-Budde,
Ronald Wahl
On 24/09/2013 23:11, Uwe Kleine-König :
> Hello Nicolas, hello Jean-Christophe,
>
> I expect you should be able to revert
>
> b7a8ca5 (ARM: at91: rm9200 fix time support)
>
> which reverted
>
> 838a2ae (ARM: use clockevents_config_and_register() where possible)
>
> for at91rm9200 on top of the patch this mail is a reply to.
>
> Can you please test that?
I have just tested that on at91rm9200-ek:
- revert the revert (!)
- experience the crash
- then apply the v2 patch
- and finally see it working nicely!
Thanks a lot for having corrected this. Now, do you want that I stack
this revert-of-revert on the AT91 git tree or you are able to apply it
through the clock git tree together with the patch that is fixing the
issue (it may prevent synchronization difficulties)? (BTW, I do not see
it yet in "next").
Anyway, if it is required, you can add my:
Tested-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Thanks, bye,
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v2] clockevents: Sanitize ticks to nsec conversion
2013-09-24 19:50 ` Uwe Kleine-König
@ 2013-09-24 21:16 ` Uwe Kleine-König
-1 siblings, 0 replies; 62+ messages in thread
From: Uwe Kleine-König @ 2013-09-24 21:16 UTC (permalink / raw)
To: linux-arm-kernel
Hello Thomas,
just found another little issue ...
On Tue, Sep 24, 2013 at 09:50:23PM +0200, Uwe Kleine-K?nig wrote:
> +/**
> + * clockevents_delta2ns - Convert a latch value (device ticks) to nanoseconds
> + * @latch: value to convert
> + * @evt: pointer to clock event device descriptor
> + *
> + * Math helper, returns latch value converted to nanoseconds (bound checked)
> + */
> +u64 clockevent_delta2ns(unsigned long latch, struct clock_event_device *evt)
... the kernel-doc comment has an 's' too much in the function name.
I'm not sending a v3 for just that. Can you fix that up when/if
applying?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v2] clockevents: Sanitize ticks to nsec conversion
@ 2013-09-24 21:16 ` Uwe Kleine-König
0 siblings, 0 replies; 62+ messages in thread
From: Uwe Kleine-König @ 2013-09-24 21:16 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Russell King - ARM Linux, nicolas.ferre, linux-kernel,
Marc Pignat, Ludovic Desroches, john.stultz, kernel,
Marc Kleine-Budde, Ronald Wahl, LAK
Hello Thomas,
just found another little issue ...
On Tue, Sep 24, 2013 at 09:50:23PM +0200, Uwe Kleine-König wrote:
> +/**
> + * clockevents_delta2ns - Convert a latch value (device ticks) to nanoseconds
> + * @latch: value to convert
> + * @evt: pointer to clock event device descriptor
> + *
> + * Math helper, returns latch value converted to nanoseconds (bound checked)
> + */
> +u64 clockevent_delta2ns(unsigned long latch, struct clock_event_device *evt)
... the kernel-doc comment has an 's' too much in the function name.
I'm not sending a v3 for just that. Can you fix that up when/if
applying?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v2] clockevents: Sanitize ticks to nsec conversion
2013-09-24 19:50 ` Uwe Kleine-König
@ 2013-10-08 10:08 ` Marc Kleine-Budde
-1 siblings, 0 replies; 62+ messages in thread
From: Marc Kleine-Budde @ 2013-10-08 10:08 UTC (permalink / raw)
To: linux-arm-kernel
On 09/24/2013 09:50 PM, Uwe Kleine-K?nig wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> Marc Kleine-Budde pointed out, that commit 77cc982 "clocksource: use
> clockevents_config_and_register() where possible" caused a regression
> for some of the converted subarchs.
>
> The reason is, that the clockevents core code converts the minimal
> hardware tick delta to a nanosecond value for core internal
> usage. This conversion is affected by integer math rounding loss, so
> the backwards conversion to hardware ticks will likely result in a
> value which is less than the configured hardware limitation. The
> affected subarchs used their own workaround (SIGH!) which got lost in
> the conversion.
>
> The solution for the issue at hand is simple: adding evt->mult - 1 to
> the shifted value before the integer divison in the core conversion
> function takes care of it. But this only works for the case where for
> the scaled math mult/shift pair "mult <= 1 << shift" is true. For the
> case where "mult > 1 << shift" we can apply the rounding add only for
> the minimum delta value to make sure that the backward conversion is
> not less than the given hardware limit. For the upper bound we need to
> omit the rounding add, because the backwards conversion is always
> larger than the original latch value. That would violate the upper
> bound of the hardware device.
>
> Though looking closer at the details of that function reveals another
> bogosity: The upper bounds check is broken as well. Checking for a
> resulting "clc" value greater than KTIME_MAX after the conversion is
> pointless. The conversion does:
>
> u64 clc = (latch << evt->shift) / evt->mult;
>
> So there is no sanity check for (latch << evt->shift) exceeding the
> 64bit boundary. The latch argument is "unsigned long", so on a 64bit
> arch the handed in argument could easily lead to an unnoticed shift
> overflow. With the above rounding fix applied the calculation before
> the divison is:
>
> u64 clc = (latch << evt->shift) + evt->mult - 1;
>
> So we need to make sure, that neither the shift nor the rounding add
> is overflowing the u64 boundary.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: nicolas.ferre at atmel.com
> Cc: Marc Pignat <marc.pignat@hevs.ch>
> Cc: john.stultz at linaro.org
> Cc: kernel at pengutronix.de
> Cc: Ronald Wahl <ronald.wahl@raritan.com>
> Cc: LAK <linux-arm-kernel@lists.infradead.org>
> Cc: Ludovic Desroches <ludovic.desroches@atmel.com>
> [ukl: move assignment to rnd after eventually changing mult, fix build
> issue and correct comment with the right math]
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
What's the status of this patch?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 259 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131008/e2dd9038/attachment.sig>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v2] clockevents: Sanitize ticks to nsec conversion
@ 2013-10-08 10:08 ` Marc Kleine-Budde
0 siblings, 0 replies; 62+ messages in thread
From: Marc Kleine-Budde @ 2013-10-08 10:08 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Thomas Gleixner, Russell King - ARM Linux, nicolas.ferre,
linux-kernel, Marc Pignat, Ludovic Desroches, john.stultz, kernel,
Ronald Wahl, LAK
[-- Attachment #1: Type: text/plain, Size: 3160 bytes --]
On 09/24/2013 09:50 PM, Uwe Kleine-König wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> Marc Kleine-Budde pointed out, that commit 77cc982 "clocksource: use
> clockevents_config_and_register() where possible" caused a regression
> for some of the converted subarchs.
>
> The reason is, that the clockevents core code converts the minimal
> hardware tick delta to a nanosecond value for core internal
> usage. This conversion is affected by integer math rounding loss, so
> the backwards conversion to hardware ticks will likely result in a
> value which is less than the configured hardware limitation. The
> affected subarchs used their own workaround (SIGH!) which got lost in
> the conversion.
>
> The solution for the issue at hand is simple: adding evt->mult - 1 to
> the shifted value before the integer divison in the core conversion
> function takes care of it. But this only works for the case where for
> the scaled math mult/shift pair "mult <= 1 << shift" is true. For the
> case where "mult > 1 << shift" we can apply the rounding add only for
> the minimum delta value to make sure that the backward conversion is
> not less than the given hardware limit. For the upper bound we need to
> omit the rounding add, because the backwards conversion is always
> larger than the original latch value. That would violate the upper
> bound of the hardware device.
>
> Though looking closer at the details of that function reveals another
> bogosity: The upper bounds check is broken as well. Checking for a
> resulting "clc" value greater than KTIME_MAX after the conversion is
> pointless. The conversion does:
>
> u64 clc = (latch << evt->shift) / evt->mult;
>
> So there is no sanity check for (latch << evt->shift) exceeding the
> 64bit boundary. The latch argument is "unsigned long", so on a 64bit
> arch the handed in argument could easily lead to an unnoticed shift
> overflow. With the above rounding fix applied the calculation before
> the divison is:
>
> u64 clc = (latch << evt->shift) + evt->mult - 1;
>
> So we need to make sure, that neither the shift nor the rounding add
> is overflowing the u64 boundary.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: nicolas.ferre@atmel.com
> Cc: Marc Pignat <marc.pignat@hevs.ch>
> Cc: john.stultz@linaro.org
> Cc: kernel@pengutronix.de
> Cc: Ronald Wahl <ronald.wahl@raritan.com>
> Cc: LAK <linux-arm-kernel@lists.infradead.org>
> Cc: Ludovic Desroches <ludovic.desroches@atmel.com>
> [ukl: move assignment to rnd after eventually changing mult, fix build
> issue and correct comment with the right math]
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
What's the status of this patch?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]
^ permalink raw reply [flat|nested] 62+ messages in thread
* [GIT PULL] fixes for integer rounding in timer core (Was: [PATCH v2] clockevents: Sanitize ticks to nsec conversion)
2013-10-08 10:08 ` Marc Kleine-Budde
@ 2013-10-08 15:31 ` Uwe Kleine-König
-1 siblings, 0 replies; 62+ messages in thread
From: Uwe Kleine-König @ 2013-10-08 15:31 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
> What's the status of this patch?
I pinged Thomas about it already via irc, but I didn't get a response up
to now. It would be great to get that into 3.12. To make it easy for
Thomas, here is a pull request that includes the fix and converts
at91rm9200 back to clockevents_config_and_register dropping one of the
workarounds for the fixed bug.
The following changes since commit 4a10c2ac2f368583138b774ca41fac4207911983:
Linux 3.12-rc2 (2013-09-23 15:41:09 -0700)
are available in the git repository at:
git://git.pengutronix.de/git/ukl/linux.git tags/timer-for-tip
for you to fetch changes up to b58c4b70121070f45eefa739ecbb58d57da1c4d9:
ARM: at91: rm9200: switch back to clockevents_config_and_register (2013-10-08 16:58:22 +0200)
----------------------------------------------------------------
The first patch in this pull request fixes an old integer rounding bug in the
timer core that was (and still is) worked around in a few arch's timer code.
The second patch drops such a work around now that the core is fixed.
----------------------------------------------------------------
Thomas Gleixner (1):
clockevents: Sanitize ticks to nsec conversion
Uwe Kleine-K?nig (1):
ARM: at91: rm9200: switch back to clockevents_config_and_register
arch/arm/mach-at91/at91rm9200_time.c | 7 ++--
kernel/time/clockevents.c | 65 +++++++++++++++++++++++++++---------
2 files changed, 52 insertions(+), 20 deletions(-)
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 62+ messages in thread* [GIT PULL] fixes for integer rounding in timer core (Was: [PATCH v2] clockevents: Sanitize ticks to nsec conversion)
@ 2013-10-08 15:31 ` Uwe Kleine-König
0 siblings, 0 replies; 62+ messages in thread
From: Uwe Kleine-König @ 2013-10-08 15:31 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Marc Kleine-Budde, Russell King - ARM Linux, Nicolas Ferre,
linux-kernel, Marc Pignat, Ludovic Desroches, John Stulz, kernel,
Ronald Wahl, linux-arm-kernel
Hello,
> What's the status of this patch?
I pinged Thomas about it already via irc, but I didn't get a response up
to now. It would be great to get that into 3.12. To make it easy for
Thomas, here is a pull request that includes the fix and converts
at91rm9200 back to clockevents_config_and_register dropping one of the
workarounds for the fixed bug.
The following changes since commit 4a10c2ac2f368583138b774ca41fac4207911983:
Linux 3.12-rc2 (2013-09-23 15:41:09 -0700)
are available in the git repository at:
git://git.pengutronix.de/git/ukl/linux.git tags/timer-for-tip
for you to fetch changes up to b58c4b70121070f45eefa739ecbb58d57da1c4d9:
ARM: at91: rm9200: switch back to clockevents_config_and_register (2013-10-08 16:58:22 +0200)
----------------------------------------------------------------
The first patch in this pull request fixes an old integer rounding bug in the
timer core that was (and still is) worked around in a few arch's timer code.
The second patch drops such a work around now that the core is fixed.
----------------------------------------------------------------
Thomas Gleixner (1):
clockevents: Sanitize ticks to nsec conversion
Uwe Kleine-König (1):
ARM: at91: rm9200: switch back to clockevents_config_and_register
arch/arm/mach-at91/at91rm9200_time.c | 7 ++--
kernel/time/clockevents.c | 65 +++++++++++++++++++++++++++---------
2 files changed, 52 insertions(+), 20 deletions(-)
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 62+ messages in thread* [GIT PULL] fixes for integer rounding in timer core
2013-10-08 15:31 ` Uwe Kleine-König
@ 2013-10-14 7:34 ` Uwe Kleine-König
-1 siblings, 0 replies; 62+ messages in thread
From: Uwe Kleine-König @ 2013-10-14 7:34 UTC (permalink / raw)
To: linux-arm-kernel
Hello Linus, hello Ingo,
On Tue, Oct 08, 2013 at 05:31:47PM +0200, Uwe Kleine-K?nig wrote:
> > What's the status of this patch?
> I pinged Thomas about it already via irc, but I didn't get a response up
> to now. It would be great to get that into 3.12. To make it easy for
> Thomas, here is a pull request that includes the fix and converts
> at91rm9200 back to clockevents_config_and_register dropping one of the
> workarounds for the fixed bug.
>
> The following changes since commit 4a10c2ac2f368583138b774ca41fac4207911983:
>
> Linux 3.12-rc2 (2013-09-23 15:41:09 -0700)
>
> are available in the git repository at:
>
> git://git.pengutronix.de/git/ukl/linux.git tags/timer-for-tip
>
> for you to fetch changes up to b58c4b70121070f45eefa739ecbb58d57da1c4d9:
>
> ARM: at91: rm9200: switch back to clockevents_config_and_register (2013-10-08 16:58:22 +0200)
>
> ----------------------------------------------------------------
> The first patch in this pull request fixes an old integer rounding bug in the
> timer core that was (and still is) worked around in a few arch's timer code.
> The second patch drops such a work around now that the core is fixed.
It would be nice to get that fix into 3.12, but Thomas stopped
responding nearly a month ago---his last mail in this thread[1] dates back
to September 20.
Can one of you please take it directly?
Thanks
Uwe
[1] http://thread.gmane.org/gmane.linux.kernel/1563370/
> ----------------------------------------------------------------
> Thomas Gleixner (1):
> clockevents: Sanitize ticks to nsec conversion
>
> Uwe Kleine-K?nig (1):
> ARM: at91: rm9200: switch back to clockevents_config_and_register
>
> arch/arm/mach-at91/at91rm9200_time.c | 7 ++--
> kernel/time/clockevents.c | 65 +++++++++++++++++++++++++++---------
> 2 files changed, 52 insertions(+), 20 deletions(-)
>
> --
> Pengutronix e.K. | Uwe Kleine-K?nig |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [GIT PULL] fixes for integer rounding in timer core
@ 2013-10-14 7:34 ` Uwe Kleine-König
0 siblings, 0 replies; 62+ messages in thread
From: Uwe Kleine-König @ 2013-10-14 7:34 UTC (permalink / raw)
To: Thomas Gleixner, Linus Torvalds, Ingo Molnar
Cc: Russell King - ARM Linux, Nicolas Ferre, linux-kernel,
Marc Pignat, Ludovic Desroches, Marc Kleine-Budde, kernel,
John Stulz, Ronald Wahl, linux-arm-kernel
Hello Linus, hello Ingo,
On Tue, Oct 08, 2013 at 05:31:47PM +0200, Uwe Kleine-König wrote:
> > What's the status of this patch?
> I pinged Thomas about it already via irc, but I didn't get a response up
> to now. It would be great to get that into 3.12. To make it easy for
> Thomas, here is a pull request that includes the fix and converts
> at91rm9200 back to clockevents_config_and_register dropping one of the
> workarounds for the fixed bug.
>
> The following changes since commit 4a10c2ac2f368583138b774ca41fac4207911983:
>
> Linux 3.12-rc2 (2013-09-23 15:41:09 -0700)
>
> are available in the git repository at:
>
> git://git.pengutronix.de/git/ukl/linux.git tags/timer-for-tip
>
> for you to fetch changes up to b58c4b70121070f45eefa739ecbb58d57da1c4d9:
>
> ARM: at91: rm9200: switch back to clockevents_config_and_register (2013-10-08 16:58:22 +0200)
>
> ----------------------------------------------------------------
> The first patch in this pull request fixes an old integer rounding bug in the
> timer core that was (and still is) worked around in a few arch's timer code.
> The second patch drops such a work around now that the core is fixed.
It would be nice to get that fix into 3.12, but Thomas stopped
responding nearly a month ago---his last mail in this thread[1] dates back
to September 20.
Can one of you please take it directly?
Thanks
Uwe
[1] http://thread.gmane.org/gmane.linux.kernel/1563370/
> ----------------------------------------------------------------
> Thomas Gleixner (1):
> clockevents: Sanitize ticks to nsec conversion
>
> Uwe Kleine-König (1):
> ARM: at91: rm9200: switch back to clockevents_config_and_register
>
> arch/arm/mach-at91/at91rm9200_time.c | 7 ++--
> kernel/time/clockevents.c | 65 +++++++++++++++++++++++++++---------
> 2 files changed, 52 insertions(+), 20 deletions(-)
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 62+ messages in thread
* [GIT PULL] fixes for integer rounding in timer core
2013-10-14 7:34 ` Uwe Kleine-König
@ 2013-10-16 14:19 ` Nicolas Ferre
-1 siblings, 0 replies; 62+ messages in thread
From: Nicolas Ferre @ 2013-10-16 14:19 UTC (permalink / raw)
To: linux-arm-kernel
On 14/10/2013 09:34, Uwe Kleine-K?nig :
> Hello Linus, hello Ingo,
>
> On Tue, Oct 08, 2013 at 05:31:47PM +0200, Uwe Kleine-K?nig wrote:
>>> What's the status of this patch?
>> I pinged Thomas about it already via irc, but I didn't get a response up
>> to now. It would be great to get that into 3.12. To make it easy for
>> Thomas, here is a pull request that includes the fix and converts
>> at91rm9200 back to clockevents_config_and_register dropping one of the
>> workarounds for the fixed bug.
>>
>> The following changes since commit 4a10c2ac2f368583138b774ca41fac4207911983:
>>
>> Linux 3.12-rc2 (2013-09-23 15:41:09 -0700)
>>
>> are available in the git repository at:
>>
>> git://git.pengutronix.de/git/ukl/linux.git tags/timer-for-tip
>>
>> for you to fetch changes up to b58c4b70121070f45eefa739ecbb58d57da1c4d9:
>>
>> ARM: at91: rm9200: switch back to clockevents_config_and_register (2013-10-08 16:58:22 +0200)
>>
>> ----------------------------------------------------------------
>> The first patch in this pull request fixes an old integer rounding bug in the
>> timer core that was (and still is) worked around in a few arch's timer code.
>> The second patch drops such a work around now that the core is fixed.
> It would be nice to get that fix into 3.12, but Thomas stopped
> responding nearly a month ago---his last mail in this thread[1] dates back
> to September 20.
>
> Can one of you please take it directly?
I second Uwe in his request (aka +1).
Thanks, bye,
> [1] http://thread.gmane.org/gmane.linux.kernel/1563370/
>> ----------------------------------------------------------------
>> Thomas Gleixner (1):
>> clockevents: Sanitize ticks to nsec conversion
>>
>> Uwe Kleine-K?nig (1):
>> ARM: at91: rm9200: switch back to clockevents_config_and_register
>>
>> arch/arm/mach-at91/at91rm9200_time.c | 7 ++--
>> kernel/time/clockevents.c | 65 +++++++++++++++++++++++++++---------
>> 2 files changed, 52 insertions(+), 20 deletions(-)
>>
>> --
>> Pengutronix e.K. | Uwe Kleine-K?nig |
>> Industrial Linux Solutions | http://www.pengutronix.de/ |
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [GIT PULL] fixes for integer rounding in timer core
@ 2013-10-16 14:19 ` Nicolas Ferre
0 siblings, 0 replies; 62+ messages in thread
From: Nicolas Ferre @ 2013-10-16 14:19 UTC (permalink / raw)
To: Uwe Kleine-König, Thomas Gleixner, Linus Torvalds,
Ingo Molnar
Cc: Russell King - ARM Linux, linux-kernel, Marc Pignat,
Ludovic Desroches, Marc Kleine-Budde, kernel, John Stulz,
Ronald Wahl, linux-arm-kernel
On 14/10/2013 09:34, Uwe Kleine-König :
> Hello Linus, hello Ingo,
>
> On Tue, Oct 08, 2013 at 05:31:47PM +0200, Uwe Kleine-König wrote:
>>> What's the status of this patch?
>> I pinged Thomas about it already via irc, but I didn't get a response up
>> to now. It would be great to get that into 3.12. To make it easy for
>> Thomas, here is a pull request that includes the fix and converts
>> at91rm9200 back to clockevents_config_and_register dropping one of the
>> workarounds for the fixed bug.
>>
>> The following changes since commit 4a10c2ac2f368583138b774ca41fac4207911983:
>>
>> Linux 3.12-rc2 (2013-09-23 15:41:09 -0700)
>>
>> are available in the git repository at:
>>
>> git://git.pengutronix.de/git/ukl/linux.git tags/timer-for-tip
>>
>> for you to fetch changes up to b58c4b70121070f45eefa739ecbb58d57da1c4d9:
>>
>> ARM: at91: rm9200: switch back to clockevents_config_and_register (2013-10-08 16:58:22 +0200)
>>
>> ----------------------------------------------------------------
>> The first patch in this pull request fixes an old integer rounding bug in the
>> timer core that was (and still is) worked around in a few arch's timer code.
>> The second patch drops such a work around now that the core is fixed.
> It would be nice to get that fix into 3.12, but Thomas stopped
> responding nearly a month ago---his last mail in this thread[1] dates back
> to September 20.
>
> Can one of you please take it directly?
I second Uwe in his request (aka +1).
Thanks, bye,
> [1] http://thread.gmane.org/gmane.linux.kernel/1563370/
>> ----------------------------------------------------------------
>> Thomas Gleixner (1):
>> clockevents: Sanitize ticks to nsec conversion
>>
>> Uwe Kleine-König (1):
>> ARM: at91: rm9200: switch back to clockevents_config_and_register
>>
>> arch/arm/mach-at91/at91rm9200_time.c | 7 ++--
>> kernel/time/clockevents.c | 65 +++++++++++++++++++++++++++---------
>> 2 files changed, 52 insertions(+), 20 deletions(-)
>>
>> --
>> Pengutronix e.K. | Uwe Kleine-König |
>> Industrial Linux Solutions | http://www.pengutronix.de/ |
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 62+ messages in thread
* [GIT PULL] fixes for integer rounding in timer core
2013-10-14 7:34 ` Uwe Kleine-König
@ 2013-10-21 7:12 ` Uwe Kleine-König
-1 siblings, 0 replies; 62+ messages in thread
From: Uwe Kleine-König @ 2013-10-21 7:12 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
[added Daniel Lezcano to To:]
On Mon, Oct 14, 2013 at 09:34:37AM +0200, Uwe Kleine-K?nig wrote:
> On Tue, Oct 08, 2013 at 05:31:47PM +0200, Uwe Kleine-K?nig wrote:
> > > What's the status of this patch?
> > I pinged Thomas about it already via irc, but I didn't get a response up
> > to now. It would be great to get that into 3.12. To make it easy for
> > Thomas, here is a pull request that includes the fix and converts
> > at91rm9200 back to clockevents_config_and_register dropping one of the
> > workarounds for the fixed bug.
> >
> > The following changes since commit 4a10c2ac2f368583138b774ca41fac4207911983:
> >
> > Linux 3.12-rc2 (2013-09-23 15:41:09 -0700)
> >
> > are available in the git repository at:
> >
> > git://git.pengutronix.de/git/ukl/linux.git tags/timer-for-tip
> >
> > for you to fetch changes up to b58c4b70121070f45eefa739ecbb58d57da1c4d9:
> >
> > ARM: at91: rm9200: switch back to clockevents_config_and_register (2013-10-08 16:58:22 +0200)
> >
> > ----------------------------------------------------------------
> > The first patch in this pull request fixes an old integer rounding bug in the
> > timer core that was (and still is) worked around in a few arch's timer code.
> > The second patch drops such a work around now that the core is fixed.
> It would be nice to get that fix into 3.12, but Thomas stopped
> responding nearly a month ago---his last mail in this thread[1] dates back
> to September 20.
Still no sign of life from Thomas.
> Can one of you please take it directly?
Daniel, would you care to take it?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [GIT PULL] fixes for integer rounding in timer core
@ 2013-10-21 7:12 ` Uwe Kleine-König
0 siblings, 0 replies; 62+ messages in thread
From: Uwe Kleine-König @ 2013-10-21 7:12 UTC (permalink / raw)
To: Thomas Gleixner, Linus Torvalds, Ingo Molnar, Daniel Lezcano
Cc: Russell King - ARM Linux, Nicolas Ferre, linux-kernel,
Marc Pignat, Ludovic Desroches, Marc Kleine-Budde, kernel,
John Stulz, Ronald Wahl, linux-arm-kernel
Hello,
[added Daniel Lezcano to To:]
On Mon, Oct 14, 2013 at 09:34:37AM +0200, Uwe Kleine-König wrote:
> On Tue, Oct 08, 2013 at 05:31:47PM +0200, Uwe Kleine-König wrote:
> > > What's the status of this patch?
> > I pinged Thomas about it already via irc, but I didn't get a response up
> > to now. It would be great to get that into 3.12. To make it easy for
> > Thomas, here is a pull request that includes the fix and converts
> > at91rm9200 back to clockevents_config_and_register dropping one of the
> > workarounds for the fixed bug.
> >
> > The following changes since commit 4a10c2ac2f368583138b774ca41fac4207911983:
> >
> > Linux 3.12-rc2 (2013-09-23 15:41:09 -0700)
> >
> > are available in the git repository at:
> >
> > git://git.pengutronix.de/git/ukl/linux.git tags/timer-for-tip
> >
> > for you to fetch changes up to b58c4b70121070f45eefa739ecbb58d57da1c4d9:
> >
> > ARM: at91: rm9200: switch back to clockevents_config_and_register (2013-10-08 16:58:22 +0200)
> >
> > ----------------------------------------------------------------
> > The first patch in this pull request fixes an old integer rounding bug in the
> > timer core that was (and still is) worked around in a few arch's timer code.
> > The second patch drops such a work around now that the core is fixed.
> It would be nice to get that fix into 3.12, but Thomas stopped
> responding nearly a month ago---his last mail in this thread[1] dates back
> to September 20.
Still no sign of life from Thomas.
> Can one of you please take it directly?
Daniel, would you care to take it?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 62+ messages in thread
* [GIT PULL] fixes for integer rounding in timer core
2013-10-21 7:12 ` Uwe Kleine-König
@ 2013-10-21 20:53 ` Daniel Lezcano
-1 siblings, 0 replies; 62+ messages in thread
From: Daniel Lezcano @ 2013-10-21 20:53 UTC (permalink / raw)
To: linux-arm-kernel
On 10/21/2013 09:12 AM, Uwe Kleine-K?nig wrote:
> Hello,
>
> [added Daniel Lezcano to To:]
>
> On Mon, Oct 14, 2013 at 09:34:37AM +0200, Uwe Kleine-K?nig wrote:
>> On Tue, Oct 08, 2013 at 05:31:47PM +0200, Uwe Kleine-K?nig wrote:
>>>> What's the status of this patch?
>>> I pinged Thomas about it already via irc, but I didn't get a response up
>>> to now. It would be great to get that into 3.12. To make it easy for
>>> Thomas, here is a pull request that includes the fix and converts
>>> at91rm9200 back to clockevents_config_and_register dropping one of the
>>> workarounds for the fixed bug.
[ ... ]
>>> ----------------------------------------------------------------
>>> The first patch in this pull request fixes an old integer rounding bug in the
>>> timer core that was (and still is) worked around in a few arch's timer code.
>>> The second patch drops such a work around now that the core is fixed.
>> It would be nice to get that fix into 3.12, but Thomas stopped
>> responding nearly a month ago---his last mail in this thread[1] dates back
>> to September 20.
> Still no sign of life from Thomas.
>
>> Can one of you please take it directly?
> Daniel, would you care to take it?
Yes, sure.
-- Daniel
--
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [GIT PULL] fixes for integer rounding in timer core
@ 2013-10-21 20:53 ` Daniel Lezcano
0 siblings, 0 replies; 62+ messages in thread
From: Daniel Lezcano @ 2013-10-21 20:53 UTC (permalink / raw)
To: Uwe Kleine-König, Thomas Gleixner, Linus Torvalds,
Ingo Molnar
Cc: Russell King - ARM Linux, Nicolas Ferre, linux-kernel,
Marc Pignat, Ludovic Desroches, Marc Kleine-Budde, kernel,
John Stulz, Ronald Wahl, linux-arm-kernel
On 10/21/2013 09:12 AM, Uwe Kleine-König wrote:
> Hello,
>
> [added Daniel Lezcano to To:]
>
> On Mon, Oct 14, 2013 at 09:34:37AM +0200, Uwe Kleine-König wrote:
>> On Tue, Oct 08, 2013 at 05:31:47PM +0200, Uwe Kleine-König wrote:
>>>> What's the status of this patch?
>>> I pinged Thomas about it already via irc, but I didn't get a response up
>>> to now. It would be great to get that into 3.12. To make it easy for
>>> Thomas, here is a pull request that includes the fix and converts
>>> at91rm9200 back to clockevents_config_and_register dropping one of the
>>> workarounds for the fixed bug.
[ ... ]
>>> ----------------------------------------------------------------
>>> The first patch in this pull request fixes an old integer rounding bug in the
>>> timer core that was (and still is) worked around in a few arch's timer code.
>>> The second patch drops such a work around now that the core is fixed.
>> It would be nice to get that fix into 3.12, but Thomas stopped
>> responding nearly a month ago---his last mail in this thread[1] dates back
>> to September 20.
> Still no sign of life from Thomas.
>
>> Can one of you please take it directly?
> Daniel, would you care to take it?
Yes, sure.
-- Daniel
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply [flat|nested] 62+ messages in thread
* [tip:timers/urgent] clockevents: Sanitize ticks to nsec conversion
2013-09-24 19:50 ` Uwe Kleine-König
` (3 preceding siblings ...)
(?)
@ 2013-10-23 10:56 ` tip-bot for Thomas Gleixner
-1 siblings, 0 replies; 62+ messages in thread
From: tip-bot for Thomas Gleixner @ 2013-10-23 10:56 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, linux-arm-kernel, mkl,
ludovic.desroches, linux, marc.pignat, u.kleine-koenig, tglx,
ronald.wahl
Commit-ID: 97b9410643475d6557d2517c2aff9fd2221141a9
Gitweb: http://git.kernel.org/tip/97b9410643475d6557d2517c2aff9fd2221141a9
Author: Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Tue, 24 Sep 2013 21:50:23 +0200
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 23 Oct 2013 12:51:21 +0200
clockevents: Sanitize ticks to nsec conversion
Marc Kleine-Budde pointed out, that commit 77cc982 "clocksource: use
clockevents_config_and_register() where possible" caused a regression
for some of the converted subarchs.
The reason is, that the clockevents core code converts the minimal
hardware tick delta to a nanosecond value for core internal
usage. This conversion is affected by integer math rounding loss, so
the backwards conversion to hardware ticks will likely result in a
value which is less than the configured hardware limitation. The
affected subarchs used their own workaround (SIGH!) which got lost in
the conversion.
The solution for the issue at hand is simple: adding evt->mult - 1 to
the shifted value before the integer divison in the core conversion
function takes care of it. But this only works for the case where for
the scaled math mult/shift pair "mult <= 1 << shift" is true. For the
case where "mult > 1 << shift" we can apply the rounding add only for
the minimum delta value to make sure that the backward conversion is
not less than the given hardware limit. For the upper bound we need to
omit the rounding add, because the backwards conversion is always
larger than the original latch value. That would violate the upper
bound of the hardware device.
Though looking closer at the details of that function reveals another
bogosity: The upper bounds check is broken as well. Checking for a
resulting "clc" value greater than KTIME_MAX after the conversion is
pointless. The conversion does:
u64 clc = (latch << evt->shift) / evt->mult;
So there is no sanity check for (latch << evt->shift) exceeding the
64bit boundary. The latch argument is "unsigned long", so on a 64bit
arch the handed in argument could easily lead to an unnoticed shift
overflow. With the above rounding fix applied the calculation before
the divison is:
u64 clc = (latch << evt->shift) + evt->mult - 1;
So we need to make sure, that neither the shift nor the rounding add
is overflowing the u64 boundary.
[ukl: move assignment to rnd after eventually changing mult, fix build
issue and correct comment with the right math]
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: nicolas.ferre@atmel.com
Cc: Marc Pignat <marc.pignat@hevs.ch>
Cc: john.stultz@linaro.org
Cc: kernel@pengutronix.de
Cc: Ronald Wahl <ronald.wahl@raritan.com>
Cc: LAK <linux-arm-kernel@lists.infradead.org>
Cc: Ludovic Desroches <ludovic.desroches@atmel.com>
Cc: stable@vger.kernel.org
Link: http://lkml.kernel.org/r/1380052223-24139-1-git-send-email-u.kleine-koenig@pengutronix.de
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
kernel/time/clockevents.c | 65 ++++++++++++++++++++++++++++++++++++-----------
1 file changed, 50 insertions(+), 15 deletions(-)
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 38959c8..662c579 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -33,29 +33,64 @@ struct ce_unbind {
int res;
};
-/**
- * clockevents_delta2ns - Convert a latch value (device ticks) to nanoseconds
- * @latch: value to convert
- * @evt: pointer to clock event device descriptor
- *
- * Math helper, returns latch value converted to nanoseconds (bound checked)
- */
-u64 clockevent_delta2ns(unsigned long latch, struct clock_event_device *evt)
+static u64 cev_delta2ns(unsigned long latch, struct clock_event_device *evt,
+ bool ismax)
{
u64 clc = (u64) latch << evt->shift;
+ u64 rnd;
if (unlikely(!evt->mult)) {
evt->mult = 1;
WARN_ON(1);
}
+ rnd = (u64) evt->mult - 1;
+
+ /*
+ * Upper bound sanity check. If the backwards conversion is
+ * not equal latch, we know that the above shift overflowed.
+ */
+ if ((clc >> evt->shift) != (u64)latch)
+ clc = ~0ULL;
+
+ /*
+ * Scaled math oddities:
+ *
+ * For mult <= (1 << shift) we can safely add mult - 1 to
+ * prevent integer rounding loss. So the backwards conversion
+ * from nsec to device ticks will be correct.
+ *
+ * For mult > (1 << shift), i.e. device frequency is > 1GHz we
+ * need to be careful. Adding mult - 1 will result in a value
+ * which when converted back to device ticks can be larger
+ * than latch by up to (mult - 1) >> shift. For the min_delta
+ * calculation we still want to apply this in order to stay
+ * above the minimum device ticks limit. For the upper limit
+ * we would end up with a latch value larger than the upper
+ * limit of the device, so we omit the add to stay below the
+ * device upper boundary.
+ *
+ * Also omit the add if it would overflow the u64 boundary.
+ */
+ if ((~0ULL - clc > rnd) &&
+ (!ismax || evt->mult <= (1U << evt->shift)))
+ clc += rnd;
do_div(clc, evt->mult);
- if (clc < 1000)
- clc = 1000;
- if (clc > KTIME_MAX)
- clc = KTIME_MAX;
- return clc;
+ /* Deltas less than 1usec are pointless noise */
+ return clc > 1000 ? clc : 1000;
+}
+
+/**
+ * clockevents_delta2ns - Convert a latch value (device ticks) to nanoseconds
+ * @latch: value to convert
+ * @evt: pointer to clock event device descriptor
+ *
+ * Math helper, returns latch value converted to nanoseconds (bound checked)
+ */
+u64 clockevent_delta2ns(unsigned long latch, struct clock_event_device *evt)
+{
+ return cev_delta2ns(latch, evt, false);
}
EXPORT_SYMBOL_GPL(clockevent_delta2ns);
@@ -380,8 +415,8 @@ void clockevents_config(struct clock_event_device *dev, u32 freq)
sec = 600;
clockevents_calc_mult_shift(dev, freq, sec);
- dev->min_delta_ns = clockevent_delta2ns(dev->min_delta_ticks, dev);
- dev->max_delta_ns = clockevent_delta2ns(dev->max_delta_ticks, dev);
+ dev->min_delta_ns = cev_delta2ns(dev->min_delta_ticks, dev, false);
+ dev->max_delta_ns = cev_delta2ns(dev->max_delta_ticks, dev, true);
}
/**
^ permalink raw reply related [flat|nested] 62+ messages in thread