[PATCH RFT 0/5] hwmon: Introduce hwmon_update_groups()

[PATCH RFT 0/5] hwmon: Introduce hwmon_update_groups()

[Guenter Roeck]

Introduce and use hwmon_update_groups() to support changing sysfs
attribute visibility dynamically without having to re-register
affected hardware monitoring devices.

Use the new function in the ACPI power meter driver.

RFT: The new function was tested extensively with a dummy driver
which changes attribute visibility every few seconds. However,
it was not tested in the 'real world'.

----------------------------------------------------------------
Guenter Roeck (5):
      hwmon: Handle attribute visibility evaluation in device core
      hwmon: Provide helper function to find thermal zones
      hwmon: Add support for updating thermal zones
      hwmon: Implement hwmon_update_groups()
      hwmon: (acpi_power_meter) Use hwmon_update_groups() to update sensor visibility

 Documentation/hwmon/hwmon-kernel-api.rst |   8 +++
 drivers/hwmon/acpi_power_meter.c         |  37 ++++-------
 drivers/hwmon/hwmon.c                    | 108 ++++++++++++++++++++++++-------
 include/linux/hwmon.h                    |   2 +
 4 files changed, 109 insertions(+), 46 deletions(-)

[PATCH RFT 1/5] hwmon: Handle attribute visibility evaluation in device core

[Guenter Roeck]

In preparation for supporting sysfs attribute updates, move
attribute visibility evaluation into device core.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/hwmon.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 0b4bdcd33c7b..9e9ad42b6d7d 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -523,10 +523,12 @@ static struct attribute *hwmon_genattr(const void *drvdata,
 	const char *name;
 	bool is_string = is_string_attr(type, attr);
 
+	/*
+	 * Basic mode sanity check. This is less than perfect since
+	 * attribute visibility and with it the mode can change during
+	 * runtime, but it is the best we can do.
+	 */
 	mode = hwmon_is_visible(ops, drvdata, type, attr, index);
-	if (!mode)
-		return ERR_PTR(-ENOENT);
-
 	if ((mode & 0444) && ((is_string && !ops->read_string) ||
 				 (!is_string && !ops->read)))
 		return ERR_PTR(-EINVAL);
@@ -557,7 +559,7 @@ static struct attribute *hwmon_genattr(const void *drvdata,
 	a = &dattr->attr;
 	sysfs_attr_init(a);
 	a->name = name;
-	a->mode = mode;
+	a->mode = ops->write ? 0644 : 0444;	/* updated when attributes are generated */
 
 	return a;
 }
@@ -896,6 +898,17 @@ __hwmon_create_attrs(const void *drvdata, const struct hwmon_chip_info *chip)
 	return attrs;
 }
 
+static umode_t hwmon_kobj_is_visible(struct kobject *kobj, struct attribute *attr, int n)
+{
+	struct device_attribute *dattr = to_dev_attr(attr);
+	struct hwmon_device_attribute *hattr = to_hwmon_attr(dattr);
+	struct device *dev = kobj_to_dev(kobj);
+	void *drvdata = dev_get_drvdata(dev);
+
+	return hwmon_is_visible(hattr->ops, drvdata, hattr->type, hattr->attr,
+				hattr->index);
+}
+
 static struct device *
 __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
 			const struct hwmon_chip_info *chip,
@@ -946,6 +959,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
 		}
 
 		hwdev->group.attrs = attrs;
+		hwdev->group.is_visible = hwmon_kobj_is_visible;
 		ngroups = 0;
 		hwdev->groups[ngroups++] = &hwdev->group;
 
-- 
2.45.2

[PATCH RFT 2/5] hwmon: Provide helper function to find thermal zones

[Guenter Roeck]

Provide a helper function to find registered thermal zones
and use it in the thermal notification function.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/hwmon.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 9e9ad42b6d7d..1f35285ca7a0 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -261,6 +261,21 @@ static int hwmon_thermal_add_sensor(struct device *dev, int index)
 	return 0;
 }
 
+static struct hwmon_thermal_data *hwmon_thermal_find_tz(struct device *dev, int index)
+{
+	struct hwmon_device *hwdev = to_hwmon_device(dev);
+	struct hwmon_thermal_data *tzdata;
+
+	if (!IS_ENABLED(CONFIG_THERMAL_OF))
+		return NULL;
+
+	list_for_each_entry(tzdata, &hwdev->tzdata, node) {
+		if (tzdata->index == index)
+			return tzdata;
+	}
+	return NULL;
+}
+
 static int hwmon_thermal_register_sensors(struct device *dev)
 {
 	struct hwmon_device *hwdev = to_hwmon_device(dev);
@@ -297,18 +312,10 @@ static int hwmon_thermal_register_sensors(struct device *dev)
 
 static void hwmon_thermal_notify(struct device *dev, int index)
 {
-	struct hwmon_device *hwdev = to_hwmon_device(dev);
-	struct hwmon_thermal_data *tzdata;
+	struct hwmon_thermal_data *tzdata = hwmon_thermal_find_tz(dev, index);
 
-	if (!IS_ENABLED(CONFIG_THERMAL_OF))
-		return;
-
-	list_for_each_entry(tzdata, &hwdev->tzdata, node) {
-		if (tzdata->index == index) {
-			thermal_zone_device_update(tzdata->tzd,
-						   THERMAL_EVENT_UNSPECIFIED);
-		}
-	}
+	if (tzdata)
+		thermal_zone_device_update(tzdata->tzd, THERMAL_EVENT_UNSPECIFIED);
 }
 
 static int hwmon_attr_base(enum hwmon_sensor_types type)
-- 
2.45.2

[PATCH RFT 3/5] hwmon: Add support for updating thermal zones

[Guenter Roeck]

Implement support for updating thermal zones. This is necessary
to be able to handle updates to sysfs attribute visibility.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/hwmon.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 1f35285ca7a0..cb89218a0b6a 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -276,7 +276,7 @@ static struct hwmon_thermal_data *hwmon_thermal_find_tz(struct device *dev, int
 	return NULL;
 }
 
-static int hwmon_thermal_register_sensors(struct device *dev)
+static int hwmon_thermal_handle_sensors(struct device *dev, bool update)
 {
 	struct hwmon_device *hwdev = to_hwmon_device(dev);
 	const struct hwmon_chip_info *chip = hwdev->chip;
@@ -294,22 +294,42 @@ static int hwmon_thermal_register_sensors(struct device *dev)
 			continue;
 
 		for (j = 0; info[i]->config[j]; j++) {
+			umode_t mode;
 			int err;
 
-			if (!(info[i]->config[j] & HWMON_T_INPUT) ||
-			    !hwmon_is_visible(chip->ops, drvdata, hwmon_temp,
-					      hwmon_temp_input, j))
+			if (!(info[i]->config[j] & HWMON_T_INPUT))
 				continue;
+			mode = hwmon_is_visible(chip->ops, drvdata, hwmon_temp,
+						hwmon_temp_input, j);
+			if (!mode) {
+				struct hwmon_thermal_data *tzdata;
 
-			err = hwmon_thermal_add_sensor(dev, j);
-			if (err)
-				return err;
+				if (!update)
+					continue;
+				tzdata = hwmon_thermal_find_tz(dev, j);
+				if (tzdata) {
+					devm_thermal_of_zone_unregister(dev, tzdata->tzd);
+					devm_release_action(dev, hwmon_thermal_remove_sensor,
+							    &tzdata->node);
+				}
+			} else {
+				if (!update || !hwmon_thermal_find_tz(dev, j)) {
+					err = hwmon_thermal_add_sensor(dev, j);
+					if (err)
+						return err;
+				}
+			}
 		}
 	}
 
 	return 0;
 }
 
+static int hwmon_thermal_register_sensors(struct device *dev)
+{
+	return hwmon_thermal_handle_sensors(dev, false);
+}
+
 static void hwmon_thermal_notify(struct device *dev, int index)
 {
 	struct hwmon_thermal_data *tzdata = hwmon_thermal_find_tz(dev, index);
-- 
2.45.2

Re: [PATCH RFT 3/5] hwmon: Add support for updating thermal zones

[Guenter Roeck]

On 1/23/26 10:22, Guenter Roeck wrote:
> Implement support for updating thermal zones. This is necessary
> to be able to handle updates to sysfs attribute visibility.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

This patch is both racy because it doesn't protect the list of thermal
zones, and it leaks tzdata until the device is removed. SO it will need
(much) more work.

Guenter

> ---
>   drivers/hwmon/hwmon.c | 34 +++++++++++++++++++++++++++-------
>   1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 1f35285ca7a0..cb89218a0b6a 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -276,7 +276,7 @@ static struct hwmon_thermal_data *hwmon_thermal_find_tz(struct device *dev, int
>   	return NULL;
>   }
>   
> -static int hwmon_thermal_register_sensors(struct device *dev)
> +static int hwmon_thermal_handle_sensors(struct device *dev, bool update)
>   {
>   	struct hwmon_device *hwdev = to_hwmon_device(dev);
>   	const struct hwmon_chip_info *chip = hwdev->chip;
> @@ -294,22 +294,42 @@ static int hwmon_thermal_register_sensors(struct device *dev)
>   			continue;
>   
>   		for (j = 0; info[i]->config[j]; j++) {
> +			umode_t mode;
>   			int err;
>   
> -			if (!(info[i]->config[j] & HWMON_T_INPUT) ||
> -			    !hwmon_is_visible(chip->ops, drvdata, hwmon_temp,
> -					      hwmon_temp_input, j))
> +			if (!(info[i]->config[j] & HWMON_T_INPUT))
>   				continue;
> +			mode = hwmon_is_visible(chip->ops, drvdata, hwmon_temp,
> +						hwmon_temp_input, j);
> +			if (!mode) {
> +				struct hwmon_thermal_data *tzdata;
>   
> -			err = hwmon_thermal_add_sensor(dev, j);
> -			if (err)
> -				return err;
> +				if (!update)
> +					continue;
> +				tzdata = hwmon_thermal_find_tz(dev, j);
> +				if (tzdata) {
> +					devm_thermal_of_zone_unregister(dev, tzdata->tzd);
> +					devm_release_action(dev, hwmon_thermal_remove_sensor,
> +							    &tzdata->node);
> +				}
> +			} else {
> +				if (!update || !hwmon_thermal_find_tz(dev, j)) {
> +					err = hwmon_thermal_add_sensor(dev, j);
> +					if (err)
> +						return err;
> +				}
> +			}
>   		}
>   	}
>   
>   	return 0;
>   }
>   
> +static int hwmon_thermal_register_sensors(struct device *dev)
> +{
> +	return hwmon_thermal_handle_sensors(dev, false);
> +}
> +
>   static void hwmon_thermal_notify(struct device *dev, int index)
>   {
>   	struct hwmon_thermal_data *tzdata = hwmon_thermal_find_tz(dev, index);

[PATCH RFT 4/5] hwmon: Implement hwmon_update_groups()

[Guenter Roeck]

In some situations the visibility of hwmon sysfs attributes may change.
Support this by providing a new API function hwmon_update_groups()
to update both visible attributes and thermal zones.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 Documentation/hwmon/hwmon-kernel-api.rst |  8 ++++++++
 drivers/hwmon/hwmon.c                    | 24 ++++++++++++++++++++++++
 include/linux/hwmon.h                    |  2 ++
 3 files changed, 34 insertions(+)

diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst
index 1d7f1397a827..a41b1038fbf0 100644
--- a/Documentation/hwmon/hwmon-kernel-api.rst
+++ b/Documentation/hwmon/hwmon-kernel-api.rst
@@ -42,6 +42,8 @@ register/unregister functions::
 
   char *devm_hwmon_sanitize_name(struct device *dev, const char *name);
 
+  int hwmon_update_groups(struct device *dev);
+
   void hwmon_lock(struct device *dev);
   void hwmon_unlock(struct device *dev);
 
@@ -89,6 +91,12 @@ for other functions such as interrupt handlers or for attributes which are
 fully implemented in the driver, hwmon_lock() and hwmon_unlock() can be used
 to ensure that calls to those functions are serialized.
 
+If the visibility of sysfs attributes changes during runtime, the driver
+needs to call hwmon_update_groups() with the hwmon device as parameter
+to update attribute visibility. If the driver registered thermal zones
+using hwmon_device_register_with_info() and the visibility of thermal
+sensors changes, this call will also update thermal zones as needed.
+
 Using devm_hwmon_device_register_with_info()
 --------------------------------------------
 
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index cb89218a0b6a..9163b8290dbe 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -330,6 +330,11 @@ static int hwmon_thermal_register_sensors(struct device *dev)
 	return hwmon_thermal_handle_sensors(dev, false);
 }
 
+static int hwmon_thermal_update_sensors(struct device *dev)
+{
+	return hwmon_thermal_handle_sensors(dev, true);
+}
+
 static void hwmon_thermal_notify(struct device *dev, int index)
 {
 	struct hwmon_thermal_data *tzdata = hwmon_thermal_find_tz(dev, index);
@@ -799,6 +804,25 @@ static const int __templates_size[] = {
 	[hwmon_intrusion] = ARRAY_SIZE(hwmon_intrusion_attr_templates),
 };
 
+int hwmon_update_groups(struct device *dev)
+{
+	struct hwmon_device *hwdev = to_hwmon_device(dev);
+	const struct hwmon_chip_info *chip = hwdev->chip;
+	const struct hwmon_channel_info * const *info;
+	int ret;
+
+	ret = sysfs_update_groups(&dev->kobj, dev->groups);
+	if (ret || !chip)
+		return ret;
+
+	info = chip->info;
+	if (info[0]->type != hwmon_chip || !(info[0]->config[0] & HWMON_C_REGISTER_TZ))
+		return 0;
+
+	return hwmon_thermal_update_sensors(dev);
+}
+EXPORT_SYMBOL_GPL(hwmon_update_groups);
+
 int hwmon_notify_event(struct device *dev, enum hwmon_sensor_types type,
 		       u32 attr, int channel)
 {
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 301a83afbd66..8cadba24ed4b 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -492,6 +492,8 @@ int hwmon_notify_event(struct device *dev, enum hwmon_sensor_types type,
 char *hwmon_sanitize_name(const char *name);
 char *devm_hwmon_sanitize_name(struct device *dev, const char *name);
 
+int hwmon_update_groups(struct device *dev);
+
 void hwmon_lock(struct device *dev);
 void hwmon_unlock(struct device *dev);
 
-- 
2.45.2

[PATCH RFT 5/5] hwmon: (acpi_power_meter) Use hwmon_update_groups() to update sensor visibility

[Guenter Roeck]

If the driver is notified about hardware a configuration change, the driver
unregisters the hardware monitoring device and registers it again. This is
conceptually wrong and can have unintended side effects, especially if a
userspace application is in the process of reading attributes during that
time.

If the hardware configuration changed, call hwmon_update_groups() instead
to update attribute visibility. Update driver locking to use the hardware
monitoring lock for all locking operations and drop the driver internal
lock.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/acpi_power_meter.c | 37 +++++++++++---------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
index 29ccdc2fb7ff..59b56217e856 100644
--- a/drivers/hwmon/acpi_power_meter.c
+++ b/drivers/hwmon/acpi_power_meter.c
@@ -75,7 +75,6 @@ struct acpi_power_meter_capabilities {
 struct acpi_power_meter_resource {
 	struct acpi_device	*acpi_dev;
 	acpi_bus_id		name;
-	struct mutex		lock;
 	struct device		*hwmon_dev;
 	struct acpi_power_meter_capabilities	caps;
 	acpi_string		model_number;
@@ -445,8 +444,6 @@ static int power_meter_read(struct device *dev, enum hwmon_sensor_types type,
 	if (type != hwmon_power)
 		return -EINVAL;
 
-	guard(mutex)(&res->lock);
-
 	switch (attr) {
 	case hwmon_power_average:
 		ret = update_meter(res);
@@ -501,7 +498,6 @@ static int power_meter_write(struct device *dev, enum hwmon_sensor_types type,
 	if (type != hwmon_power)
 		return -EINVAL;
 
-	guard(mutex)(&res->lock);
 	switch (attr) {
 	case hwmon_power_cap:
 		ret = set_cap(res, val);
@@ -547,9 +543,9 @@ static ssize_t power1_average_max_store(struct device *dev,
 	if (ret)
 		return ret;
 
-	mutex_lock(&res->lock);
+	hwmon_lock(res->hwmon_dev);
 	ret = set_trip(res, POWER_METER_TRIP_AVERAGE_MAX_IDX, trip);
-	mutex_unlock(&res->lock);
+	hwmon_unlock(res->hwmon_dev);
 
 	return ret == 0 ? count : ret;
 }
@@ -566,9 +562,9 @@ static ssize_t power1_average_min_store(struct device *dev,
 	if (ret)
 		return ret;
 
-	mutex_lock(&res->lock);
+	hwmon_lock(res->hwmon_dev);
 	ret = set_trip(res, POWER_METER_TRIP_AVERAGE_MIN_IDX, trip);
-	mutex_unlock(&res->lock);
+	hwmon_unlock(res->hwmon_dev);
 
 	return ret == 0 ? count : ret;
 }
@@ -825,44 +821,38 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
 
 	switch (event) {
 	case METER_NOTIFY_CONFIG:
-		mutex_lock(&resource->lock);
+		hwmon_lock(resource->hwmon_dev);
 		free_capabilities(resource);
 		remove_domain_devices(resource);
-		hwmon_device_unregister(resource->hwmon_dev);
 		res = read_capabilities(resource);
 		if (res)
 			dev_err_once(&device->dev, "read capabilities failed.\n");
 		res = read_domain_devices(resource);
 		if (res && res != -ENODEV)
 			dev_err_once(&device->dev, "read domain devices failed.\n");
-		resource->hwmon_dev =
-			hwmon_device_register_with_info(&device->dev,
-							ACPI_POWER_METER_NAME,
-							resource,
-							&power_meter_chip_info,
-							power_extra_groups);
-		if (IS_ERR(resource->hwmon_dev))
-			dev_err_once(&device->dev, "register hwmon device failed.\n");
-		mutex_unlock(&resource->lock);
+		res = hwmon_update_groups(resource->hwmon_dev);
+		if (res)
+			dev_err_once(&device->dev, "Failed to update hardware monitoring data\n");
+		hwmon_unlock(resource->hwmon_dev);
 		break;
 	case METER_NOTIFY_TRIP:
 		sysfs_notify(&device->dev.kobj, NULL, POWER_AVERAGE_NAME);
 		break;
 	case METER_NOTIFY_CAP:
-		mutex_lock(&resource->lock);
+		hwmon_lock(resource->hwmon_dev);
 		res = update_cap(resource);
 		if (res)
 			dev_err_once(&device->dev, "update cap failed when capping value is changed.\n");
-		mutex_unlock(&resource->lock);
+		hwmon_unlock(resource->hwmon_dev);
 		sysfs_notify(&device->dev.kobj, NULL, POWER_CAP_NAME);
 		break;
 	case METER_NOTIFY_INTERVAL:
 		sysfs_notify(&device->dev.kobj, NULL, POWER_AVG_INTERVAL_NAME);
 		break;
 	case METER_NOTIFY_CAPPING:
-		mutex_lock(&resource->lock);
+		hwmon_lock(resource->hwmon_dev);
 		resource->power_alarm = true;
-		mutex_unlock(&resource->lock);
+		hwmon_unlock(resource->hwmon_dev);
 		sysfs_notify(&device->dev.kobj, NULL, POWER_ALARM_NAME);
 		dev_info(&device->dev, "Capping in progress.\n");
 		break;
@@ -889,7 +879,6 @@ static int acpi_power_meter_add(struct acpi_device *device)
 
 	resource->sensors_valid = 0;
 	resource->acpi_dev = device;
-	mutex_init(&resource->lock);
 	strcpy(acpi_device_name(device), ACPI_POWER_METER_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_POWER_METER_CLASS);
 	device->driver_data = resource;
-- 
2.45.2