Re: [PATCH 1/2] hwmon: (asus-ec-sensors) add T_Sensor for ROG STRIX X870E-E GAMING WIFI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The patch enables a sensor that returns a specific 'blank' value (-62) without filtering it, resulting in a bogus temperature being reported to userspace.

Pre-existing issues:
- [High] Missing bounds check in `asus_ec_block_read()` causes unintended reads of EC registers from the wrong bank.
--

--- Patch [1]: [PATCH 1/2] hwmon: (asus-ec-sensors) add T_Sensor for ROG STRIX X870E-E GAMING WIFI ---
commit 65789a63d90e6f671eaa090793abd6eb2ac05064
Author: Vishaal Saraiya <vishaal.saraiya@gmail.com>

hwmon: (asus-ec-sensors) add T_Sensor for ROG STRIX X870E-E GAMING WIFI

Enable T_Sensor for ROG STRIX X870E-E GAMING WIFI. Board specs list the
sensor, reading return -62, which is one of the blank values for
temperature sensors in the ASUS EC.

> diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
> index 29a23484cbe7b..742520c485ca6 100644
> --- a/drivers/hwmon/asus-ec-sensors.c
> +++ b/drivers/hwmon/asus-ec-sensors.c
> @@ -743,7 +743,7 @@ static const struct ec_board_info board_info_strix_x870_i_gaming_wifi = {
>  
>  static const struct ec_board_info board_info_strix_x870e_e_gaming_wifi = {
>  	.sensors = SENSOR_TEMP_CPU | SENSOR_TEMP_CPU_PACKAGE |
> -		SENSOR_TEMP_MB | SENSOR_TEMP_VRM |
> +		SENSOR_TEMP_MB | SENSOR_TEMP_VRM | SENSOR_TEMP_T_SENSOR |

[Severity: Medium]
Since the commit message notes that an unpopulated T_Sensor returns a blank
value of -62, will this regression result in reporting a bogus extreme
negative temperature (-62000 mC) to userspace?

Looking at asus_ec_hwmon_read(), the driver does not appear to filter out
this known blank value before scaling and returning it:

	ret = get_cached_value_or_update(dev, sidx, state, &value);
	if (!ret) {
		*val = scale_sensor_value(value,
					  get_sensor_info(state, sidx)->type);
	}

Could this bogus value confuse thermal daemons and cause them to incorrectly
spin down fans?

>  		SENSOR_FAN_CPU_OPT,
>  	.mutex_path = ASUS_HW_ACCESS_MUTEX_SB_PCI0_SBRG_SIO1_MUT0,
>  	.family = family_amd_800_series,

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential missing
bounds check in asus_ec_block_read() that might cause unintended reads of EC
registers from the wrong bank:

drivers/hwmon/asus-ec-sensors.c:asus_ec_block_read() {
    ...
	reg_bank = register_bank(ec->registers[ireg]);
	if (reg_bank < bank) {
		continue;
	}
	ec_read(register_index(ec->registers[ireg]),
		ec->read_buffer + ireg);
    ...
}

Because the loop only skips registers where reg_bank < bank, will this issue
reads for registers belonging to higher banks while the EC is still switched
to a lower bank?

Could this read incorrect hardware registers and potentially cause hardware
misbehavior due to side-effects, as well as trigger redundant, slow ACPI
reads?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709204037.1998433-1-eugene.shalygin@gmail.com?part=1