Re: [PATCH 1/1] platform/x86: thinkpad_acpi: skip invalid fan speed

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 10/14/22 23:17, Jelle van der Waa wrote:
> 65535 is most likely an invalid read.

I wonder if it is an invalid read, or if it actually is a reserved value
which means "FAN_NOT_PRESENT"

I'm tempted to add:

#define FAN_NOT_PRESENT		65535

and then change the check to:

			if (res >= 0 && speed != FAN_NOT_PRESENT) {


That would make the code more logical to read.

Jelle, can you make this change for v2 ? Also please Cc: platform-driver-x86@vger.kernel.org
for v2.

Mark, what do you think of this change (and of adding the
FAN_NOT_PRESENT define) ?

Regards,

Hans



> 
> Signed-off-by: Jelle van der Waa <jvanderwaa@redhat.com>
> 
> ---
> 
> Cc: Mark Pearson <markpearson@lenovo.com>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 6a823b850a77..7e0f72dc53b7 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8876,7 +8876,7 @@ static int __init fan_init(struct ibm_init_struct *iibm)
>  			/* Try and probe the 2nd fan */
>  			tp_features.second_fan = 1; /* needed for get_speed to work */
>  			res = fan2_get_speed(&speed);
> -			if (res >= 0) {
> +			if (res >= 0 && speed != 65535) {
>  				/* It responded - so let's assume it's there */
>  				tp_features.second_fan = 1;
>  				tp_features.second_fan_ctl = 1;