diff mbox series

[1/2] hwmon: pwm-fan: Ensure that calculation doesn't discard big period values

Message ID 20201215092031.152243-1-u.kleine-koenig@pengutronix.de
State Not Applicable
Headers show
Series [1/2] hwmon: pwm-fan: Ensure that calculation doesn't discard big period values | expand

Commit Message

Uwe Kleine-König Dec. 15, 2020, 9:20 a.m. UTC 
With MAX_PWM being defined to 255 the code

	unsigned long period;
	...
	period = ctx->pwm->args.period;
	state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);

calculates a too small value for duty_cycle if the configured period is
big (either by discarding the 64 bit value ctx->pwm->args.period or by
overflowing the multiplication). As this results in a too slow fan and
so maybe an overheating machine better be safe than sorry and error out
in .probe.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/hwmon/pwm-fan.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)


base-commit: 2c85ebc57b3e1817b6ce1a6b703928e113a90442

Comments

Paul Barker Dec. 15, 2020, 11:29 a.m. UTC | #1 
On Tue, 15 Dec 2020 at 09:23, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> With MAX_PWM being defined to 255 the code
>
>         unsigned long period;
>         ...
>         period = ctx->pwm->args.period;
>         state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);

Reviewing this I noticed that in pwm_fan_resume() we use
DIV_ROUND_UP_ULL for what looks like essentially the same calculation.
Could we just switch this line to DIV_ROUND_UP_ULL instead?

>
> calculates a too small value for duty_cycle if the configured period is
> big (either by discarding the 64 bit value ctx->pwm->args.period or by
> overflowing the multiplication). As this results in a too slow fan and
> so maybe an overheating machine better be safe than sorry and error out
> in .probe.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/hwmon/pwm-fan.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 1f63807c0399..ec171f2b684a 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -324,8 +324,18 @@ static int pwm_fan_probe(struct platform_device *pdev)
>
>         ctx->pwm_value = MAX_PWM;
>
> -       /* Set duty cycle to maximum allowed and enable PWM output */
>         pwm_init_state(ctx->pwm, &state);
> +       /*
> +        * __set_pwm assumes that MAX_PWM * (period - 1) fits into an unsigned
> +        * long. Check this here to prevent the fan running at a too low
> +        * frequency.
> +        */
> +       if (state.period > ULONG_MAX / MAX_PWM + 1) {
> +               dev_err(dev, "Configured period too big\n");
> +               return -EINVAL;
> +       }
> +
> +       /* Set duty cycle to maximum allowed and enable PWM output */
>         state.duty_cycle = ctx->pwm->args.period - 1;
>         state.enabled = true;
>
>
> base-commit: 2c85ebc57b3e1817b6ce1a6b703928e113a90442
> --
> 2.29.2
>
Uwe Kleine-König Dec. 15, 2020, 12:56 p.m. UTC | #2 
On Tue, Dec 15, 2020 at 11:29:39AM +0000, Paul Barker wrote:
> On Tue, 15 Dec 2020 at 09:23, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > With MAX_PWM being defined to 255 the code
> >
> >         unsigned long period;
> >         ...
> >         period = ctx->pwm->args.period;
> >         state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
> 
> Reviewing this I noticed that in pwm_fan_resume() we use
> DIV_ROUND_UP_ULL for what looks like essentially the same calculation.

After my second patch this isn't true any more. With it applied
__set_pwm is the only place in the driver that calculates this stuff.

> Could we just switch this line to DIV_ROUND_UP_ULL instead?

Yes that would work, but actually I don't expect someone specifiying a
period big enough to justify the additional overhead of a 64 bit
division.

Best regards
Uwe
Paul Barker Dec. 15, 2020, 1:29 p.m. UTC | #3 
On Tue, 15 Dec 2020 at 12:56, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Tue, Dec 15, 2020 at 11:29:39AM +0000, Paul Barker wrote:
> > On Tue, 15 Dec 2020 at 09:23, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > With MAX_PWM being defined to 255 the code
> > >
> > >         unsigned long period;
> > >         ...
> > >         period = ctx->pwm->args.period;
> > >         state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
> >
> > Reviewing this I noticed that in pwm_fan_resume() we use
> > DIV_ROUND_UP_ULL for what looks like essentially the same calculation.
>
> After my second patch this isn't true any more. With it applied
> __set_pwm is the only place in the driver that calculates this stuff.
>
> > Could we just switch this line to DIV_ROUND_UP_ULL instead?
>
> Yes that would work, but actually I don't expect someone specifiying a
> period big enough to justify the additional overhead of a 64 bit
> division.

So ULONG_MAX / (MAX_PWM + 1) is 16.7 million on 32-bit platforms. As
the period is in nanoseconds (if I understand correctly), this would
allow a period of up to 16.7ms and so a minimum frequency of around
60Hz.

That does seem fairly reasonable to me but it's probably worth making
a note of these limits in the commit message for future reference.

Thanks,
Guenter Roeck Dec. 30, 2020, 4:20 p.m. UTC | #4 
On Tue, Dec 15, 2020 at 10:20:30AM +0100, Uwe Kleine-König wrote:
> With MAX_PWM being defined to 255 the code
> 
> 	unsigned long period;
> 	...
> 	period = ctx->pwm->args.period;
> 	state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
> 
> calculates a too small value for duty_cycle if the configured period is
> big (either by discarding the 64 bit value ctx->pwm->args.period or by
> overflowing the multiplication). As this results in a too slow fan and
> so maybe an overheating machine better be safe than sorry and error out
> in .probe.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/pwm-fan.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> 
> base-commit: 2c85ebc57b3e1817b6ce1a6b703928e113a90442
> 
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 1f63807c0399..ec171f2b684a 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -324,8 +324,18 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  
>  	ctx->pwm_value = MAX_PWM;
>  
> -	/* Set duty cycle to maximum allowed and enable PWM output */
>  	pwm_init_state(ctx->pwm, &state);
> +	/*
> +	 * __set_pwm assumes that MAX_PWM * (period - 1) fits into an unsigned
> +	 * long. Check this here to prevent the fan running at a too low
> +	 * frequency.
> +	 */
> +	if (state.period > ULONG_MAX / MAX_PWM + 1) {
> +		dev_err(dev, "Configured period too big\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Set duty cycle to maximum allowed and enable PWM output */
>  	state.duty_cycle = ctx->pwm->args.period - 1;
>  	state.enabled = true;
>
diff mbox series

Patch

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 1f63807c0399..ec171f2b684a 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -324,8 +324,18 @@  static int pwm_fan_probe(struct platform_device *pdev)
 
 	ctx->pwm_value = MAX_PWM;
 
-	/* Set duty cycle to maximum allowed and enable PWM output */
 	pwm_init_state(ctx->pwm, &state);
+	/*
+	 * __set_pwm assumes that MAX_PWM * (period - 1) fits into an unsigned
+	 * long. Check this here to prevent the fan running at a too low
+	 * frequency.
+	 */
+	if (state.period > ULONG_MAX / MAX_PWM + 1) {
+		dev_err(dev, "Configured period too big\n");
+		return -EINVAL;
+	}
+
+	/* Set duty cycle to maximum allowed and enable PWM output */
 	state.duty_cycle = ctx->pwm->args.period - 1;
 	state.enabled = true;