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

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

On Fri, 18 Aug 2023, Suma Hegde wrote:

> From: Suma Hegde <suma.hegde@amd.com>
> 
> AMD MI300 MCM provides GET_METRICS_TABLE message with which
> all the system management information from SMU can be retrieved in just
> one message.
> 
> The metrics table is available as hexadecimal sysfs binary file in
> /sys/devices/platform/amd_hsmp/socket%d_metrics_bin 
> Metrics table definitions will be documented as part of PPR which
> available in public domain. The same is defined in the amd_hsmp.h header
> file as well.
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---

> +#define HSMP_ATRR_NAME_SIZE 25
> +static int hsmp_create_metric_tbl_sysfs_file(int sock_ind)
> +{
> +	struct bin_attribute *hattr = &plat_dev.sock[sock_ind].hsmp_attr;
> +	char *name;
> +
> +	sysfs_attr_init(&plat_dev.sock[sock_ind].hsmp_attr);
> +	name = devm_kzalloc(plat_dev.dev, HSMP_ATRR_NAME_SIZE, GFP_KERNEL);
> +	if (!name)
> +		return -ENOMEM;
> +	sprintf(name, "socket%d_metrics_bin", sock_ind);

The structure would be more future-proof if you do socket%d/metrics_bin 
instead. Are you expected to add at some point in the future more files 
which are socket specific?

You didn't provide any documentation for this interface btw, you should 
add it.

> +	hattr->attr.name	= name;
> +	hattr->attr.mode	= 0444;
> +	hattr->read		= hsmp_metric_tbl_read;
> +	hattr->size		= sizeof(struct hsmp_metric_table);
> +	hattr->private		= &plat_dev.sock[sock_ind];
> +
> +	return device_create_bin_file(plat_dev.dev, hattr);
> +}

> +static int hsmp_create_sysfs_file(void)
> +{
> +	int ret, i;
> +
> +	for (i = 0; i < num_sockets; i++) {
> +		ret = hsmp_get_tbl_dram_base(i);
> +		if (ret)
> +			goto cleanup;
> +
> +		ret = hsmp_create_metric_tbl_sysfs_file(i);
> +		if (ret) {
> +			dev_err(plat_dev.dev, "Unable to create sysfs file for metric table\n");
> +			goto cleanup;
> +		}
> +	}
> +
> +	return 0;
> +
> +cleanup:
> +	while (i > 0)
> +		device_remove_bin_file(plat_dev.dev, &plat_dev.sock[--i].hsmp_attr);
> +	return ret;
> +}
> +
> +static int hsmp_cache_proto_ver(void)
> +{
> +	struct hsmp_message msg = { 0 };
> +	int ret;
> +
> +	msg.msg_id	= HSMP_GET_PROTO_VER;
> +	msg.sock_ind	= 0;
> +	msg.response_sz = hsmp_msg_desc_table[HSMP_GET_PROTO_VER].response_sz;
> +
> +	ret = hsmp_send_message(&msg);
> +	if (!ret)
> +		plat_dev.proto_ver = msg.args[0];
> +
> +	return ret;
> +}
> +
>  static int hsmp_pltdrv_probe(struct platform_device *pdev)
>  {
> -	int i;
> +	int ret, i;
>  
>  	plat_dev.sock = devm_kzalloc(&pdev->dev,
>  				     (num_sockets * sizeof(struct hsmp_socket)),
> @@ -344,18 +463,44 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
>  		plat_dev.sock[i].sock_ind = i;
>  	}
>  
> -	plat_dev.hsmp_device.name	= "hsmp_cdev";
> +	plat_dev.hsmp_device.name	= HSMP_CDEV_NAME;
>  	plat_dev.hsmp_device.minor	= MISC_DYNAMIC_MINOR;
>  	plat_dev.hsmp_device.fops	= &hsmp_fops;
>  	plat_dev.hsmp_device.parent	= &pdev->dev;
> -	plat_dev.hsmp_device.nodename	= "hsmp";
> +	plat_dev.hsmp_device.nodename	= HSMP_DEVNODE_NAME;
>  	plat_dev.hsmp_device.mode	= 0644;
>  
> -	return misc_register(&plat_dev.hsmp_device);
> +	ret = misc_register(&plat_dev.hsmp_device);
> +	if (ret)
> +		return ret;
> +
> +	ret = hsmp_cache_proto_ver();
> +	if (ret) {
> +		dev_err(plat_dev.dev, "Failed to read HSMP protocol version\n");
> +		goto cleanup;
> +	}
> +
> +	/* metrics table is supported only from proto ver6 EPYCs */
> +	if (plat_dev.proto_ver >= HSMP_PROTO_VER6) {
> +		ret = hsmp_create_sysfs_file();

For new things, driver/device attribute_group should be used where 
possible. .is_visible() can handle that condition check for you.


-- 
 i.