HI Hans
On 2022-10-15 10:22, Hans de Goede wrote:
> 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@xxxxxxxxxxxxxxx
> for v2.
>
> Mark, what do you think of this change (and of adding the
> FAN_NOT_PRESENT define) ?
Looks good to me.
I've asked the FW team to see if they can confirm if this will be
standard behaviour (it's not defined in the spec that I have) and will
update if I get an answer. But regardless I think it makes sense.
Mark
>
> Regards,
>
> Hans
>
>
>
>>
>> Signed-off-by: Jelle van der Waa <jvanderwaa@xxxxxxxxxx>
>>
>> ---
>>
>> Cc: Mark Pearson <markpearson@xxxxxxxxxx>
>> ---
>> 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;
>
