[PATCH v3 0/2] Fix RPM calculation in pwm-fan

[PATCH v3 0/2] Fix RPM calculation in pwm-fan

[Paul Barker]

Fix inaccuracy and inefficiency when calculating fan RPM in the pwm-fan
driver.

These changes can also be pulled from:

  https://gitlab.com/pbarker.dev/staging/linux.git
  tag: for-hwmon/pwm-fan-fix-rpm-v3_2020-11-10

Changes from v2:

  * Handle the case where delta=0 to avoid potential divide-by-zero
    exceptions.

  * Store a device pointer in pwm_fan_ctx so we can print an error
    message.

Paul Barker (2):
  hwmon: pwm-fan: Store device pointer in pwm_fan_ctx
  hwmon: pwm-fan: Fix RPM calculation

 drivers/hwmon/pwm-fan.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)


base-commit: f8394f232b1eab649ce2df5c5f15b0e528c92091
-- 
2.29.2

[PATCH v3 1/2] hwmon: pwm-fan: Store device pointer in pwm_fan_ctx

[Paul Barker]

This allows us to use dev_err() and friends from functions which only
have a pwm_fan_ctx pointer.

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

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index bdba2143021a..edc0453be25a 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -26,6 +26,7 @@ struct pwm_fan_ctx {
 	struct mutex lock;
 	struct pwm_device *pwm;
 	struct regulator *reg_en;
+	struct device *dev;
 
 	int irq;
 	atomic_t pulses;
@@ -290,6 +291,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
 	if (!ctx)
 		return -ENOMEM;
 
+	ctx->dev = dev;
 	mutex_init(&ctx->lock);
 
 	ctx->pwm = devm_of_pwm_get(dev, dev->of_node, NULL);
-- 
2.29.2

[PATCH v3 2/2] 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. If the width of the measurement interval is zero we
bail out instead of dividing 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.

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

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index edc0453be25a..24cfed4d625e 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -55,14 +55,19 @@ 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;
+	if (delta) {
+		pulses = atomic_read(&ctx->pulses);
+		atomic_sub(pulses, &ctx->pulses);
+		ctx->rpm = (unsigned int)(pulses * 1000 * 60) /
+			(ctx->pulses_per_revolution * delta);
+	} else {
+		dev_err(ctx->dev,
+			"Cannot determine fan RPM as time delta is zero\n");
+		ctx->rpm = 0;
+	}
 
 	ctx->sample_start = ktime_get();
 	mod_timer(&ctx->rpm_timer, jiffies + HZ);
-- 
2.29.2

Re: [PATCH v3 2/2] hwmon: pwm-fan: Fix RPM calculation

[Guenter Roeck]

On Tue, Nov 10, 2020 at 11:28:17AM +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. If the width of the measurement interval is zero we
> bail out instead of dividing 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.
> 
> Signed-off-by: Paul Barker <pbarker@konsulko.com>
> ---
>  drivers/hwmon/pwm-fan.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index edc0453be25a..24cfed4d625e 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -55,14 +55,19 @@ 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;
> +	if (delta) {
> +		pulses = atomic_read(&ctx->pulses);
> +		atomic_sub(pulses, &ctx->pulses);
> +		ctx->rpm = (unsigned int)(pulses * 1000 * 60) /
> +			(ctx->pulses_per_revolution * delta);
> +	} else {
> +		dev_err(ctx->dev,
> +			"Cannot determine fan RPM as time delta is zero\n");
> +		ctx->rpm = 0;

I don't think that warrants an error message. At best it should be a debug
message, but even that seems not worth it. I would suggest to not update
rpm if that happens. After all, it is pretty much a theoretic case.

Guenter


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

Re: [PATCH v3 2/2] hwmon: pwm-fan: Fix RPM calculation

[Paul Barker]

On Tue, 10 Nov 2020 at 16:08, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Tue, Nov 10, 2020 at 11:28:17AM +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. If the width of the measurement interval is zero we
> > bail out instead of dividing 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.
> >
> > Signed-off-by: Paul Barker <pbarker@konsulko.com>
> > ---
> >  drivers/hwmon/pwm-fan.c | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> > index edc0453be25a..24cfed4d625e 100644
> > --- a/drivers/hwmon/pwm-fan.c
> > +++ b/drivers/hwmon/pwm-fan.c
> > @@ -55,14 +55,19 @@ 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;
> > +     if (delta) {
> > +             pulses = atomic_read(&ctx->pulses);
> > +             atomic_sub(pulses, &ctx->pulses);
> > +             ctx->rpm = (unsigned int)(pulses * 1000 * 60) /
> > +                     (ctx->pulses_per_revolution * delta);
> > +     } else {
> > +             dev_err(ctx->dev,
> > +                     "Cannot determine fan RPM as time delta is zero\n");
> > +             ctx->rpm = 0;
>
> I don't think that warrants an error message. At best it should be a debug
> message, but even that seems not worth it. I would suggest to not update
> rpm if that happens. After all, it is pretty much a theoretic case.

My thought process was that setting an RPM value of zero would be
confusing - it could be caused due to fan failure or due to this
(theoretical) error. I'm happy to drop the error message though - is
the patch acceptable other than that?

-- 
Paul Barker
Konsulko Group

Re: [PATCH v3 2/2] hwmon: pwm-fan: Fix RPM calculation

[Guenter Roeck]

On 11/10/20 8:20 AM, Paul Barker wrote:
> On Tue, 10 Nov 2020 at 16:08, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Tue, Nov 10, 2020 at 11:28:17AM +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. If the width of the measurement interval is zero we
>>> bail out instead of dividing 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.
>>>
>>> Signed-off-by: Paul Barker <pbarker@konsulko.com>
>>> ---
>>>  drivers/hwmon/pwm-fan.c | 17 +++++++++++------
>>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
>>> index edc0453be25a..24cfed4d625e 100644
>>> --- a/drivers/hwmon/pwm-fan.c
>>> +++ b/drivers/hwmon/pwm-fan.c
>>> @@ -55,14 +55,19 @@ 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;
>>> +     if (delta) {
>>> +             pulses = atomic_read(&ctx->pulses);
>>> +             atomic_sub(pulses, &ctx->pulses);
>>> +             ctx->rpm = (unsigned int)(pulses * 1000 * 60) /
>>> +                     (ctx->pulses_per_revolution * delta);
>>> +     } else {
>>> +             dev_err(ctx->dev,
>>> +                     "Cannot determine fan RPM as time delta is zero\n");
>>> +             ctx->rpm = 0;
>>
>> I don't think that warrants an error message. At best it should be a debug
>> message, but even that seems not worth it. I would suggest to not update
>> rpm if that happens. After all, it is pretty much a theoretic case.
> 
> My thought process was that setting an RPM value of zero would be
> confusing - it could be caused due to fan failure or due to this
> (theoretical) error. I'm happy to drop the error message though - is

Yes, that is why I suggested to keep the old speed in that situation.
After all, it _will_ be updated shortly afterwards. Either case, people
won't typically look into the kernel log if they see the 0 rpm.

Guenter

> the patch acceptable other than that?
> 

Re: [PATCH v3 2/2] hwmon: pwm-fan: Fix RPM calculation

[Paul Barker]

On Tue, 10 Nov 2020 at 17:32, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 11/10/20 8:20 AM, Paul Barker wrote:
> > On Tue, 10 Nov 2020 at 16:08, Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On Tue, Nov 10, 2020 at 11:28:17AM +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. If the width of the measurement interval is zero we
> >>> bail out instead of dividing 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.
> >>>
> >>> Signed-off-by: Paul Barker <pbarker@konsulko.com>
> >>> ---
> >>>  drivers/hwmon/pwm-fan.c | 17 +++++++++++------
> >>>  1 file changed, 11 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> >>> index edc0453be25a..24cfed4d625e 100644
> >>> --- a/drivers/hwmon/pwm-fan.c
> >>> +++ b/drivers/hwmon/pwm-fan.c
> >>> @@ -55,14 +55,19 @@ 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;
> >>> +     if (delta) {
> >>> +             pulses = atomic_read(&ctx->pulses);
> >>> +             atomic_sub(pulses, &ctx->pulses);
> >>> +             ctx->rpm = (unsigned int)(pulses * 1000 * 60) /
> >>> +                     (ctx->pulses_per_revolution * delta);
> >>> +     } else {
> >>> +             dev_err(ctx->dev,
> >>> +                     "Cannot determine fan RPM as time delta is zero\n");
> >>> +             ctx->rpm = 0;
> >>
> >> I don't think that warrants an error message. At best it should be a debug
> >> message, but even that seems not worth it. I would suggest to not update
> >> rpm if that happens. After all, it is pretty much a theoretic case.
> >
> > My thought process was that setting an RPM value of zero would be
> > confusing - it could be caused due to fan failure or due to this
> > (theoretical) error. I'm happy to drop the error message though - is
>
> Yes, that is why I suggested to keep the old speed in that situation.
> After all, it _will_ be updated shortly afterwards. Either case, people
> won't typically look into the kernel log if they see the 0 rpm.

Ah, I misunderstood your earlier reply then. Yes - this makes sense,
clearly I haven't had enough coffee as that solution should have been
obvious!

v4 incoming.

-- 
Paul Barker
Konsulko Group