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

["Hegde, Suma" <Suma.Hegde@amd.com>]

On 10/6/2023 6:51 PM, Ilpo Järvinen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Thu, 5 Oct 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>
>
>> +static int hsmp_create_sysfs_interface(void)
>> +{
>> +     const struct attribute_group **hsmp_attr_grps;
>> +     struct bin_attribute **hsmp_bin_attrs;
>> +     struct attribute_group *attr_grp;
>> +     int ret;
>> +     u8 i;
>> +
>> +     hsmp_attr_grps = devm_kzalloc(plat_dev.dev, sizeof(struct attribute_group *) *
>> +                                   (plat_dev.num_sockets + 1), GFP_KERNEL);
>> +     if (!hsmp_attr_grps)
>> +             return -ENOMEM;
>> +
>> +     /* Create a sysfs directory for each socket */
>> +     for (i = 0; i < plat_dev.num_sockets; i++) {
> The change for i to u8 seems not very wise given num_sockets still is u16
> as it can turn this into an infinite loop.

Hi Ilpo,

amd_nb_num() API which we use to identify the number of sockets on a 
node returns u16. So, we used u16 for plat_dev.num_sockets.
u8 should be enough, as present AMD processors support upto 8 sockets on 
a node.

Coming to the warning raised:
We have defined, HSMP_ATTR_GRP_NAME_SIZE as 10bytes, 6 chars for 
'socket', 3 chars for 3digits (socket index) and a null terminator.
Socket index may not need more than 3 digits in the foreseeable future. 
How about, we declare i as u16 and typecast it as (u8) in the snprintf.

         int ret;
-       u8 i;
+       u16 i;

         hsmp_attr_grps = devm_kzalloc(plat_dev.dev, sizeof(struct 
attribute_group *) *
                                   (plat_dev.num_sockets + 1), 
GFP_KERNEL); @@ -449,7 +449,7 @@ static int 
hsmp_create_sysfs_interface(void)
         if (!attr_grp)
                 return -ENOMEM;

-       snprintf(plat_dev.sock[i].name, HSMP_ATTR_GRP_NAME_SIZE, 
"socket%u", i);
+       snprintf(plat_dev.sock[i].name, HSMP_ATTR_GRP_NAME_SIZE,
+ "socket%u", (u8)i);
         attr_grp->name = plat_dev.sock[i].name;


Thanks and Regards,

Suma

>
> --
>   i.
>