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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3306 bytes --]

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().


-- 
 i.