[PATCH] hwmon: (pwm-fan) Let ctx->pwm_value be assigned only in __set_pwm

[PATCH] hwmon: (pwm-fan) Let ctx->pwm_value be assigned only in __set_pwm

[Billy Tsai]

This patch is used to fix the bug when pwm_fan_probe the pwm_value will
be out of sync with the PWM hardware drivers.

Fixes: 86585c61972f ("hwmon: (pwm-fan) stop using legacy PWM functions and some cleanups")
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 drivers/hwmon/pwm-fan.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 17518b4cab1b..f12b9a28a232 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -336,8 +336,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
 			return ret;
 	}
 
-	ctx->pwm_value = MAX_PWM;
-
 	pwm_init_state(ctx->pwm, &ctx->pwm_state);
 
 	/*
-- 
2.25.1

Re: [PATCH] hwmon: (pwm-fan) Let ctx->pwm_value be assigned only in __set_pwm

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2111 bytes --]

Hello,

On Tue, Nov 30, 2021 at 11:00:46AM +0800, Billy Tsai wrote:
> This patch is used to fix the bug when pwm_fan_probe the pwm_value will
> be out of sync with the PWM hardware drivers.
> 
> Fixes: 86585c61972f ("hwmon: (pwm-fan) stop using legacy PWM functions and some cleanups")
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>  drivers/hwmon/pwm-fan.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 17518b4cab1b..f12b9a28a232 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -336,8 +336,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  			return ret;
>  	}
>  
> -	ctx->pwm_value = MAX_PWM;
> -
>  	pwm_init_state(ctx->pwm, &ctx->pwm_state);

Ah, I see. The effect is that the FAN is supposed to be running at full
speed after probe, but this isn't asserted.

So the patch is fine, but I suggest to make the commit log a bit more
frightening. Something like:

	hwmon: (pwm-fan) Ensure the fan going on in .probe()
	
	Before commit 86585c61972f ("hwmon: (pwm-fan) stop using legacy
	PWM functions and some cleanups") pwm_apply_state() was called
	unconditionally in pwm_fan_probe(). In this commit this direct
	call was replaced by a call to __set_pwm(ct, MAX_PWM) which
	however is a noop if ctx->pwm_value already matches the value to
	set.

	After probe the fan is supposed to run at full speed, and the
	internal driver state suggests it does, but this isn't asserted
	and depending on bootloader and pwm low-level driver, the fan
	might just be off.

	So drop setting pwm_value to MAXX_PWM to ensure the check in
	__set_pwm doesn't make it exit early and the fan goes on as
	intended.

	Cc: stable@vger.kernel.org
	Fixes: 86585c61972f ("hwmon: (pwm-fan) stop using legacy PWM functions and some cleanups")

Best regards and thanks for cleaning up after me,
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] hwmon: (pwm-fan) Let ctx->pwm_value be assigned only in __set_pwm

[Billy Tsai]

Hi Uwe,

On 2021/11/30, 4:21 PM, "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> wrote:

    Hello,

    On Tue, Nov 30, 2021 at 11:00:46AM +0800, Billy Tsai wrote:
    >   > This patch is used to fix the bug when pwm_fan_probe the pwm_value will
    >   > be out of sync with the PWM hardware drivers.
    >   > 
    >   > Fixes: 86585c61972f ("hwmon: (pwm-fan) stop using legacy PWM functions and some cleanups")
    >   > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
    >   > ---
    >   >  drivers/hwmon/pwm-fan.c | 2 --
    >   >  1 file changed, 2 deletions(-)
    >   > 
    >   > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
    >   > index 17518b4cab1b..f12b9a28a232 100644
    >   > --- a/drivers/hwmon/pwm-fan.c
    >   > +++ b/drivers/hwmon/pwm-fan.c
    >   > @@ -336,8 +336,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
    >   >  			return ret;
    >   >  	}
    >   >  
    >   > -	ctx->pwm_value = MAX_PWM;
    >   > -
    >   >  	pwm_init_state(ctx->pwm, &ctx->pwm_state);

    >   Ah, I see. The effect is that the FAN is supposed to be running at full
    >   speed after probe, but this isn't asserted.

    >   So the patch is fine, but I suggest to make the commit log a bit more
    >   frightening. Something like:

    >   	hwmon: (pwm-fan) Ensure the fan going on in .probe()

    >   	Before commit 86585c61972f ("hwmon: (pwm-fan) stop using legacy
    >   	PWM functions and some cleanups") pwm_apply_state() was called
    >   	unconditionally in pwm_fan_probe(). In this commit this direct
    >   	call was replaced by a call to __set_pwm(ct, MAX_PWM) which
    >   	however is a noop if ctx->pwm_value already matches the value to
    >   	set.

    >   	After probe the fan is supposed to run at full speed, and the
    >   	internal driver state suggests it does, but this isn't asserted
    >   	and depending on bootloader and pwm low-level driver, the fan
    >   	might just be off.

    >   	So drop setting pwm_value to MAXX_PWM to ensure the check in
    >   	__set_pwm doesn't make it exit early and the fan goes on as
    >   	intended.

    >   	Cc: stable@vger.kernel.org
    >   	Fixes: 86585c61972f ("hwmon: (pwm-fan) stop using legacy PWM functions and some cleanups")
Ok, I will use this commit log and resend the patch.

Best Regards,
Billy Tsai