Re: [RFC] hwmon: (acpi_power_meter) Replace hwmon_device_register

["lihuisong (C)" <lihuisong@huawei.com>]

在 2025/2/27 20:31, Guenter Roeck 写道:
> 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.
>
ok
>>>
>>>> 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.
Yes
> libsensors handles this automatically, as should any userspace programs
> accessing the attributes directly.
>
>
So we don't need to care or handle this change of syfs path, right?

/Huisong

>
>
> .