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

From: Werner Sembach
Date: Fri Mar 14 2025 - 08:40:17 EST


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