linux-kernel - Re: [PATCH v2] hwmon: (acpi_power_meter) Replace the deprecated hwmon_device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <899edff7-eba1-49fb-9626-576798452f30@roeck-us.net>
Date: Fri, 14 Mar 2025 08:01:08 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Huisong Li <lihuisong@...wei.com>, jdelvare@...e.com
Cc: linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
 zhanjie9@...ilicon.com, zhenglifeng1@...wei.com, liuyonglong@...wei.com
Subject: Re: [PATCH v2] hwmon: (acpi_power_meter) Replace the deprecated
 hwmon_device_register

On 3/14/25 01:18, Huisong Li wrote:
> When load this mode, we can see the following log:
> "power_meter ACPI000D:00: hwmon_device_register() is deprecated. Please
>   convert the driver to use hwmon_device_register_with_info()."
> 
> So replace hwmon_device_register with hwmon_device_register_with_info.
> 
> These attributes, 'power_accuracy', 'power_cap_hyst', 'power_average_min'
> and 'power_average_max', should have been placed in hwmon_chip_info as
> power data type. But these attributes are displayed as string format on
> the following case:
> a) power1_accuracy  --> display like '90.0%'
> b) power1_cap_hyst  --> display 'unknown' when its value is 0xFFFFFFFF
> c) power1_average_min/max --> display 'unknown' when its value is
> 			      negative.
> To avoid any changes in the display of these sysfs interfaces, we can't
> modifiy the type of these attributes in hwmon core and have to put them
> to extra_groups.
> 
> Please note that the path of these sysfs interfaces are modified
> accordingly if use hwmon_device_register_with_info():
> old: all sysfs interfaces are under acpi device, namely,
>       /sys/class/hwmon/hwmonX/device/
> now: all sysfs interfaces are under hwmon device, namely,
>       /sys/class/hwmon/hwmonX/
> The new ABI does not guarantee that the underlying path remains the same.
> But we have to accept this change so as to replace the deprecated API.
> Fortunately, some userspace application, like libsensors, would scan
> the two path and handles this automatically. So to drop the deprecated
> message, we can accept this change.
> 
> Signed-off-by: Huisong Li <lihuisong@...wei.com>
> ---
>   drivers/hwmon/acpi_power_meter.c | 861 +++++++++++++++----------------
>   1 file changed, 427 insertions(+), 434 deletions(-)
> 
> diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> index f05986e4f379..46e8a0b6b210 100644
> --- a/drivers/hwmon/acpi_power_meter.c
> +++ b/drivers/hwmon/acpi_power_meter.c
...
> +
> +// depend on POWER_METER_CAN_TRIP

Please di not intermix C++ comments with a driver using C comments.

> +static DEVICE_ATTR_RW(power1_average_max);
> +static DEVICE_ATTR_RW(power1_average_min);
> +
> +// depend on POWER_METER_CAN_CAP
> +static DEVICE_ATTR_RO(power1_cap_hyst);
> +
> +// depend on POWER_METER_CAN_MEASURE
> +static DEVICE_ATTR_RO(power1_accuracy);
> +static DEVICE_ATTR_RO(power1_is_battery);
> +
> +static DEVICE_ATTR_RO(power1_model_number);
> +static DEVICE_ATTR_RO(power1_oem_info);
> +static DEVICE_ATTR_RO(power1_serial_number);
> +
> +#define EXTRA_FIRST_DYNAMIC_ATTR_ID	3
> +#define EXTRA_ATTR_MAX	10
> +
> +static struct attribute *power_extra_attrs[EXTRA_ATTR_MAX] = {
> +	&dev_attr_power1_model_number.attr,
> +	&dev_attr_power1_oem_info.attr,
> +	&dev_attr_power1_serial_number.attr,
> +	NULL
> +};
> +
> +ATTRIBUTE_GROUPS(power_extra);
> +
> +static void fill_extra_dynamic_attr(struct attribute *attr)
> +{
> +	int idx = EXTRA_FIRST_DYNAMIC_ATTR_ID;
> +
> +	for (idx = EXTRA_FIRST_DYNAMIC_ATTR_ID; idx < EXTRA_ATTR_MAX; idx++) {
> +		if (!power_extra_attrs[idx])
> +			break;
> +	}
> +
> +	power_extra_attrs[idx] = attr;
> +}
> +

Please use the .is_visible() callback in attribute_group to determine
attribute visibility.

That means you'll have to code attribute_group manually, but that is
still better than re-implementing the is_visible() callback.
Something like

static const struct attribute_group power_extra_group = {
	.attrs = power_extra_attrs,
	.is_visible = power_extra_is_visible;
};

__ATTRIBUTE_GROUPS(power_extra);

should do.

Thanks,
Guenter