Message-ID: <7c8db8ba-19ad-417f-bacd-00e8e88eea25@roeck-us.net>
Date: Thu, 27 Feb 2025 04:31:05 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: "lihuisong (C)" <lihuisong@...wei.com>
Cc: linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
 zhanjie9@...ilicon.com, zhenglifeng1@...wei.com, liuyonglong@...wei.com,
 jdelvare@...e.com
Subject: Re: [RFC] hwmon: (acpi_power_meter) Replace hwmon_device_register

On 2/27/25 00:21, lihuisong (C) wrote:
> 
> 在 2025/2/26 21:26, Guenter Roeck 写道:
>> 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.
> Got it.
>>
>>> (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.
> will do it.
>>
>>> 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.
> Ack
>>
>>> 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.
> ok.
> What I mean is that is the display in power meter.

Again, that is a problem in the power meter driver, not a problem
in the hwmon core.

>>
>>> 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.
> Yes
>>
>> 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.
> Agreed. But we still can't break ABI of this driver. will retain what it was.
>>
>>>
>>> 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.
> Ack
>>
>>> 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.
>>
>>
> Ok, I will put these attributes above into extra_groups and add some comments for them.
> Many thanks for your good suggestion.
> 
> 
> Hi Guenter,
> 
> BTW, I have another problem as commit log described:
> -->
> 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, path is "/sys/class/hwmon/hwmon1/device/" ('device' in the path is a soft link of acpi device)
> Now: all sysfs interfaces are under hwmon device, namely, path is "/sys/class/hwmon/hwmon1/"
> What do you think about this?
> 

That is as intended. The ABI states that the attributes are under
/sys/class/hwmon/hwmonX _or_ /sys/class/hwmon/hwmonX/device/.
The ABI does not guarantee that the underlying path remains the same.
libsensors handles this automatically, as should any userspace programs
accessing the attributes directly.

Guenter

Powered by blists - more mailing lists