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

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

On 10/9/2023 4:53 PM, Ilpo Järvinen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Mon, 9 Oct 2023, Hegde, Suma wrote:
>> On 10/6/2023 6:51 PM, Ilpo Järvinen wrote:
>>> 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.
> I wasn't expecting it to fail at the moment, I just don't want to leave a
> silent trap for the future.
>
>> 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;
> This is similarly trappy as it truncates i if num_sockets is > 255.
>
> I suggest you just put this into start of the function:
>
>          /* String formatting is currently limited to u8 sockets */
>          if (WARN_ON(plat_dev.num_sockets > U8_MAX))
>                  return -ERANGE;
>
> to catch the too many sockets case if it is ever going to occur.
> And explain in the changelog that u8 is enough for foreseeable future
> and the string formatting triggers a warning if unnecessarily large type
> is passed to snprintf().
Ok Ilpo, I will add this WARN_ON and send v5 patch set.
>
> --
>   i.