Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info
From: Guenter Roeck
Date: Sat May 21 2022 - 09:52:24 EST
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@xxxxxxxxxxxx>
---
[ ... ]
@@ -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.
GuenterFrom 0ff1a316c0f0ca36c49f2415f41d49ef1581808d Mon Sep 17 00:00:00 2001
From: Guenter Roeck <linux@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
---
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