Re: [PATCH v4 6/9] platform/x86/amd/hsmp: Add support for ACPI based probing

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

On Fri, 5 Jan 2024, Suma Hegde wrote:

> ACPI table provides mailbox base address and register offset
> information. The base address is provided as part of CRS method
> and mailbox offsets are provided through DSD table.
> Sockets are differentiated by UIDs.
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
> Changes since v3:
> 1. Add hsmp_create_acpi_sysfs_if() and
>    hsmp_create_non_acpi_sysfs_if() separately
> 2. Change hardcoded value 16 in is_acpi_hsmp_uuid() to UUID_SIZE
> 3. Change commit message
> Changes since v2:
> 1. Change EINVAL to ENODEV in hsmp_read_acpi_dsd()
> 2. Change EINVAL to ENOENT in hsmp_read_acpi_dsd()
> 3. Use resource_size() in hsmp_resource()
> Changes since v1:
> 1. Define amd_hsmp_acpi_rdwr() for doing mailbox memory mapped io
> 2. Add a check to see if mailbox register offsets are set in
>    hsmp_read_acpi_dsd()
> 3. Add a check to see if sock->mbinfo.base_addr sockck->mbinfo.size are
>    set in hsmp_read_acpi_crs()
> 4. Change order of the statements in switch case ACPI_RESOURCE_TYPE_FIXED_MEMORY32
>    in hsmp_resource()
> 5. Add hsmp_test() after hsmp_parse_acpi_table() call
> 6. Add r.end < r.start check in hsmp_resource()
> 7. Add !dsd error check in hsmp_read_acpi_dsd


> @@ -442,58 +648,85 @@ static int hsmp_init_metric_tbl_bin_attr(struct bin_attribute **hattrs, u16 sock
>  /* One bin sysfs for metrics table*/
>  #define NUM_HSMP_ATTRS		1
>  
> -static int hsmp_create_sysfs_interface(void)
> +static int hsmp_create_attr_list(struct attribute_group *attr_grp,
> +				 struct device *dev, u16 sock_ind)
>  {
> -	const struct attribute_group **hsmp_attr_grps;
>  	struct bin_attribute **hsmp_bin_attrs;
> +
> +	/* Null terminated list of attributes */
> +	hsmp_bin_attrs = devm_kzalloc(dev, sizeof(struct bin_attribute *) *
> +				      (NUM_HSMP_ATTRS + 1), GFP_KERNEL);
> +	if (!hsmp_bin_attrs)
> +		return -ENOMEM;
> +
> +	attr_grp->bin_attrs = hsmp_bin_attrs;
> +
> +	return hsmp_init_metric_tbl_bin_attr(hsmp_bin_attrs, sock_ind);
> +}
> +
> +static int hsmp_create_non_acpi_sysfs_if(struct device *dev)
> +{
> +	const struct attribute_group **hsmp_attr_grps;
>  	struct attribute_group *attr_grp;
> -	int ret;
>  	u16 i;
>  
>  	/* String formatting is currently limited to u8 sockets */
>  	if (WARN_ON(plat_dev.num_sockets > U8_MAX))
>  		return -ERANGE;
>  
> -	hsmp_attr_grps = devm_kzalloc(plat_dev.sock[0].dev, sizeof(struct attribute_group *) *
> +	hsmp_attr_grps = devm_kzalloc(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++) {
> -		attr_grp = devm_kzalloc(plat_dev.sock[i].dev, sizeof(struct attribute_group),
> +		attr_grp = devm_kzalloc(dev, sizeof(struct attribute_group),
>  					GFP_KERNEL);
>  		if (!attr_grp)
>  			return -ENOMEM;
>  
>  		snprintf(plat_dev.sock[i].name, HSMP_ATTR_GRP_NAME_SIZE, "socket%u", (u8)i);
> -		attr_grp->name = plat_dev.sock[i].name;
> -
> -		/* Null terminated list of attributes */
> -		hsmp_bin_attrs = devm_kzalloc(plat_dev.sock[i].dev, sizeof(struct bin_attribute *) *
> -					      (NUM_HSMP_ATTRS + 1), GFP_KERNEL);
> -		if (!hsmp_bin_attrs)
> -			return -ENOMEM;
> -
> -		attr_grp->bin_attrs		= hsmp_bin_attrs;
> +		attr_grp->name			= plat_dev.sock[i].name;
>  		attr_grp->is_bin_visible	= hsmp_is_sock_attr_visible;
>  		hsmp_attr_grps[i]		= attr_grp;
>  
> -		/* Now create the leaf nodes */
> -		ret = hsmp_init_metric_tbl_bin_attr(hsmp_bin_attrs, i);
> -		if (ret)
> -			return ret;
> +		hsmp_create_attr_list(attr_grp, dev, i);
>  	}
> -	return devm_device_add_groups(plat_dev.sock[0].dev, hsmp_attr_grps);
> +
> +	return devm_device_add_groups(dev, hsmp_attr_grps);
>  }

Can this refactoring of existing code be put into own patch, it would make 
this patch smaller?

> -static int hsmp_cache_proto_ver(void)
> +static int hsmp_create_acpi_sysfs_if(struct device *dev)
> +{
> +	struct attribute_group *attr_grp;
> +	u16 sock_ind;
> +	int ret;
> +
> +	attr_grp = devm_kzalloc(dev, sizeof(struct attribute_group), GFP_KERNEL);
> +	if (!attr_grp)
> +		return -ENOMEM;
> +
> +	attr_grp->is_bin_visible = hsmp_is_sock_attr_visible;
> +	ret = hsmp_get_uid(dev, &sock_ind);
> +	if (ret)
> +		return ret;

Is it intentional you don't provide .name here? hsmp_get_uid() gets you a 
socket id if I read your code correctly so wouldn't be more consistent to 
generate the "socket%u" name based on that?

> +	ret = hsmp_create_attr_list(attr_grp, dev, sock_ind);
> +	if (ret)
> +		return ret;
> +
> +	return devm_device_add_group(dev, attr_grp);
> +}


-- 
 i.