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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Ilpo, Suma,

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@xxxxxxx>
>> Reviewed-by: Naveen Krishna Chatradhi <nchatrad@xxxxxxx>
>> ---
>> 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

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 ?


Regards,

Hans






[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux