Re: [PATCH v2] platform/x86/tuxedo: Implement TUXEDO TUXI ACPI TFAN via hwmon

[Werner Sembach <wse@tuxedocomputers.com>]

Sorry, resend, mail client did html message by accident

Hi Ilpo,

Am 14.03.25 um 11:05 schrieb Ilpo Järvinen:

[snip]
>>>> +#define TUXI_MAX_FAN_COUNT 16           /* If this is increased, new
>>>> lines must
>>>> +					 * be added to hwmcinfo below.
>>>> +					 */
>>> Please use static_assert() to actually enforce what the comment says.
>> I actually struggle to come up with how to do this for the array length
>> because of the macro and the pointer
>>
>> `static_assert(TUXI_MAX_FAN_COUNT <= ARRAY_SIZE(hwmcinfo[0]->config));`
>> doesn't work
> Oh, I see. Yeah, there isn't way to get it to work in C.

ok

[snip]

>>> Add linux/limits.h include.
>> ack
>>
>> It is included in kernel.h, so I read from this that kernel.h should not be
>> used but the parts of it directly?
> kernel.h used to contain way too much of random things. Some people are
> slowly working towards splitting that up.
>
> So we try to include what is used directly, not rely on includes through
> some other include, and especially not through kernel.h.
ok
>>>> +			S32_MAX : (retval - TUXI_FW_TEMP_OFFSET) * 100;
>>> Is the math wrong, as you do retval - TUXI_FW_TEMP_OFFSET before
>>> multiplying?
>> No, retval is in 10th of °K (but the last number is always 0) so is
>> TUXI_FW_TEMP_OFFSET which is there to convert it to 10th of °C, the * 100 is
>> then to bring it in line with hwmon wanting to output milli degrees
> So is result of S32_MAX correct when retval is 21474837?
>
> (21474837-2730)*100
> 2147210700
> 2^31-1
> 2147483647
>
> 2147210700 would have been representable but the upper bound is
> still applied (the value might be large enough to not have practical
> significance but to me the code looks still illogical why it applies the
> bound prematurely).
Yeah my though was: this check is only here to catch the firmware doing some 
crazy stuff and sending highly unrealistic values, so gifting a small bit of the 
available range away doesn't matter
> I see you already sent another version, it would have been prudent to wait
> a bit longer as you contested some of the comments so you could have seen
> my replies before sending the next version.
I'm sorry. I just wanted to show that I'm iterating as I wait for the reply if 
the design with the periodic safeguard is acceptable. If that's gets rejected 
this driver must be rewritten anyway.
>>> Shouldn't it be like this:
>>>
>>> 		retval -= TUXI_FW_TEMP_OFFSET;
>>> 		*val = min(retval * 100, (unsigned long long)S32_MAX);
>> As retval is unsigned this would not work with (theoretical) negative °C.
> So your code relies on implicit type conversion in this: (retval -
> TUXI_FW_TEMP_OFFSET) ?

I can add an explicit cast, np.

[snip]

>>>> +	}
>>>> +	if (temp >= temp_high)
>>>> +		ret = i;
> Now that I reread things, is this also incorrect, as "i" is at the
> terminator entry at this point?

Yes that's intentional, the 3 entries in the array open up 4 ranges:

lower then 1st entry i=0, between 1st and 2nd entry i=1, 2nd and 3rd i=2, higher 
then 3rd i=3 (the value that terminates the for loop)

[snip]

>>>> +
>>>> +		temp = retval > S32_MAX / 100 ?
>>>> +			S32_MAX : (retval - TUXI_FW_TEMP_OFFSET) * 100;
>>> Same math issue comment as above.
>>>
>>> Why is the read+conversion code duplicated into two places?
>> because here it is with special error handling and didn't thought about an own
>> function for a defacto 2 liner
> A function that does read+conversion would be 6-8 lines with the error
> handling.

I can add it.

[snip]

Thanks for the code review again.


Last but not least: As already mentioned, I still wonder if the design with the 
periodic safeguard is ok or not or?

Best regards,

Werner