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

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

[Uwe Kleine-König]

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

[PATCH 2/2] hwmon: pwm-fan: stop using legacy PWM functions and some cleanups

[Uwe Kleine-König]

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

Re: [PATCH 2/2] hwmon: pwm-fan: stop using legacy PWM functions and some cleanups

[Paul Barker]

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

Re: [PATCH 2/2] hwmon: pwm-fan: stop using legacy PWM functions and some cleanups

[Guenter Roeck]

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
>  

[PATCH] hwmon: pwm-fan: stop using legacy PWM functions and some cleanups

[Uwe Kleine-König]

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

Re: [PATCH] hwmon: pwm-fan: stop using legacy PWM functions and some cleanups

[Guenter Roeck]

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
>  

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

[Paul Barker]

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

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

[Uwe Kleine-König]

[-- 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 --]

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

[Paul Barker]

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

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

[Guenter Roeck]

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;
>