Re: [PATCH 1/2] hwmon: (macsmc) Fix regressions in Apple Silicon SMC hwmon driver

[Nathan Chancellor <nathan@kernel.org>]

On Thu, Jan 29, 2026 at 09:51:10AM -0800, Guenter Roeck wrote:
> The recently added macsmc-hwmon driver contained several critical
> bugs in its sensor population logic and float conversion routines.
> 
> Specifically:
> - The voltage sensor population loop used the wrong prefix ("volt-"
>   instead of "voltage-") and incorrectly assigned sensors to the
>   temperature sensor array (hwmon->temp.sensors) instead of the
>   voltage sensor array (hwmon->volt.sensors). This would lead to
>   out-of-bounds memory access or data corruption when both temperature
>   and voltage sensors were present.
> - The float conversion in macsmc_hwmon_write_f32() had flawed exponent
>   logic for values >= 2^24 and lacked masking for the mantissa, which
>   could lead to incorrect values being written to the SMC.
> 
> Fix these issues to ensure correct sensor registration and reliable
> manual fan control.
> 
> Confirm that the reported overflow in FIELD_PREP is fixed by declaring
> macsmc_hwmon_write_f32() as __always_inline for a compile test.
> 
> Fixes: 785205fd8139 ("hwmon: Add Apple Silicon SMC hwmon driver")
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Closes: https://lore.kernel.org/linux-hwmon/20260119195817.GA1035354@ax162/
> Cc: James Calligeros <jcalligeros99@gmail.com>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Neal Gompa <neal@gompa.dev>
> Cc: Janne Grunau <j@jannau.net>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Thanks, I build tested this with all affected clang versions and saw no
errors. I cannot say if it is correct from a hardware perspective
though.

Tested-by: Nathan Chancellor <nathan@kernel.org> # build only

> ---
>  drivers/hwmon/macsmc-hwmon.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwmon/macsmc-hwmon.c b/drivers/hwmon/macsmc-hwmon.c
> index 1c0bbec7e8eb..40d25c81b443 100644
> --- a/drivers/hwmon/macsmc-hwmon.c
> +++ b/drivers/hwmon/macsmc-hwmon.c
> @@ -228,25 +228,22 @@ static int macsmc_hwmon_write_f32(struct apple_smc *smc, smc_key key, int value)
>  {
>  	u64 val;
>  	u32 fval = 0;
> -	int exp = 0, neg;
> +	int exp, neg;
>  
> +	neg = value < 0;
>  	val = abs(value);
> -	neg = val != value;
>  
>  	if (val) {
> -		int msb = __fls(val) - exp;
> +		exp = __fls(val);
>  
> -		if (msb > 23) {
> -			val >>= msb - FLT_MANT_BIAS;
> -			exp -= msb - FLT_MANT_BIAS;
> -		} else if (msb < 23) {
> -			val <<= FLT_MANT_BIAS - msb;
> -			exp += msb;
> -		}
> +		if (exp > 23)
> +			val >>= exp - 23;
> +		else
> +			val <<= 23 - exp;
>  
>  		fval = FIELD_PREP(FLT_SIGN_MASK, neg) |
>  		       FIELD_PREP(FLT_EXP_MASK, exp + FLT_EXP_BIAS) |
> -		       FIELD_PREP(FLT_MANT_MASK, val);
> +		       FIELD_PREP(FLT_MANT_MASK, val & FLT_MANT_MASK);
>  	}
>  
>  	return apple_smc_write_u32(smc, key, fval);
> @@ -663,8 +660,8 @@ static int macsmc_hwmon_populate_sensors(struct macsmc_hwmon *hwmon,
>  		if (!hwmon->volt.sensors)
>  			return -ENOMEM;
>  
> -		for_each_child_of_node_with_prefix(hwmon_node, key_node, "volt-") {
> -			sensor = &hwmon->temp.sensors[hwmon->temp.count];
> +		for_each_child_of_node_with_prefix(hwmon_node, key_node, "voltage-") {
> +			sensor = &hwmon->volt.sensors[hwmon->volt.count];
>  			if (!macsmc_hwmon_create_sensor(hwmon->dev, hwmon->smc, key_node, sensor)) {
>  				sensor->attrs = HWMON_I_INPUT;
>  
> -- 
> 2.45.2
>