Message-ID: <d765aeb3-3ca6-44a1-9337-2706621df903@roeck-us.net>
Date: Wed, 26 Feb 2025 05:26:33 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: "lihuisong (C)" <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: [RFC] hwmon: (acpi_power_meter) Replace hwmon_device_register

On 2/26/25 02:19, lihuisong (C) wrote:
> Hi Guenter,
> 
> 在 2025/2/25 21:01, Guenter Roeck 写道:
>> On 2/25/25 00:51, 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.
>>>
>>> To avoid any changes in the display of some sysfs interfaces, some of
>>> necessary changes in hwmon.c must be made:
>>> 1> For 'power1_average_interval_max/min' interface, insert 'average' to the
>>>     string corresponding to hwmon_power_average_interval_max/max in
>>>     hwmon_power_attr_templates[]. I guess that is what's missing.
>>> 2> Add some string attributes in power sensor type because of below items:
>>>     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.
>>> Note: All the attributes modified above in hwmon core are not used by other
>>> drivers.
>>>
>>
>> That is not a reason to change the ABI, much less so hiding the change
>> in a driver patch.
>>
>>
> I am trying to replace the deprecated hwmon_device_register with hwmon_device_register_with_info for acpi power meter driver.
> 
> To avoid any changes in the display of some sysfs interfaces, there are two modifications in hwmon core as follows:

The only reason to change the hwmon core would be if it is wrong or if it needs to
be amended. Matching driver expectations is not a valid reason.

> (1) The first modification in hwmon is as follows:
> -->
> @@ -646,8 +653,8 @@ static const char * const hwmon_power_attr_templates[] = {
>       [hwmon_power_enable] = "power%d_enable",
>       [hwmon_power_average] = "power%d_average",
>       [hwmon_power_average_interval] = "power%d_average_interval",
> -    [hwmon_power_average_interval_max] = "power%d_interval_max",
> -    [hwmon_power_average_interval_min] = "power%d_interval_min",
> +    [hwmon_power_average_interval_max] = "power%d_average_interval_max",
> +    [hwmon_power_average_interval_min] = "power%d_average_interval_min",
>       [hwmon_power_average_highest] = "power%d_average_highest",
> 
That is indeed a bug and should be fixed, but in a separate patch.

> The string names, "power%d_interval_max/min", are missing 'average'.
> I think the meaning of these attributes are unclear If no this word. It can be regarded as a fault.
> And power attribute name in acpi power meter is "power1_average_interval_min/max".
> 
> (2)The second modification changes the attribute of 'power_accuracy', 'power_cap_hyst', 'power_average_min' and 'power_average_max' from data to string.
> It is appropriate to assign 'power_accuracy' to string attribute.

No. The ABI states that this is the accuracy in %. We don't append "mV"
to voltages, or "mA" to currents either. The unit is determined by the ABI,
which states that the expected value is a number reflecting %. If a driver
adds "%", it is a driver oddity, but not a hwmon bug. The whole point of
providing numeric values is to simplify parsing from userspace. Adding units
to the displayed value would not only be pointless (since the unit is defined
by the ABI) but also make parsing more difficult.

> Because it can be displayed as '%' and also include decimal point like acpi power meter driver, which is more in line with the meaning of this attribute.

Why ? Are you suggesting that all other attributes should provide units as well
"to be more in line with the meaning of those attributes" ?

It is absolutely not common to add units to sysfs attributes. We are not going
to do that, period.

> It might be better to keep other attributes as data types. But it breaks the cornor display of these attributes in acpi power meter driver as said below.
>     a) power1_cap_hyst  --> display 'unknown' when its value is 0xFFFFFFFF
>     b) power1_average_min/max --> display 'unknown' when its value is negative.

That is a driver problem, not a subsystem problem. If it is so important to retain
that (i.e., if for some reason some userspace program depends on it), just
implement the attributes in the driver.

On a practical note, I do wonder what it means if ACPI reports those values.
It might simply mean that they are not supported. If so, the attributes
should not be instantiated in the first place.

> 
> I want to say that all the attributes modified above in hwmon core are not used by other drivers, so don't break ABI of some driver.

That is not a valid argument. Especially displaying values such as "unknown" or
starting to display units as part of an attributes _is_ an API break since that
is completely unexpected.

> These can't be solved in this driver side.

That is incorrect.

> 
> AFAICS, acpi power meter driver can't replace the deprecated API because their sysfs interfaces will be broken if there's no any modification in hwmon core.
> 

That is simply wrong. The _with_info API supports non-standard attributes
with the extra_groups parameter. Just use that and implement the non-standard
attributes in the driver and explain why you are doing it in a comment.

Guenter

Powered by blists - more mailing lists