1. Patchew
  2. linux
  3. View series

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

Thorsten Blum posted 1 patch 1 month, 1 week ago 
drivers/w1/slaves/w1_therm.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
[PATCH] w1: therm: Use clamp_t to simplify int_to_short helper
Posted by Thorsten Blum 1 month, 1 week ago 
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.

Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 drivers/w1/slaves/w1_therm.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index 9ccedb3264fb..cf686e6ba3d5 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -961,9 +961,8 @@ static inline int temperature_from_RAM(struct w1_slave *sl, u8 rom[9])
  */
 static inline s8 int_to_short(int i)
 {
-	/* Prepare to cast to short by eliminating out of range values */
-	i = clamp(i, MIN_TEMP, MAX_TEMP);
-	return (s8) i;
+	/* Cast to short by eliminating out of range values */
+	return clamp_t(s8, i, MIN_TEMP, MAX_TEMP);
 }
 
 /* Interface Functions */
@@ -1702,7 +1701,7 @@ static ssize_t temperature_show(struct device *device,
 		return 0;
 	}
 
-	return sprintf(buf, "%d\n", temperature_from_RAM(sl, info.rom));
+	return sysfs_emit(buf, "%d\n", temperature_from_RAM(sl, info.rom));
 }
 
 static ssize_t ext_power_show(struct device *device,
@@ -1724,7 +1723,7 @@ static ssize_t ext_power_show(struct device *device,
 			"%s: Power_mode may be corrupted. err=%d\n",
 			__func__, SLAVE_POWERMODE(sl));
 	}
-	return sprintf(buf, "%d\n", SLAVE_POWERMODE(sl));
+	return sysfs_emit(buf, "%d\n", SLAVE_POWERMODE(sl));
 }
 
 static ssize_t resolution_show(struct device *device,
@@ -1746,7 +1745,7 @@ static ssize_t resolution_show(struct device *device,
 			__func__, SLAVE_RESOLUTION(sl));
 	}
 
-	return sprintf(buf, "%d\n", SLAVE_RESOLUTION(sl));
+	return sysfs_emit(buf, "%d\n", SLAVE_RESOLUTION(sl));
 }
 
 static ssize_t resolution_store(struct device *device,
@@ -1827,7 +1826,7 @@ static ssize_t alarms_show(struct device *device,
 			__func__, ret);
 	}
 
-	return sprintf(buf, "%hd %hd\n", tl, th);
+	return sysfs_emit(buf, "%hd %hd\n", tl, th);
 }
 
 static ssize_t alarms_store(struct device *device,
@@ -1969,7 +1968,7 @@ static ssize_t therm_bulk_read_show(struct device *device,
 		}
 	}
 show_result:
-	return sprintf(buf, "%d\n", ret);
+	return sysfs_emit(buf, "%d\n", ret);
 }
 
 static ssize_t conv_time_show(struct device *device,
@@ -1982,7 +1981,7 @@ static ssize_t conv_time_show(struct device *device,
 			"%s: Device is not supported by the driver\n", __func__);
 		return 0;  /* No device family */
 	}
-	return sprintf(buf, "%d\n", conversion_time(sl));
+	return sysfs_emit(buf, "%d\n", conversion_time(sl));
 }
 
 static ssize_t conv_time_store(struct device *device,
@@ -2024,7 +2023,7 @@ static ssize_t features_show(struct device *device,
 			 "%s: Device not supported by the driver\n", __func__);
 		return 0;  /* No device family */
 	}
-	return sprintf(buf, "%u\n", SLAVE_FEATURES(sl));
+	return sysfs_emit(buf, "%u\n", SLAVE_FEATURES(sl));
 }
 
 static ssize_t features_store(struct device *device,
-- 
2.51.1
Re: [PATCH] w1: therm: Use clamp_t to simplify int_to_short helper
Posted by David Laight 1 month, 1 week ago 
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.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  drivers/w1/slaves/w1_therm.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
> index 9ccedb3264fb..cf686e6ba3d5 100644
> --- a/drivers/w1/slaves/w1_therm.c
> +++ b/drivers/w1/slaves/w1_therm.c
> @@ -961,9 +961,8 @@ static inline int temperature_from_RAM(struct w1_slave *sl, u8 rom[9])
>   */
>  static inline s8 int_to_short(int i)
>  {
> -	/* Prepare to cast to short by eliminating out of range values */
> -	i = clamp(i, MIN_TEMP, MAX_TEMP);
> -	return (s8) i;
> +	/* Cast to short by eliminating out of range values */
                   ^^^^^ no shorts here...
> +	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.
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'.

	David

>  }
Re: [PATCH] w1: therm: Use clamp_t to simplify int_to_short helper
Posted by Thorsten Blum 1 month, 1 week ago 
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?

Thanks,
Thorsten
Re: [PATCH] w1: therm: Use clamp_t to simplify int_to_short helper
Posted by David Laight 1 month, 1 week ago 
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
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.