| Message ID | 20201110112817.52549-3-pbarker@konsulko.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix RPM calculation in pwm-fan | expand |
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 >
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?
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? >
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.
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);
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(-)