Hi Ilpo, On 1/5/2024 3:01 PM, Ilpo Järvinen wrote:
Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. 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?
Yes, it was intentional. In case of non-acpi probing, only one "amd_hsmp/" directory will be created for all the sockets under "/sys/devices/platform/". So we need to create "socket%u" directories separately for each socket. Where as in case of acpi probing, there is already a separate directory "AMDI000XX:00/" created for each socket since there is a one probe call for each socket. So I thought no need of one more "socket%u" directory.
Thanks for all your review comments. I will address them in v5 patchset.
+ ret = hsmp_create_attr_list(attr_grp, dev, sock_ind); + if (ret) + return ret; + + return devm_device_add_group(dev, attr_grp); +}
Thanks and Regards, Suma
-- i.
