Re: [PATCH] w1: therm: Use clamp_t to simplify int_to_short helper

[David Laight <david.laight.linux@gmail.com>]

On Sun, 9 Nov 2025 21:30:00 +0100
Thorsten Blum <thorsten.blum@linux.dev> wrote:

> On 9. Nov 2025, at 17:20, David Laight wrote:
> > On Sun,  9 Nov 2025 13:59:55 +0100
> > Thorsten Blum <thorsten.blum@linux.dev> wrote:
> >   
> >> Use clamp_t() instead of manually casting the return value.
> >> 
> >> Replace sprintf() with sysfs_emit() to improve sysfs show functions
> >> while we're at it.
> >> 
> >> ...
> >> +	/* Cast to short by eliminating out of range values */  
> >                  ^^^^^ no shorts here...  
> 
> It's even shorter than short. I didn't even notice...
> 
> >> +	return clamp_t(s8, i, MIN_TEMP, MAX_TEMP);  
> > 
> > That is just plain broken.
> > clamp_t() really shouldn't have been allowed to exist.
> > That is a typical example of how it gets misused.
> > (min_t() and max_t() get misused the same way.)
> > 
> > Think what happens when i is 256.
> > The code should just be:
> > 
> > 	return clamp(i, MIN_TEMP, MAX_TEMP);
> > 
> > No casts anywhere.  
> 
> Ok, yeah 256 would be 0 when cast to s8 even though it should be clamped
> to MAX_TEMP. Never thought about this side effect of clamp_t(). Will
> change it to just clamp() in v2, thanks!
> 
> > I'm not even sure the return type (s8) makes any sense.
> > It is quite likely that the code will be better if it is 'int'.
> > The fact that the domain in inside -128..127 doesn't mean that
> > the correct type for a variable isn't 'int'.  
> 
> The low and high temperatures (s8) are only written to the u8 array
> 'new_config_register' for which s8 seems fine. What made you think int
> might be better?

Because 's8' is promoted to 'int' whenever it is used.
And, because cpu registers are all 32/64bit (except on x86 and m68k),
the compiler has to mask the results of any arithmetic assigned to
an 's8' (or u8) local (which you want to be in a register) just in case
the value is out of range and needs the high bits discarding.

Now it might be that the current compilers track the values through
	th = clamp(temp, -128, 127);
so know that only the low 8 bits are significant and the high bits
can be left matching the sign.
But it is more likely to generate:
	reg_containing_th = clamp(temp, -128, 127) & 0xff;
then later when you have 'if (tl > th) ...' the compiler has
to generate code to sign extend both 8bit values to 32bits in order to
do a signed comparison.

So calculate the value as int (or long) and then assign it to the u8 array.

While it can make sense to use u8/s8/u16/s16 to save space in a structure
(the fields get read with either zero-extending or sign-extending memory
reads), using them for locals, function parameters or function return
values is very likely to generate additional instructions.

	David

> 
> Thanks,
> Thorsten
>