Re: [PATCH v3 6/7] platform/x86/amd/hsmp: Drop ACPI sysfs metrics_bin in favour of the IOCTL

["M K, Muralidhara" <muralimk@amd.com>]

On 5/22/2026 5:14 PM, Ilpo Järvinen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Sun, 17 May 2026, Muralidhara M K wrote:
> 
>> The HSMP_IOCTL_GET_TELEMETRY_DATA character-device ioctl introduced in
>> the previous patch is now the canonical interface for reading the
>> metric table on the ACPI driver path.  Unlike the metrics_bin sysfs
>> binary attribute, the ioctl is not constrained by PAGE_SIZE, so it
>> works for the ~13 KB hsmp_metric_table_zen6 layout
> 
> This was only a secondary reason from moving away from sysfs files? The
> main cause for moving to misc device according to my impression was that
> you wanted to prevent partial reads which are always possible with files.
> 
Sure I will update the changelog to document the rationale for moving 
away from sysfs files.

>> used on Family 1Ah
>> Model 50h-5Fh as well as for the existing hsmp_metric_table layout
>> used on protocol version 6.
> 
> 
> 
>> Drop the metrics_bin bin_attribute from the ACPI hsmp_attr_grp.
> 
>> Widen the remaining proto_ver gate in init_acpi() from
>> '== HSMP_PROTO_VER6' to '>= HSMP_PROTO_VER6' so
>> hsmp_get_tbl_dram_base() is invoked on protocol version 7
>> (Family 1Ah Model 50h-5Fh) and any future protocol version that
>> retains a compatible per-socket metric table.  This populates
>> sock->metric_tbl_addr and hsmp_pdev.hsmp_table_size, which the ioctl
>> handler requires.
> 
> These are two logically separate changes so they should not be in the same
> patch (but you'll likely need to alter the drop patch anyway, see below).
> 
Ya Understood. I will make this change as a separate patch.

>> This is an ABI change for users of the ACPI driver:
>> /sys/bus/platform/devices/AMDI0097:*/metrics_bin no longer exists.
>> Userspace must read telemetry through the HSMP_IOCTL_GET_TELEMETRY_DATA
>> ioctl on /dev/hsmp instead, sizing its buffer using the matching UAPI
>> metric table struct.
> 
> I might not be entirely following what's the extent of removal here but it
> looks too extensive to me. The number 1 rule is that we cannot take away
> existing and working ABI without properly deprecating it first.
> 
> You don't have to make it work with version 7 and can return error in
> that case as it has never worked. But with version 6, things shouls be
> left as is until properly deprecated. You may consider adding a warning
> print too to warn about the deprecation and point towards the new way.
> 
I will review and ensure that an appropriate error is returned if sysfs 
is not supported for a particular version.

> This interface probably never was documented in Documentation/ABI where
> such deprecations are usually marked... oh well, maybe we need to add
> a simple entry for this ABI there to follow the usual deprecation path.
> 
> --
>   i.
> 
>> The non-ACPI plat.c path is intentionally left
>> unchanged: it covers Family 1Ah Model 0h-Fh hardware that is fixed at
>> protocol version 6, and its per-socket metrics_bin remains available
>> for existing userspace tooling on those systems.
>>
>> Co-developed-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
>> Signed-off-by: Muthusamy Ramalingam <muthusamy.ramalingam@amd.com>
>> Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
>> ---
>> Changes:
>> v1->v3: Remove bin attributes
>>
>>   drivers/platform/x86/amd/hsmp/acpi.c | 34 +---------------------------
>>   1 file changed, 1 insertion(+), 33 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
>> index 97ed71593bdf..49765fefe1fb 100644
>> --- a/drivers/platform/x86/amd/hsmp/acpi.c
>> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
>> @@ -231,25 +231,6 @@ static int hsmp_parse_acpi_table(struct device *dev, u16 sock_ind)
>>        return hsmp_read_acpi_dsd(sock);
>>   }
>>
>> -static ssize_t hsmp_metric_tbl_acpi_read(struct file *filp, struct kobject *kobj,
>> -                                      const struct bin_attribute *bin_attr, char *buf,
>> -                                      loff_t off, size_t count)
>> -{
>> -     struct device *dev = container_of(kobj, struct device, kobj);
>> -     struct hsmp_socket *sock = dev_get_drvdata(dev);
>> -
>> -     return hsmp_metric_tbl_read(sock, buf, count);
>> -}
>> -
>> -static umode_t hsmp_is_sock_attr_visible(struct kobject *kobj,
>> -                                      const struct bin_attribute *battr, int id)
>> -{
>> -     if (hsmp_pdev->proto_ver == HSMP_PROTO_VER6)
>> -             return battr->attr.mode;
>> -
>> -     return 0;
>> -}
>> -
>>   static umode_t hsmp_is_sock_dev_attr_visible(struct kobject *kobj,
>>                                             struct attribute *attr, int id)
>>   {
>> @@ -491,7 +472,7 @@ static int init_acpi(struct device *dev)
>>                return ret;
>>        }
>>
>> -     if (hsmp_pdev->proto_ver == HSMP_PROTO_VER6) {
>> +     if (hsmp_pdev->proto_ver >= HSMP_PROTO_VER6) {
>>                ret = hsmp_get_tbl_dram_base(sock_ind);
>>                if (ret)
>>                        dev_info(dev, "Failed to init metric table\n");
>> @@ -506,17 +487,6 @@ static int init_acpi(struct device *dev)
>>        return 0;
>>   }
>>
>> -static const struct bin_attribute  hsmp_metric_tbl_attr = {
>> -     .attr = { .name = HSMP_METRICS_TABLE_NAME, .mode = 0444},
>> -     .read = hsmp_metric_tbl_acpi_read,
>> -     .size = sizeof(struct hsmp_metric_table),
>> -};
>> -
>> -static const struct bin_attribute *hsmp_attr_list[] = {
>> -     &hsmp_metric_tbl_attr,
>> -     NULL
>> -};
>> -
>>   #define HSMP_DEV_ATTR(_name, _msg_id, _show, _mode)  \
>>   static struct hsmp_sys_attr hattr_##_name = {                \
>>        .dattr = __ATTR(_name, _mode, _show, NULL),     \
>> @@ -559,9 +529,7 @@ static struct attribute *hsmp_dev_attr_list[] = {
>>   };
>>
>>   static const struct attribute_group hsmp_attr_grp = {
>> -     .bin_attrs = hsmp_attr_list,
>>        .attrs = hsmp_dev_attr_list,
>> -     .is_bin_visible = hsmp_is_sock_attr_visible,
>>        .is_visible = hsmp_is_sock_dev_attr_visible,
>>   };
>>
>>