On 11/10/20 8:20 AM, Paul Barker wrote:
> On Tue, 10 Nov 2020 at 16:08, Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>>> ---
>>> 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?
>
