[PATCH v2] hwmon: pwm-fan: Fix RPM calculation

[PATCH v2] hwmon: pwm-fan: Fix RPM calculation

[Paul Barker]

To convert the number of pulses counted into an RPM estimation, we need
to divide by the width of our measurement interval instead of
multiplying by it.

We also don't need to do 64-bit division, with 32-bits we can handle a
fan running at over 4 million RPM.

As the sample_timer() function is called once per second, delta will usually
be just over 1000 and should never be zero, avoiding the risk of a divide by
zero exception.

Signed-off-by: Paul Barker <pbarker@konsulko.com>
---
 drivers/hwmon/pwm-fan.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index bdba2143021a..2f2950629892 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -54,14 +54,13 @@ static irqreturn_t pulse_handler(int irq, void *dev_id)
 static void sample_timer(struct timer_list *t)
 {
 	struct pwm_fan_ctx *ctx = from_timer(ctx, t, rpm_timer);
+	unsigned int delta = ktime_ms_delta(ktime_get(), ctx->sample_start);
 	int pulses;
-	u64 tmp;
 
 	pulses = atomic_read(&ctx->pulses);
 	atomic_sub(pulses, &ctx->pulses);
-	tmp = (u64)pulses * ktime_ms_delta(ktime_get(), ctx->sample_start) * 60;
-	do_div(tmp, ctx->pulses_per_revolution * 1000);
-	ctx->rpm = tmp;
+	ctx->rpm = (unsigned int)(pulses * 1000 * 60) /
+		(ctx->pulses_per_revolution * delta);
 
 	ctx->sample_start = ktime_get();
 	mod_timer(&ctx->rpm_timer, jiffies + HZ);
-- 
2.29.2

Re: [PATCH v2] hwmon: pwm-fan: Fix RPM calculation

[Guenter Roeck]

On Thu, Nov 05, 2020 at 04:41:46PM +0000, Paul Barker wrote:
> To convert the number of pulses counted into an RPM estimation, we need
> to divide by the width of our measurement interval instead of
> multiplying by it.
> 
> We also don't need to do 64-bit division, with 32-bits we can handle a
> fan running at over 4 million RPM.
> 
> As the sample_timer() function is called once per second, delta will usually
> be just over 1000 and should never be zero, avoiding the risk of a divide by
> zero exception.
> 
> Signed-off-by: Paul Barker <pbarker@konsulko.com>
> ---
>  drivers/hwmon/pwm-fan.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index bdba2143021a..2f2950629892 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -54,14 +54,13 @@ static irqreturn_t pulse_handler(int irq, void *dev_id)
>  static void sample_timer(struct timer_list *t)
>  {
>  	struct pwm_fan_ctx *ctx = from_timer(ctx, t, rpm_timer);
> +	unsigned int delta = ktime_ms_delta(ktime_get(), ctx->sample_start);
>  	int pulses;
> -	u64 tmp;
>  
>  	pulses = atomic_read(&ctx->pulses);
>  	atomic_sub(pulses, &ctx->pulses);
> -	tmp = (u64)pulses * ktime_ms_delta(ktime_get(), ctx->sample_start) * 60;
> -	do_div(tmp, ctx->pulses_per_revolution * 1000);
> -	ctx->rpm = tmp;
> +	ctx->rpm = (unsigned int)(pulses * 1000 * 60) /
> +		(ctx->pulses_per_revolution * delta);

delta can, at least in theory, be 0. While that is quite unlikely to happen,
the code should handle this situation.

>  
>  	ctx->sample_start = ktime_get();
>  	mod_timer(&ctx->rpm_timer, jiffies + HZ);

Re: [PATCH v2] hwmon: pwm-fan: Fix RPM calculation

[Paul Barker]

On Sun, 8 Nov 2020 at 03:47, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Thu, Nov 05, 2020 at 04:41:46PM +0000, Paul Barker wrote:
> > To convert the number of pulses counted into an RPM estimation, we need
> > to divide by the width of our measurement interval instead of
> > multiplying by it.
> >
> > We also don't need to do 64-bit division, with 32-bits we can handle a
> > fan running at over 4 million RPM.
> >
> > As the sample_timer() function is called once per second, delta will usually
> > be just over 1000 and should never be zero, avoiding the risk of a divide by
> > zero exception.
> >
> > Signed-off-by: Paul Barker <pbarker@konsulko.com>
> > ---
> >  drivers/hwmon/pwm-fan.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> > index bdba2143021a..2f2950629892 100644
> > --- a/drivers/hwmon/pwm-fan.c
> > +++ b/drivers/hwmon/pwm-fan.c
> > @@ -54,14 +54,13 @@ static irqreturn_t pulse_handler(int irq, void *dev_id)
> >  static void sample_timer(struct timer_list *t)
> >  {
> >       struct pwm_fan_ctx *ctx = from_timer(ctx, t, rpm_timer);
> > +     unsigned int delta = ktime_ms_delta(ktime_get(), ctx->sample_start);
> >       int pulses;
> > -     u64 tmp;
> >
> >       pulses = atomic_read(&ctx->pulses);
> >       atomic_sub(pulses, &ctx->pulses);
> > -     tmp = (u64)pulses * ktime_ms_delta(ktime_get(), ctx->sample_start) * 60;
> > -     do_div(tmp, ctx->pulses_per_revolution * 1000);
> > -     ctx->rpm = tmp;
> > +     ctx->rpm = (unsigned int)(pulses * 1000 * 60) /
> > +             (ctx->pulses_per_revolution * delta);
>
> delta can, at least in theory, be 0. While that is quite unlikely to happen,
> the code should handle this situation.

I'll send a v3 which bails out if delta=0.

Thanks,

-- 
Paul Barker
Konsulko Group