DIV_ROUND_CLOSEST returns a bad result for negative dividends:
DIV_ROUND_CLOSEST(-2, 2) = 0
Most of the time this does not matter. However, in the hardware monitoring
subsystem, it is often used on integers which can be negative (such as
temperatures). Introduce new macro IDIV_ROUND_CLOSEST which also supports
negative dividends.
Signed-off-by: Guenter Roeck <[email protected]>
---
I can take this patch through my hwmon tree, but would like to get an Ack first.
Alternative would be to put it into include/linux/hwmon.h, but I would prefer
to avoid that.
Also, if someone has an idea for a simpler implementation, I would really like
to know about it.
include/linux/kernel.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 6043821..a89483c 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -89,6 +89,15 @@
} \
)
+#define IDIV_ROUND_CLOSEST(x, divisor)( \
+{ \
+ typeof(x) __x = x; \
+ typeof(divisor) __d1 = divisor; \
+ typeof(divisor) __d2 = (__x) < 0 ? -(__d1) : (__d1);\
+ (((__x) + ((__d2) / 2)) / (__d1)); \
+} \
+)
+
/*
* Multiplies an integer by a fraction, while avoiding unnecessary
* overflow or loss of precision.
--
1.7.9.7
Hi Guenter,
On Fri, 24 Aug 2012 17:53:17 -0700, Guenter Roeck wrote:
> DIV_ROUND_CLOSEST returns a bad result for negative dividends:
> DIV_ROUND_CLOSEST(-2, 2) = 0
>
> Most of the time this does not matter. However, in the hardware monitoring
> subsystem, it is often used on integers which can be negative (such as
> temperatures). Introduce new macro IDIV_ROUND_CLOSEST which also supports
> negative dividends.
Good catch. It's been broken for years and I never noticed :(
Acked-by: Jean Delvare <[email protected]>
>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> I can take this patch through my hwmon tree, but would like to get an Ack first.
> Alternative would be to put it into include/linux/hwmon.h, but I would prefer
> to avoid that.
I agree it should sit next to DIV_ROUND_CLOSEST in <linux/kernel.h>.
>
> Also, if someone has an idea for a simpler implementation, I would really like
> to know about it.
I can't think of anything better.
Note that your implementation only supports negative dividend not
divisor. While it won't be a problem for hwmon drivers, and most
probably not a problem for other drivers either, it might be worth
putting in a comment so as to prevent false expectations from the
reader.
>
> include/linux/kernel.h | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 6043821..a89483c 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -89,6 +89,15 @@
> } \
> )
>
> +#define IDIV_ROUND_CLOSEST(x, divisor)( \
> +{ \
> + typeof(x) __x = x; \
> + typeof(divisor) __d1 = divisor; \
> + typeof(divisor) __d2 = (__x) < 0 ? -(__d1) : (__d1);\
> + (((__x) + ((__d2) / 2)) / (__d1)); \
> +} \
> +)
> +
> /*
> * Multiplies an integer by a fraction, while avoiding unnecessary
> * overflow or loss of precision.
--
Jean Delvare
On Tue, Aug 28, 2012 at 06:09:26PM +0200, Jean Delvare wrote:
> Hi Guenter,
>
> On Fri, 24 Aug 2012 17:53:17 -0700, Guenter Roeck wrote:
> > DIV_ROUND_CLOSEST returns a bad result for negative dividends:
> > DIV_ROUND_CLOSEST(-2, 2) = 0
> >
> > Most of the time this does not matter. However, in the hardware monitoring
> > subsystem, it is often used on integers which can be negative (such as
> > temperatures). Introduce new macro IDIV_ROUND_CLOSEST which also supports
> > negative dividends.
>
> Good catch. It's been broken for years and I never noticed :(
>
> Acked-by: Jean Delvare <[email protected]>
>
> >
> > Signed-off-by: Guenter Roeck <[email protected]>
> > ---
> > I can take this patch through my hwmon tree, but would like to get an Ack first.
> > Alternative would be to put it into include/linux/hwmon.h, but I would prefer
> > to avoid that.
>
> I agree it should sit next to DIV_ROUND_CLOSEST in <linux/kernel.h>.
>
> >
> > Also, if someone has an idea for a simpler implementation, I would really like
> > to know about it.
>
> I can't think of anything better.
>
> Note that your implementation only supports negative dividend not
> divisor. While it won't be a problem for hwmon drivers, and most
> probably not a problem for other drivers either, it might be worth
> putting in a comment so as to prevent false expectations from the
> reader.
>
I noticed. I am not concerned about negative divisors, though, but unsigned
divisor variables are a problem too. I have a new version which also supports
divisors which are declared as unsigned.
Thanks,
Guenter
> >
> > include/linux/kernel.h | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > index 6043821..a89483c 100644
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -89,6 +89,15 @@
> > } \
> > )
> >
> > +#define IDIV_ROUND_CLOSEST(x, divisor)( \
> > +{ \
> > + typeof(x) __x = x; \
> > + typeof(divisor) __d1 = divisor; \
> > + typeof(divisor) __d2 = (__x) < 0 ? -(__d1) : (__d1);\
> > + (((__x) + ((__d2) / 2)) / (__d1)); \
> > +} \
> > +)
> > +
> > /*
> > * Multiplies an integer by a fraction, while avoiding unnecessary
> > * overflow or loss of precision.
>
>
> --
> Jean Delvare
>