Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info

[Guenter Roeck <linux@roeck-us.net>]

[-- Attachment #1: Type: text/plain, Size: 4495 bytes --]

On 5/16/22 05:21, Guenter Roeck wrote:
> On 5/15/22 23:21, LABBE Corentin wrote:
>> Le Sun, May 15, 2022 at 05:29:54PM -0700, Guenter Roeck a écrit :
>>> On 5/15/22 12:36, LABBE Corentin wrote:
>>>> Le Wed, May 11, 2022 at 07:10:29PM -0700, Guenter Roeck a écrit :
>>>>> Corentin,
>>>>>
>>>>> On 5/8/22 23:30, Corentin Labbe wrote:
>>>>>> Booting lead to a hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
>>>>>> So let's convert the driver to use hwmon_device_register_with_info().
>>>>>>
>>>>>> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
>>>>>> ---
>>>>> [ ... ]
>>>>>
>>>>>> @@ -836,20 +740,20 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>>>>>>             if (res)
>>>>>>                 break;
>>>>>> -        remove_attrs(resource);
>>>>>> +        remove_domain_devices(resource);
>>>>>>             setup_attrs(resource);
>>>>>
>>>>> Zhang Rui found an interesting problem with this code:
>>>>> It needs a call to sysfs_update_groups(hwmon_dev->groups)
>>>>> to update sysfs attribute visibility, probably between
>>>>> remove_domain_devices() and setup_attrs().
>>>>>
>>>>>>             break;
>>>>>>         case METER_NOTIFY_TRIP:
>>>>>> -        sysfs_notify(&device->dev.kobj, NULL, POWER_AVERAGE_NAME);
>>>>>> +        hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average, 0);
>>>>>
>>>>> ... which makes realize: The notification device should be the hwmon device.
>>>>> That would be resource->hwmon_dev, not the acpi device.
>>>>>
>>>>
>>>> Hello
>>>>
>>>> Since my hardware lacks capabilities testing this, I have emulated it on qemu:
>>>> https://github.com/montjoie/qemu/commit/320f2ddacb954ab308ef699f66fca6313f75bc2b
>>>>
>>>> I have added a custom ACPI _DBX method for triggering some ACPI state change. (like config change, like enabling CAP).
>>>>
>>>> For testing config change I have tried lot of way:
>>>>                   res = read_capabilities(resource);
>>>> @@ -742,18 +758,22 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>>>>                   remove_domain_devices(resource);
>>>>                   setup_attrs(resource);
>>>> +               res = sysfs_update_groups(&resource->hwmon_dev->kobj, acpi_power_groups);
>>>> +               res = sysfs_update_groups(&resource->acpi_dev->dev.kobj, acpi_power_groups);
>>>> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_cap, 0);
>>>> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average, 0);
>>>
>>> Did you add a debug log here ?
>>
>> Yes I added debug log to check what is called.
>>
>>>
>>> acpi_power_groups would be the wrong parameter for sysfs_update_groups().
>>> It would have to be resource->hwmon_dev->groups.
>>>
>>
>> Even with that, no call to is_visible:
>> @@ -742,18 +758,22 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>>                  remove_domain_devices(resource);
>>                  setup_attrs(resource);
>> +               res = sysfs_update_groups(&resource->hwmon_dev->kobj, resource->hwmon_dev->groups);
>> +               res = sysfs_update_groups(&resource->acpi_dev->dev.kobj, resource->hwmon_dev->groups);
>> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_cap, 0);
>> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average, 0);
>>                  break;
>>
>> I checked drivers/hwmon/hwmon.c is seems that is_visible is only called by gen_attr/gen_attrs which is only called by __hwmon_create_attrs and then by registers functions.
>> So perhaps it explain why it is never called.
> 
> Ah yes, you are correct. Sorry, it has been too long ago that I wrote that code.
> Effectively that means we'll have to rework the hwmon core to generate attributes
> anyway and leave it up to the driver core to call the is_visible function.
> 

Attached is an outline of what would be needed in the hwmon core.
Completely untested. I wonder if it may be easier to always
create all attributes and have them return -ENODATA if not
supported.

Guenter

[-- Attachment #2: 0001-hwmon-Implement-hwmon_update_groups.patch --]
[-- Type: text/x-patch, Size: 6332 bytes --]

From 0ff1a316c0f0ca36c49f2415f41d49ef1581808d Mon Sep 17 00:00:00 2001
From: Guenter Roeck <linux@roeck-us.net>
Date: Fri, 20 May 2022 21:54:48 -0700
Subject: [PATCH] hwmon: Implement hwmon_update_groups()

In some situations the visibility of hwmon sysfs attributes may change.
Support this by providing a new API function hwmon_update_groups().

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/hwmon.c | 95 +++++++++++++++++++++++++++++++++----------
 include/linux/hwmon.h |  2 +
 2 files changed, 76 insertions(+), 21 deletions(-)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 2e2cd79d89eb..d842ab607ccf 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -248,7 +248,19 @@ static int hwmon_thermal_add_sensor(struct device *dev, int index)
 	return 0;
 }
 
-static int hwmon_thermal_register_sensors(struct device *dev)
+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;
+
+	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, bool update)
 {
 	struct hwmon_device *hwdev = to_hwmon_device(dev);
 	const struct hwmon_chip_info *chip = hwdev->chip;
@@ -263,16 +275,30 @@ 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) ||
-			    !chip->ops->is_visible(drvdata, hwmon_temp,
-						   hwmon_temp_input, j))
+			if (!(info[i]->config[j] & HWMON_T_INPUT))
 				continue;
-
-			err = hwmon_thermal_add_sensor(dev, j);
-			if (err)
-				return err;
+			mode = chip->ops->is_visible(drvdata, hwmon_temp,
+						     hwmon_temp_input, j);
+			if (!mode) {
+				struct hwmon_thermal_data *tzdata;
+
+				if (!update)
+					continue;
+				tzdata = hwmon_thermal_find_tz(dev, j);
+				if (tzdata) {
+					devm_thermal_zone_of_sensor_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;
+				}
+			}
 		}
 	}
 
@@ -281,15 +307,11 @@ 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);
 
-	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);
 }
 
 #else
@@ -401,10 +423,12 @@ static struct attribute *hwmon_genattr(const void *drvdata,
 	if (!template)
 		return ERR_PTR(-ENOENT);
 
+	/*
+	 * 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 = ops->is_visible(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);
@@ -435,7 +459,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;
 }
@@ -638,6 +662,24 @@ 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 **info = chip->info;
+	int ret;
+
+	ret = sysfs_update_groups(&dev->kobj, dev->groups);
+	if (ret)
+		return ret;
+
+	if (info[0]->type != hwmon_chip || !(info[0]->config[0] & HWMON_C_REGISTER_TZ))
+		return 0;
+
+	return hwmon_thermal_register_sensors(dev, true);
+}
+EXPORT_SYMBOL_GPL(hwmon_update_groups);
+
 int hwmon_notify_event(struct device *dev, enum hwmon_sensor_types type,
 		       u32 attr, int channel)
 {
@@ -748,6 +790,16 @@ __hwmon_create_attrs(const void *drvdata, const struct hwmon_chip_info *chip)
 	return attrs;
 }
 
+static umode_t hwmon_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 hattr->ops->is_visible(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,
@@ -797,6 +849,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
 		}
 
 		hwdev->group.attrs = attrs;
+		hwdev->group.is_visible = hwmon_is_visible;
 		ngroups = 0;
 		hwdev->groups[ngroups++] = &hwdev->group;
 
@@ -840,7 +893,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
 	if (dev && dev->of_node && chip && chip->ops->read &&
 	    chip->info[0]->type == hwmon_chip &&
 	    (chip->info[0]->config[0] & HWMON_C_REGISTER_TZ)) {
-		err = hwmon_thermal_register_sensors(hdev);
+		err = hwmon_thermal_register_sensors(hdev, false);
 		if (err) {
 			device_unregister(hdev);
 			/*
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 14325f93c6b2..389ed45d4bd7 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -467,6 +467,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);
+
 /**
  * hwmon_is_bad_char - Is the char invalid in a hwmon name
  * @ch: the char to be considered
-- 
2.35.1