* [PATCH 1/2] hwmon: pwm-fan: Ensure that calculation doesn't discard big period values
@ 2020-12-15 9:20 Uwe Kleine-König
2020-12-15 9:20 ` [PATCH 2/2] hwmon: pwm-fan: stop using legacy PWM functions and some cleanups Uwe Kleine-König
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Uwe Kleine-König @ 2020-12-15 9:20 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Jean Delvare, Guenter Roeck
Cc: Thierry Reding, Lee Jones, linux-hwmon, linux-pwm, kernel
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(-)
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] hwmon: pwm-fan: stop using legacy PWM functions and some cleanups
2020-12-15 9:20 [PATCH 1/2] hwmon: pwm-fan: Ensure that calculation doesn't discard big period values Uwe Kleine-König
@ 2020-12-15 9:20 ` Uwe Kleine-König
2020-12-15 11:45 ` Paul Barker
2020-12-30 16:35 ` Guenter Roeck
2020-12-15 11:29 ` [PATCH 1/2] hwmon: pwm-fan: Ensure that calculation doesn't discard big period values Paul Barker
2020-12-30 16:20 ` Guenter Roeck
2 siblings, 2 replies; 10+ messages in thread
From: Uwe Kleine-König @ 2020-12-15 9:20 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Jean Delvare, Guenter Roeck
Cc: Thierry Reding, Lee Jones, linux-hwmon, linux-pwm, kernel
pwm_apply_state() does what the legacy functions pwm_config() and
pwm_{en,dis}able() do in a single function call. This simplifies error
handling and is more efficient for new-style PWM hardware drivers.
Instead of repeatedly querying the PWM framework about the initial PWM
configuration, cache the settings in driver data.
Also use __set_pwm() in .probe() to have the algorithm calculating the PWM
state in a single place.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
drivers/hwmon/pwm-fan.c | 47 +++++++++++++++++------------------------
1 file changed, 19 insertions(+), 28 deletions(-)
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index ec171f2b684a..4ccad5a87019 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -25,6 +25,7 @@
struct pwm_fan_ctx {
struct mutex lock;
struct pwm_device *pwm;
+ struct pwm_state pwm_state;
struct regulator *reg_en;
int irq;
@@ -73,18 +74,17 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
{
unsigned long period;
int ret = 0;
- struct pwm_state state = { };
+ struct pwm_state *state = &ctx->pwm_state;
mutex_lock(&ctx->lock);
if (ctx->pwm_value == pwm)
goto exit_set_pwm_err;
- pwm_init_state(ctx->pwm, &state);
- period = ctx->pwm->args.period;
- state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
- state.enabled = pwm ? true : false;
+ period = state->period;
+ state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
+ state->enabled = pwm ? true : false;
- ret = pwm_apply_state(ctx->pwm, &state);
+ ret = pwm_apply_state(ctx->pwm, state);
if (!ret)
ctx->pwm_value = pwm;
exit_set_pwm_err:
@@ -274,7 +274,9 @@ static void pwm_fan_regulator_disable(void *data)
static void pwm_fan_pwm_disable(void *__ctx)
{
struct pwm_fan_ctx *ctx = __ctx;
- pwm_disable(ctx->pwm);
+
+ ctx->pwm_state.enabled = false;
+ pwm_apply_state(ctx->pwm, &ctx->pwm_state);
del_timer_sync(&ctx->rpm_timer);
}
@@ -285,7 +287,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
struct pwm_fan_ctx *ctx;
struct device *hwmon;
int ret;
- struct pwm_state state = { };
u32 ppr = 2;
ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
@@ -324,22 +325,20 @@ static int pwm_fan_probe(struct platform_device *pdev)
ctx->pwm_value = MAX_PWM;
- pwm_init_state(ctx->pwm, &state);
+ pwm_init_state(ctx->pwm, &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) {
+ if (ctx->pwm_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;
-
- ret = pwm_apply_state(ctx->pwm, &state);
+ ret = __set_pwm(ctx, MAX_PWM);
if (ret) {
dev_err(dev, "Failed to configure PWM: %d\n", ret);
return ret;
@@ -399,17 +398,16 @@ static int pwm_fan_probe(struct platform_device *pdev)
static int pwm_fan_disable(struct device *dev)
{
struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
- struct pwm_args args;
int ret;
- pwm_get_args(ctx->pwm, &args);
-
if (ctx->pwm_value) {
- ret = pwm_config(ctx->pwm, 0, args.period);
+ /* keep ctx->pwm_state unmodified for pwm_fan_resume() */
+ struct pwm_state state = ctx->pwm_state;
+ state.duty_cycle = 0;
+ state.enabled = false;
+ ret = pwm_apply_state(ctx->pwm, &state);
if (ret < 0)
return ret;
-
- pwm_disable(ctx->pwm);
}
if (ctx->reg_en) {
@@ -437,8 +435,6 @@ static int pwm_fan_suspend(struct device *dev)
static int pwm_fan_resume(struct device *dev)
{
struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
- struct pwm_args pargs;
- unsigned long duty;
int ret;
if (ctx->reg_en) {
@@ -452,12 +448,7 @@ static int pwm_fan_resume(struct device *dev)
if (ctx->pwm_value == 0)
return 0;
- pwm_get_args(ctx->pwm, &pargs);
- duty = DIV_ROUND_UP_ULL(ctx->pwm_value * (pargs.period - 1), MAX_PWM);
- ret = pwm_config(ctx->pwm, duty, pargs.period);
- if (ret)
- return ret;
- return pwm_enable(ctx->pwm);
+ return pwm_apply_state(ctx->pwm, &ctx->pwm_state);
}
#endif
--
2.29.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] hwmon: pwm-fan: Ensure that calculation doesn't discard big period values
2020-12-15 9:20 [PATCH 1/2] hwmon: pwm-fan: Ensure that calculation doesn't discard big period values Uwe Kleine-König
2020-12-15 9:20 ` [PATCH 2/2] hwmon: pwm-fan: stop using legacy PWM functions and some cleanups Uwe Kleine-König
@ 2020-12-15 11:29 ` Paul Barker
2020-12-15 12:56 ` Uwe Kleine-König
2020-12-30 16:20 ` Guenter Roeck
2 siblings, 1 reply; 10+ messages in thread
From: Paul Barker @ 2020-12-15 11:29 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Bartlomiej Zolnierkiewicz, Jean Delvare, Guenter Roeck,
Thierry Reding, Lee Jones, linux-hwmon, linux-pwm, kernel
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
>
--
Paul Barker
Konsulko Group
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] hwmon: pwm-fan: stop using legacy PWM functions and some cleanups
2020-12-15 9:20 ` [PATCH 2/2] hwmon: pwm-fan: stop using legacy PWM functions and some cleanups Uwe Kleine-König
@ 2020-12-15 11:45 ` Paul Barker
2020-12-30 16:35 ` Guenter Roeck
1 sibling, 0 replies; 10+ messages in thread
From: Paul Barker @ 2020-12-15 11:45 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Bartlomiej Zolnierkiewicz, Jean Delvare, Guenter Roeck,
Thierry Reding, Lee Jones, linux-hwmon, linux-pwm, kernel
On Tue, 15 Dec 2020 at 09:23, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> pwm_apply_state() does what the legacy functions pwm_config() and
> pwm_{en,dis}able() do in a single function call. This simplifies error
> handling and is more efficient for new-style PWM hardware drivers.
>
> Instead of repeatedly querying the PWM framework about the initial PWM
> configuration, cache the settings in driver data.
>
> Also use __set_pwm() in .probe() to have the algorithm calculating the PWM
> state in a single place.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> drivers/hwmon/pwm-fan.c | 47 +++++++++++++++++------------------------
> 1 file changed, 19 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index ec171f2b684a..4ccad5a87019 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -25,6 +25,7 @@
> struct pwm_fan_ctx {
> struct mutex lock;
> struct pwm_device *pwm;
> + struct pwm_state pwm_state;
> struct regulator *reg_en;
>
> int irq;
> @@ -73,18 +74,17 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
> {
> unsigned long period;
> int ret = 0;
> - struct pwm_state state = { };
> + struct pwm_state *state = &ctx->pwm_state;
>
> mutex_lock(&ctx->lock);
> if (ctx->pwm_value == pwm)
> goto exit_set_pwm_err;
>
> - pwm_init_state(ctx->pwm, &state);
> - period = ctx->pwm->args.period;
> - state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
> - state.enabled = pwm ? true : false;
> + period = state->period;
> + state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
> + state->enabled = pwm ? true : false;
>
> - ret = pwm_apply_state(ctx->pwm, &state);
> + ret = pwm_apply_state(ctx->pwm, state);
> if (!ret)
> ctx->pwm_value = pwm;
> exit_set_pwm_err:
> @@ -274,7 +274,9 @@ static void pwm_fan_regulator_disable(void *data)
> static void pwm_fan_pwm_disable(void *__ctx)
> {
> struct pwm_fan_ctx *ctx = __ctx;
> - pwm_disable(ctx->pwm);
> +
> + ctx->pwm_state.enabled = false;
> + pwm_apply_state(ctx->pwm, &ctx->pwm_state);
> del_timer_sync(&ctx->rpm_timer);
> }
>
> @@ -285,7 +287,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
> struct pwm_fan_ctx *ctx;
> struct device *hwmon;
> int ret;
> - struct pwm_state state = { };
> u32 ppr = 2;
>
> ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> @@ -324,22 +325,20 @@ static int pwm_fan_probe(struct platform_device *pdev)
>
> ctx->pwm_value = MAX_PWM;
>
> - pwm_init_state(ctx->pwm, &state);
> + pwm_init_state(ctx->pwm, &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) {
> + if (ctx->pwm_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;
> -
> - ret = pwm_apply_state(ctx->pwm, &state);
> + ret = __set_pwm(ctx, MAX_PWM);
> if (ret) {
> dev_err(dev, "Failed to configure PWM: %d\n", ret);
> return ret;
> @@ -399,17 +398,16 @@ static int pwm_fan_probe(struct platform_device *pdev)
> static int pwm_fan_disable(struct device *dev)
> {
> struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> - struct pwm_args args;
> int ret;
>
> - pwm_get_args(ctx->pwm, &args);
> -
> if (ctx->pwm_value) {
> - ret = pwm_config(ctx->pwm, 0, args.period);
> + /* keep ctx->pwm_state unmodified for pwm_fan_resume() */
> + struct pwm_state state = ctx->pwm_state;
> + state.duty_cycle = 0;
> + state.enabled = false;
> + ret = pwm_apply_state(ctx->pwm, &state);
> if (ret < 0)
> return ret;
> -
> - pwm_disable(ctx->pwm);
> }
>
> if (ctx->reg_en) {
> @@ -437,8 +435,6 @@ static int pwm_fan_suspend(struct device *dev)
> static int pwm_fan_resume(struct device *dev)
> {
> struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> - struct pwm_args pargs;
> - unsigned long duty;
> int ret;
>
> if (ctx->reg_en) {
> @@ -452,12 +448,7 @@ static int pwm_fan_resume(struct device *dev)
> if (ctx->pwm_value == 0)
> return 0;
>
> - pwm_get_args(ctx->pwm, &pargs);
> - duty = DIV_ROUND_UP_ULL(ctx->pwm_value * (pargs.period - 1), MAX_PWM);
> - ret = pwm_config(ctx->pwm, duty, pargs.period);
> - if (ret)
> - return ret;
> - return pwm_enable(ctx->pwm);
> + return pwm_apply_state(ctx->pwm, &ctx->pwm_state);
> }
> #endif
>
> --
> 2.29.2
>
All looks good to me at first glance.
This does conflict with the changes I proposed
(https://lore.kernel.org/linux-hwmon/20201212195008.6036-1-pbarker@konsulko.com/T/#t)
so we'll need to figure out that out depending which order they get
applied in (assuming both changes get accepted).
Thanks,
--
Paul Barker
Konsulko Group
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] hwmon: pwm-fan: Ensure that calculation doesn't discard big period values
2020-12-15 11:29 ` [PATCH 1/2] hwmon: pwm-fan: Ensure that calculation doesn't discard big period values Paul Barker
@ 2020-12-15 12:56 ` Uwe Kleine-König
2020-12-15 13:29 ` Paul Barker
0 siblings, 1 reply; 10+ messages in thread
From: Uwe Kleine-König @ 2020-12-15 12:56 UTC (permalink / raw)
To: Paul Barker
Cc: linux-hwmon, linux-pwm, Jean Delvare, Bartlomiej Zolnierkiewicz,
Thierry Reding, kernel, Lee Jones, Guenter Roeck
[-- Attachment #1: Type: text/plain, Size: 1057 bytes --]
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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] hwmon: pwm-fan: Ensure that calculation doesn't discard big period values
2020-12-15 12:56 ` Uwe Kleine-König
@ 2020-12-15 13:29 ` Paul Barker
0 siblings, 0 replies; 10+ messages in thread
From: Paul Barker @ 2020-12-15 13:29 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-hwmon, linux-pwm, Jean Delvare, Bartlomiej Zolnierkiewicz,
Thierry Reding, kernel, Lee Jones, Guenter Roeck
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,
--
Paul Barker
Konsulko Group
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] hwmon: pwm-fan: Ensure that calculation doesn't discard big period values
2020-12-15 9:20 [PATCH 1/2] hwmon: pwm-fan: Ensure that calculation doesn't discard big period values Uwe Kleine-König
2020-12-15 9:20 ` [PATCH 2/2] hwmon: pwm-fan: stop using legacy PWM functions and some cleanups Uwe Kleine-König
2020-12-15 11:29 ` [PATCH 1/2] hwmon: pwm-fan: Ensure that calculation doesn't discard big period values Paul Barker
@ 2020-12-30 16:20 ` Guenter Roeck
2 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2020-12-30 16:20 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Bartlomiej Zolnierkiewicz, Jean Delvare, Thierry Reding,
Lee Jones, linux-hwmon, linux-pwm, kernel
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;
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] hwmon: pwm-fan: stop using legacy PWM functions and some cleanups
2020-12-15 9:20 ` [PATCH 2/2] hwmon: pwm-fan: stop using legacy PWM functions and some cleanups Uwe Kleine-König
2020-12-15 11:45 ` Paul Barker
@ 2020-12-30 16:35 ` Guenter Roeck
2021-01-12 19:13 ` [PATCH] " Uwe Kleine-König
1 sibling, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2020-12-30 16:35 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Bartlomiej Zolnierkiewicz, Jean Delvare, Thierry Reding,
Lee Jones, linux-hwmon, linux-pwm, kernel
On Tue, Dec 15, 2020 at 10:20:31AM +0100, Uwe Kleine-König wrote:
> pwm_apply_state() does what the legacy functions pwm_config() and
> pwm_{en,dis}able() do in a single function call. This simplifies error
> handling and is more efficient for new-style PWM hardware drivers.
>
> Instead of repeatedly querying the PWM framework about the initial PWM
> configuration, cache the settings in driver data.
>
> Also use __set_pwm() in .probe() to have the algorithm calculating the PWM
> state in a single place.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Can you rebase this patch on top of hwmon-next ?
Thanks,
Guenter
> ---
> drivers/hwmon/pwm-fan.c | 47 +++++++++++++++++------------------------
> 1 file changed, 19 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index ec171f2b684a..4ccad5a87019 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -25,6 +25,7 @@
> struct pwm_fan_ctx {
> struct mutex lock;
> struct pwm_device *pwm;
> + struct pwm_state pwm_state;
> struct regulator *reg_en;
>
> int irq;
> @@ -73,18 +74,17 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
> {
> unsigned long period;
> int ret = 0;
> - struct pwm_state state = { };
> + struct pwm_state *state = &ctx->pwm_state;
>
> mutex_lock(&ctx->lock);
> if (ctx->pwm_value == pwm)
> goto exit_set_pwm_err;
>
> - pwm_init_state(ctx->pwm, &state);
> - period = ctx->pwm->args.period;
> - state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
> - state.enabled = pwm ? true : false;
> + period = state->period;
> + state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
> + state->enabled = pwm ? true : false;
>
> - ret = pwm_apply_state(ctx->pwm, &state);
> + ret = pwm_apply_state(ctx->pwm, state);
> if (!ret)
> ctx->pwm_value = pwm;
> exit_set_pwm_err:
> @@ -274,7 +274,9 @@ static void pwm_fan_regulator_disable(void *data)
> static void pwm_fan_pwm_disable(void *__ctx)
> {
> struct pwm_fan_ctx *ctx = __ctx;
> - pwm_disable(ctx->pwm);
> +
> + ctx->pwm_state.enabled = false;
> + pwm_apply_state(ctx->pwm, &ctx->pwm_state);
> del_timer_sync(&ctx->rpm_timer);
> }
>
> @@ -285,7 +287,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
> struct pwm_fan_ctx *ctx;
> struct device *hwmon;
> int ret;
> - struct pwm_state state = { };
> u32 ppr = 2;
>
> ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> @@ -324,22 +325,20 @@ static int pwm_fan_probe(struct platform_device *pdev)
>
> ctx->pwm_value = MAX_PWM;
>
> - pwm_init_state(ctx->pwm, &state);
> + pwm_init_state(ctx->pwm, &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) {
> + if (ctx->pwm_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;
> -
> - ret = pwm_apply_state(ctx->pwm, &state);
> + ret = __set_pwm(ctx, MAX_PWM);
> if (ret) {
> dev_err(dev, "Failed to configure PWM: %d\n", ret);
> return ret;
> @@ -399,17 +398,16 @@ static int pwm_fan_probe(struct platform_device *pdev)
> static int pwm_fan_disable(struct device *dev)
> {
> struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> - struct pwm_args args;
> int ret;
>
> - pwm_get_args(ctx->pwm, &args);
> -
> if (ctx->pwm_value) {
> - ret = pwm_config(ctx->pwm, 0, args.period);
> + /* keep ctx->pwm_state unmodified for pwm_fan_resume() */
> + struct pwm_state state = ctx->pwm_state;
> + state.duty_cycle = 0;
> + state.enabled = false;
> + ret = pwm_apply_state(ctx->pwm, &state);
> if (ret < 0)
> return ret;
> -
> - pwm_disable(ctx->pwm);
> }
>
> if (ctx->reg_en) {
> @@ -437,8 +435,6 @@ static int pwm_fan_suspend(struct device *dev)
> static int pwm_fan_resume(struct device *dev)
> {
> struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> - struct pwm_args pargs;
> - unsigned long duty;
> int ret;
>
> if (ctx->reg_en) {
> @@ -452,12 +448,7 @@ static int pwm_fan_resume(struct device *dev)
> if (ctx->pwm_value == 0)
> return 0;
>
> - pwm_get_args(ctx->pwm, &pargs);
> - duty = DIV_ROUND_UP_ULL(ctx->pwm_value * (pargs.period - 1), MAX_PWM);
> - ret = pwm_config(ctx->pwm, duty, pargs.period);
> - if (ret)
> - return ret;
> - return pwm_enable(ctx->pwm);
> + return pwm_apply_state(ctx->pwm, &ctx->pwm_state);
> }
> #endif
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] hwmon: pwm-fan: stop using legacy PWM functions and some cleanups
2020-12-30 16:35 ` Guenter Roeck
@ 2021-01-12 19:13 ` Uwe Kleine-König
2021-01-23 16:37 ` Guenter Roeck
0 siblings, 1 reply; 10+ messages in thread
From: Uwe Kleine-König @ 2021-01-12 19:13 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Jean Delvare, Guenter Roeck
Cc: linux-hwmon, linux-pwm, Thierry Reding, Lee Jones, kernel, Paul Barker
pwm_apply_state() does what the legacy functions pwm_config() and
pwm_{en,dis}able() do in a single function call. This simplifies error
handling and is more efficient for new-style PWM hardware drivers.
Instead of repeatedly querying the PWM framework about the initial PWM
configuration, cache the settings in driver data.
Also use __set_pwm() in .probe() to have the algorithm calculating the PWM
state in a single place.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
On Wed, Dec 30, 2020 at 08:35:20AM -0800, Guenter Roeck wrote:
> Can you rebase this patch on top of hwmon-next ?
Sure, here it is.
Best regards
Uwe
drivers/hwmon/pwm-fan.c | 47 +++++++++++++++++------------------------
1 file changed, 19 insertions(+), 28 deletions(-)
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index b5204aeb8085..41cb829784a2 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -31,6 +31,7 @@ struct pwm_fan_tach {
struct pwm_fan_ctx {
struct mutex lock;
struct pwm_device *pwm;
+ struct pwm_state pwm_state;
struct regulator *reg_en;
int tach_count;
@@ -95,18 +96,17 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
{
unsigned long period;
int ret = 0;
- struct pwm_state state = { };
+ struct pwm_state *state = &ctx->pwm_state;
mutex_lock(&ctx->lock);
if (ctx->pwm_value == pwm)
goto exit_set_pwm_err;
- pwm_init_state(ctx->pwm, &state);
- period = ctx->pwm->args.period;
- state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
- state.enabled = pwm ? true : false;
+ period = state->period;
+ state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
+ state->enabled = pwm ? true : false;
- ret = pwm_apply_state(ctx->pwm, &state);
+ ret = pwm_apply_state(ctx->pwm, state);
if (!ret)
ctx->pwm_value = pwm;
exit_set_pwm_err:
@@ -288,7 +288,9 @@ static void pwm_fan_regulator_disable(void *data)
static void pwm_fan_pwm_disable(void *__ctx)
{
struct pwm_fan_ctx *ctx = __ctx;
- pwm_disable(ctx->pwm);
+
+ ctx->pwm_state.enabled = false;
+ pwm_apply_state(ctx->pwm, &ctx->pwm_state);
del_timer_sync(&ctx->rpm_timer);
}
@@ -299,7 +301,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
struct pwm_fan_ctx *ctx;
struct device *hwmon;
int ret;
- struct pwm_state state = { };
const struct hwmon_channel_info **channels;
u32 *fan_channel_config;
int channel_count = 1; /* We always have a PWM channel. */
@@ -337,22 +338,20 @@ static int pwm_fan_probe(struct platform_device *pdev)
ctx->pwm_value = MAX_PWM;
- pwm_init_state(ctx->pwm, &state);
+ pwm_init_state(ctx->pwm, &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) {
+ if (ctx->pwm_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;
-
- ret = pwm_apply_state(ctx->pwm, &state);
+ ret = __set_pwm(ctx, MAX_PWM);
if (ret) {
dev_err(dev, "Failed to configure PWM: %d\n", ret);
return ret;
@@ -468,17 +467,16 @@ static int pwm_fan_probe(struct platform_device *pdev)
static int pwm_fan_disable(struct device *dev)
{
struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
- struct pwm_args args;
int ret;
- pwm_get_args(ctx->pwm, &args);
-
if (ctx->pwm_value) {
- ret = pwm_config(ctx->pwm, 0, args.period);
+ /* keep ctx->pwm_state unmodified for pwm_fan_resume() */
+ struct pwm_state state = ctx->pwm_state;
+ state.duty_cycle = 0;
+ state.enabled = false;
+ ret = pwm_apply_state(ctx->pwm, &state);
if (ret < 0)
return ret;
-
- pwm_disable(ctx->pwm);
}
if (ctx->reg_en) {
@@ -506,8 +504,6 @@ static int pwm_fan_suspend(struct device *dev)
static int pwm_fan_resume(struct device *dev)
{
struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
- struct pwm_args pargs;
- unsigned long duty;
int ret;
if (ctx->reg_en) {
@@ -521,12 +517,7 @@ static int pwm_fan_resume(struct device *dev)
if (ctx->pwm_value == 0)
return 0;
- pwm_get_args(ctx->pwm, &pargs);
- duty = DIV_ROUND_UP_ULL(ctx->pwm_value * (pargs.period - 1), MAX_PWM);
- ret = pwm_config(ctx->pwm, duty, pargs.period);
- if (ret)
- return ret;
- return pwm_enable(ctx->pwm);
+ return pwm_apply_state(ctx->pwm, &ctx->pwm_state);
}
#endif
base-commit: d1f7b079ce5b69c88c813439eea6a9c133f0846b
--
2.29.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] hwmon: pwm-fan: stop using legacy PWM functions and some cleanups
2021-01-12 19:13 ` [PATCH] " Uwe Kleine-König
@ 2021-01-23 16:37 ` Guenter Roeck
0 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2021-01-23 16:37 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Bartlomiej Zolnierkiewicz, Jean Delvare, linux-hwmon, linux-pwm,
Thierry Reding, Lee Jones, kernel, Paul Barker
On Tue, Jan 12, 2021 at 08:13:14PM +0100, Uwe Kleine-König wrote:
> pwm_apply_state() does what the legacy functions pwm_config() and
> pwm_{en,dis}able() do in a single function call. This simplifies error
> handling and is more efficient for new-style PWM hardware drivers.
>
> Instead of repeatedly querying the PWM framework about the initial PWM
> configuration, cache the settings in driver data.
>
> Also use __set_pwm() in .probe() to have the algorithm calculating the PWM
> state in a single place.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Applied.
Thanks,
Guenter
> ---
>
> On Wed, Dec 30, 2020 at 08:35:20AM -0800, Guenter Roeck wrote:
> > Can you rebase this patch on top of hwmon-next ?
>
> Sure, here it is.
>
> Best regards
> Uwe
>
> drivers/hwmon/pwm-fan.c | 47 +++++++++++++++++------------------------
> 1 file changed, 19 insertions(+), 28 deletions(-)
>
>
> base-commit: d1f7b079ce5b69c88c813439eea6a9c133f0846b
>
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index b5204aeb8085..41cb829784a2 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -31,6 +31,7 @@ struct pwm_fan_tach {
> struct pwm_fan_ctx {
> struct mutex lock;
> struct pwm_device *pwm;
> + struct pwm_state pwm_state;
> struct regulator *reg_en;
>
> int tach_count;
> @@ -95,18 +96,17 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
> {
> unsigned long period;
> int ret = 0;
> - struct pwm_state state = { };
> + struct pwm_state *state = &ctx->pwm_state;
>
> mutex_lock(&ctx->lock);
> if (ctx->pwm_value == pwm)
> goto exit_set_pwm_err;
>
> - pwm_init_state(ctx->pwm, &state);
> - period = ctx->pwm->args.period;
> - state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
> - state.enabled = pwm ? true : false;
> + period = state->period;
> + state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
> + state->enabled = pwm ? true : false;
>
> - ret = pwm_apply_state(ctx->pwm, &state);
> + ret = pwm_apply_state(ctx->pwm, state);
> if (!ret)
> ctx->pwm_value = pwm;
> exit_set_pwm_err:
> @@ -288,7 +288,9 @@ static void pwm_fan_regulator_disable(void *data)
> static void pwm_fan_pwm_disable(void *__ctx)
> {
> struct pwm_fan_ctx *ctx = __ctx;
> - pwm_disable(ctx->pwm);
> +
> + ctx->pwm_state.enabled = false;
> + pwm_apply_state(ctx->pwm, &ctx->pwm_state);
> del_timer_sync(&ctx->rpm_timer);
> }
>
> @@ -299,7 +301,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
> struct pwm_fan_ctx *ctx;
> struct device *hwmon;
> int ret;
> - struct pwm_state state = { };
> const struct hwmon_channel_info **channels;
> u32 *fan_channel_config;
> int channel_count = 1; /* We always have a PWM channel. */
> @@ -337,22 +338,20 @@ static int pwm_fan_probe(struct platform_device *pdev)
>
> ctx->pwm_value = MAX_PWM;
>
> - pwm_init_state(ctx->pwm, &state);
> + pwm_init_state(ctx->pwm, &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) {
> + if (ctx->pwm_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;
> -
> - ret = pwm_apply_state(ctx->pwm, &state);
> + ret = __set_pwm(ctx, MAX_PWM);
> if (ret) {
> dev_err(dev, "Failed to configure PWM: %d\n", ret);
> return ret;
> @@ -468,17 +467,16 @@ static int pwm_fan_probe(struct platform_device *pdev)
> static int pwm_fan_disable(struct device *dev)
> {
> struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> - struct pwm_args args;
> int ret;
>
> - pwm_get_args(ctx->pwm, &args);
> -
> if (ctx->pwm_value) {
> - ret = pwm_config(ctx->pwm, 0, args.period);
> + /* keep ctx->pwm_state unmodified for pwm_fan_resume() */
> + struct pwm_state state = ctx->pwm_state;
> + state.duty_cycle = 0;
> + state.enabled = false;
> + ret = pwm_apply_state(ctx->pwm, &state);
> if (ret < 0)
> return ret;
> -
> - pwm_disable(ctx->pwm);
> }
>
> if (ctx->reg_en) {
> @@ -506,8 +504,6 @@ static int pwm_fan_suspend(struct device *dev)
> static int pwm_fan_resume(struct device *dev)
> {
> struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> - struct pwm_args pargs;
> - unsigned long duty;
> int ret;
>
> if (ctx->reg_en) {
> @@ -521,12 +517,7 @@ static int pwm_fan_resume(struct device *dev)
> if (ctx->pwm_value == 0)
> return 0;
>
> - pwm_get_args(ctx->pwm, &pargs);
> - duty = DIV_ROUND_UP_ULL(ctx->pwm_value * (pargs.period - 1), MAX_PWM);
> - ret = pwm_config(ctx->pwm, duty, pargs.period);
> - if (ret)
> - return ret;
> - return pwm_enable(ctx->pwm);
> + return pwm_apply_state(ctx->pwm, &ctx->pwm_state);
> }
> #endif
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-01-23 16:38 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-15 9:20 [PATCH 1/2] hwmon: pwm-fan: Ensure that calculation doesn't discard big period values Uwe Kleine-König
2020-12-15 9:20 ` [PATCH 2/2] hwmon: pwm-fan: stop using legacy PWM functions and some cleanups Uwe Kleine-König
2020-12-15 11:45 ` Paul Barker
2020-12-30 16:35 ` Guenter Roeck
2021-01-12 19:13 ` [PATCH] " Uwe Kleine-König
2021-01-23 16:37 ` Guenter Roeck
2020-12-15 11:29 ` [PATCH 1/2] hwmon: pwm-fan: Ensure that calculation doesn't discard big period values Paul Barker
2020-12-15 12:56 ` Uwe Kleine-König
2020-12-15 13:29 ` Paul Barker
2020-12-30 16:20 ` Guenter Roeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).