Re: [PATCH v3 2/3] platform/x86/amd/hsmp: add support for metrics tbl

[Hans de Goede <hdegoede@redhat.com>]

HI,

On 9/22/23 10:56, Ilpo Järvinen wrote:
> On Thu, 21 Sep 2023, Hans de Goede wrote:
>> On 9/19/23 15:07, Ilpo Järvinen wrote:
>>> On Tue, 19 Sep 2023, Suma Hegde wrote:
>>>
>>>> AMD MI300 MCM provides GET_METRICS_TABLE message to retrieve
>>>> all the system management information from SMU.
>>>>
>>>> The metrics table is made available as hexadecimal sysfs binary file
>>>> under per socket sysfs directory created at
>>>> /sys/devices/platform/amd_hsmp/socket%d/metrics_bin
>>>>
>>>> Metrics table definitions will be documented as part of Public PPR.
>>>> The same is defined in the amd_hsmp.h header.
>>>>
>>>> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
>>>> Reviewed-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
>>>> ---
>>>> Changes since v1:
>>>> 1. Remove HSMP_DEVNODE_NAME and HSMP_CDEV_NAME macro definitions in
>>>> this patch
>>>> 2. Remove extra space in comments for HSMP_GET_METRIC_TABLE_VER,
>>>>    HSMP_GET_METRIC_TABLE and HSMP_GET_METRIC_TABLE_DRAM_ADDR enum
>>>>    definition in amd_hsmp.h files
>>>> 3. Change check, count == 0 to !count in hsmp_metric_tbl_read() function
>>>> 4. Add hsmp_metric_table_visible() function 
>>>> 5. hsmp_create_metric_tbl_sysfs_file() is renamed as hsmp_init_metric_tbl_bin_attr()
>>>>    and code is also modified slightly
>>>> 6. Modify hsmp_create_sysfs_file() to use devm_device_add_groups()
>>>> 7. Change from cleanup label to deregister label
>>>> 8. Add dev_err print in hsmp_get_tbl_dram_base()
>>>> 9. Reword "Unable to Failed" in hsmp_get_tbl_dram_base()
>>>> 10. Add HSMP_GRP_NAME_SIZE and NUM_ATTRS macros
>>>> 11. Remove sysfs cleanup code in hsmp_pltdrv_remove()
>>>> 12. Correct ATRR typo error
>>>> 13. Change sprintf to snprintf
>>>> 14. Check metrics table support only against HSMP_PROTO_VER6
>>>> Changes since v2:
>>>> 1. squash documentation patch into this patch
>>>> 2. change from num_sockets to plat_dev.num_sockets
>>>>
>>>>  Documentation/arch/x86/amd_hsmp.rst  |  16 +++
>>>>  arch/x86/include/uapi/asm/amd_hsmp.h | 109 ++++++++++++++++
>>>>  drivers/platform/x86/amd/hsmp.c      | 180 ++++++++++++++++++++++++++-
>>>>  3 files changed, 302 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/Documentation/arch/x86/amd_hsmp.rst b/Documentation/arch/x86/amd_hsmp.rst
>>>> index 440e4b645a1c..a4c308784818 100644
>>>> --- a/Documentation/arch/x86/amd_hsmp.rst
>>>> +++ b/Documentation/arch/x86/amd_hsmp.rst
>>>> @@ -41,6 +41,22 @@ In-kernel integration:
>>>>   * Locking across callers is taken care by the driver.
>>>>  
>>>>  
>>>> +HSMP sysfs interface
>>>> +====================
>>>> +
>>>> +1. Metrics table binary sysfs
>>>> +
>>>> +AMD MI300A MCM provides GET_METRICS_TABLE message to retrieve
>>>> +all the system management information from SMU.
>>>> +
>>>> +The metrics table is made available as hexadecimal sysfs binary file
>>>> +under per socket sysfs directory created at
>>>> +/sys/devices/platform/amd_hsmp/socket%d/metrics_bin
>>>> +
>>>> +Metrics table definitions will be documented as part of Public PPR.
>>>> +The same is defined in the amd_hsmp.h header.
>>>> +
>>>> +
>>>>  An example
>>>>  ==========
>>>>  
>>>
>>> I'd have expected to have the sysfs documentation appear under 
>>> Documentation/ABI/testing/sysfs-...
>>
>> Actually it is somewhat normal for sysfs files to be paired
>> together with other documentation when there is more extensive
>> documentation then just the sysfs files, see e.g. :
>>
>> Documentation/admin-guide/laptops/thinkpad-acpi.rst
> 
> Okay but that seems to result in attempting to handle deprecation 
> within that file too which feels wrong beyond just hacing documentation in 
> an unusual location.
> 
>> So there is precedent for this and I think it make sense
>> to keep all the documentation in one place, rather then to add
>> a Documentation/ABI/testing/sysfs-platform-amd-hsmp file
>> just for the sysfs attributes .
>>
>> OTOH people are used to look for sysfs attribute documentation
>> in a place like Documentation/ABI/testing/sysfs-platform-amd-hsmp,
>> but I think that keeping all the docs together is more important.
>>
>> So I have a slight preference for keeping this as is and
>> just merging v3 of this series as is.
>>
>> Ilpo, what do you think ?
> 
> I don't have strong preference myself if you think the current location 
> is fine. So feel free to merge it as is.

I'm traveling this week.

Ilpo, if you're fine with this series as is, can you merge it please ?

Regards,

Hans