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

[Werner Sembach]

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