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@xxxxxxx>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@xxxxxxx>
> ---
> 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.
