[PATCH v3 0/1] hwmon: (asus-ec-sensors) detect unconnected physical sensors

[PATCH v3 0/1] hwmon: (asus-ec-sensors) detect unconnected physical sensors

[Eugene Shalygin]

v3: apply this detection only to environment temperature sensors
(T_Sensor, water, etc.), but not to the VRM and CPU ones.

v2: make temperature_blank_values array const.

Eugene Shalygin (1):
  hwmon: (asus-ec-sensors) detect unconnected physical sensors

 drivers/hwmon/asus-ec-sensors.c | 49 ++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 4 deletions(-)

-- 
2.55.0

[PATCH v3 1/1] hwmon: (asus-ec-sensors) detect unconnected physical sensors

[Eugene Shalygin]

When there is no physical sensor connected to the motherboard
socket,reading EC register returns one of the special values
(-62, -60, -40). Test for them  and return -ENODATA to hwmon.

Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com>
---
 drivers/hwmon/asus-ec-sensors.c | 49 ++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
index 29a23484cbe7..3936006adeb6 100644
--- a/drivers/hwmon/asus-ec-sensors.c
+++ b/drivers/hwmon/asus-ec-sensors.c
@@ -185,6 +185,20 @@ enum ec_sensors {
 #define SENSOR_TEMP_SENSOR_EXTRA_2 BIT(ec_sensor_temp_sensor_extra_2)
 #define SENSOR_TEMP_SENSOR_EXTRA_3 BIT(ec_sensor_temp_sensor_extra_3)
 
+/*
+ * The values for temperature sensor readings without physical sensors connected.
+ * The value varies across generations and is seemingly defined by the EC chip
+ * used in the given board.
+ */
+static const s32 temperature_blank_values[] = {-62, -60, -40};
+
+static const s32 environment_temp_sensors =
+	SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_T_SENSOR_ALT1 |
+	SENSOR_TEMP_WATER_IN | SENSOR_TEMP_WATER_OUT |
+	SENSOR_TEMP_WATER_BLOCK_IN | SENSOR_TEMP_WATER_BLOCK_OUT |
+	SENSOR_TEMP_T_SENSOR_2 | SENSOR_TEMP_SENSOR_EXTRA_1 |
+	SENSOR_TEMP_SENSOR_EXTRA_2 | SENSOR_TEMP_SENSOR_EXTRA_3;
+
 enum board_family {
 	family_unknown,
 	family_amd_400_series,
@@ -955,6 +969,7 @@ static const struct dmi_system_id dmi_table[] = {
 };
 
 struct ec_sensor {
+	/* this is ec_sensors enum value */
 	unsigned int info_index;
 	s32 cached_value;
 };
@@ -1047,6 +1062,12 @@ get_sensor_info(const struct ec_sensors_data *state, int index)
 	return state->sensors_info + state->sensors[index].info_index;
 }
 
+static enum ec_sensors
+get_ec_sensor_type(const struct ec_sensors_data *state, int index)
+{
+	return state->sensors[index].info_index;
+}
+
 static int find_ec_sensor_index(const struct ec_sensors_data *ec,
 				enum hwmon_sensor_types type, int channel)
 {
@@ -1291,6 +1312,17 @@ static int get_cached_value_or_update(const struct device *dev,
 	return 0;
 }
 
+static bool is_blank_temperature_value(s32 value)
+{
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(temperature_blank_values); ++i) {
+		if (value == temperature_blank_values[i])
+			return true;
+	}
+	return false;
+}
+
 /*
  * Now follow the functions that implement the hwmon interface
  */
@@ -1298,6 +1330,8 @@ static int get_cached_value_or_update(const struct device *dev,
 static int asus_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 			      u32 attr, int channel, long *val)
 {
+	const struct ec_sensor_info *sensor_info;
+	enum ec_sensors ec_sensor;
 	int ret;
 	s32 value = 0;
 
@@ -1309,12 +1343,19 @@ static int asus_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 	}
 
 	ret = get_cached_value_or_update(dev, sidx, state, &value);
-	if (!ret) {
-		*val = scale_sensor_value(value,
-					  get_sensor_info(state, sidx)->type);
+	if (ret)
+		return ret;
+
+	sensor_info = get_sensor_info(state, sidx);
+	if (sensor_info->type == hwmon_temp) {
+		ec_sensor = get_ec_sensor_type(state, sidx);
+		if ((environment_temp_sensors & BIT(ec_sensor)) &&
+		    is_blank_temperature_value(value))
+			return -ENODATA;
 	}
+	*val = scale_sensor_value(value, sensor_info->type);
 
-	return ret;
+	return 0;
 }
 
 static int asus_ec_hwmon_read_string(struct device *dev,
-- 
2.55.0

Re: [PATCH v3 1/1] hwmon: (asus-ec-sensors) detect unconnected physical sensors

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the zero-initialized `last_updated` timestamp causes hwmon to report stale 0 values for the first 5 minutes of system uptime.
--

--- Patch [1]: [PATCH v3 1/1] hwmon: (asus-ec-sensors) detect unconnected physical sensors ---
commit 5f0c212609da2bfbdab37b7fbc65fb23b0a4589a
Author: Eugene Shalygin <eugene.shalygin@gmail.com>

hwmon: (asus-ec-sensors) detect unconnected physical sensors

When there is no physical sensor connected to the motherboard socket,
reading the EC register returns one of several special values (-62, -60,
-40). This commit tests for these values and returns -ENODATA to hwmon.

> diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
> index 29a23484cbe7b..3936006adeb6c 100644
> --- a/drivers/hwmon/asus-ec-sensors.c
> +++ b/drivers/hwmon/asus-ec-sensors.c
[ ... ]
> @@ -1309,12 +1343,19 @@ static int asus_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  	}
>  
>  	ret = get_cached_value_or_update(dev, sidx, state, &value);

[Severity: High]
This isn't a bug introduced by this patch, but doesn't this result in stale 0
values being returned for the first 5 minutes of system uptime?

During probe, struct ec_sensors_data is allocated via devm_kzalloc(), which
zero-initializes the last_updated field.

Because the kernel initializes jiffies to a large negative value (typically
-300 * HZ to catch wrap-around bugs), the timestamp check in
get_cached_value_or_update() effectively becomes:

    time_after(negative_val, HZ)

This evaluates to false, as (long)(HZ - negative_val) < 0 is false.

The condition remains false until jiffies wraps around and passes HZ,
which takes approximately 300 seconds. This can cause userspace fan control
daemons to disable cooling, leading to unexpected throttling.

Should last_updated be initialized to jiffies - HZ during allocation to
ensure the first sensor read correctly triggers an update?

> -	if (!ret) {
> -		*val = scale_sensor_value(value,
> -					  get_sensor_info(state, sidx)->type);
> +	if (ret)
> +		return ret;
> +
> +	sensor_info = get_sensor_info(state, sidx);
> +	if (sensor_info->type == hwmon_temp) {
> +		ec_sensor = get_ec_sensor_type(state, sidx);
> +		if ((environment_temp_sensors & BIT(ec_sensor)) &&
> +		    is_blank_temperature_value(value))
> +			return -ENODATA;
>  	}
> +	*val = scale_sensor_value(value, sensor_info->type);
>  
> -	return ret;
> +	return 0;
>  }

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