Re: [PATCH v7 3/4] platform/x86/amd/hsmp: Hide server-only attributes on client platforms
From: Ilpo Järvinen
Date: Thu Oct 08 2026 - 07:58:28 EST
On Thu, 24 Sep 2026, Muralidhara M K wrote:
> Most ACPI sysfs device attributes are wired to hardcoded server
> message IDs (HSMP_GET_SOCKET_POWER and friends); hwmon power
> attributes registered from init_acpi() have the same problem. On a
> Family 1Ah client platform these numeric IDs resolve against enum
> hsmp_client_message_ids instead, and mostly collide with unrelated
> client commands, e.g. HSMP_GET_SOCKET_POWER (4) is
> HSMP_CLIENT_GET_METRICS_TABLE_VER on the client side.
>
> Hide the affected ACPI sysfs attributes on client platforms except
> smu_fw_version and protocol_version, which report the same data in
> both tables. Forward-declare hattr_smu_fw_version and
> hattr_protocol_version ahead of their HSMP_DEV_ATTR() definitions.
> Skip hsmp_create_sensor() in init_acpi() on client platforms for the
> same reason.
>
> The metrics_bin sysfs binary attribute has the same visibility problem:
> hsmp_is_sock_attr_visible() gates it on hsmp_pdev->proto_ver against
> HSMP_PROTO_VER6 with no is_client_platform() check. Exclude client
> platforms outright there too, matching the ACPI sysfs attributes
> above: the client set has its own telemetry table, read only through
> HSMP_IOCTL_GET_TELEMETRY_DATA.
>
> Signed-off-by: Muralidhara M K <muralidhara.mk@xxxxxxx>
> ---
> drivers/platform/x86/amd/hsmp/acpi.c | 29 ++++++++++++++++++++++++----
> 1 file changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
> index 8b4bf57c3485..d775ea8ba603 100644
> --- a/drivers/platform/x86/amd/hsmp/acpi.c
> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
> @@ -300,16 +300,31 @@ static umode_t hsmp_is_sock_attr_visible(struct kobject *kobj,
> * so that userspace which expects the file to exist gets a clear
> * -EOPNOTSUPP from the read handler instead of -ENOENT, and is
> * pointed at HSMP_IOCTL_GET_TELEMETRY_DATA as the supported path.
> + * The client set has its own telemetry table, read only through
> + * HSMP_IOCTL_GET_TELEMETRY_DATA; never show this file there.
> */
> - if (hsmp_pdev->proto_ver >= HSMP_PROTO_VER6)
> + if (!is_client_platform() && hsmp_pdev->proto_ver >= HSMP_PROTO_VER6)
> return battr->attr.mode;
>
> return 0;
> }
>
> +/* Defined below by HSMP_DEV_ATTR(); same message ID in both message tables */
> +static struct hsmp_sys_attr hattr_smu_fw_version;
> +static struct hsmp_sys_attr hattr_protocol_version;
> +
> static umode_t hsmp_is_sock_dev_attr_visible(struct kobject *kobj,
> struct attribute *attr, int id)
> {
> + /*
> + * smu_fw_version and protocol_version map to the same data in both
> + * message tables. Every other attribute here uses server-only
> + * message IDs that resolve to unrelated client commands.
> + */
> + if (is_client_platform() && attr != &hattr_smu_fw_version.dattr.attr &&
> + attr != &hattr_protocol_version.dattr.attr)
> + return 0;
> +
> return attr->mode;
> }
>
> @@ -564,9 +579,15 @@ static int init_acpi(struct device *dev)
> dev_info(dev, "Failed to init metric table\n");
> }
>
> - ret = hsmp_create_sensor(dev, sock_ind);
> - if (ret)
> - dev_info(dev, "Failed to register HSMP sensors with hwmon\n");
> + /*
> + * hwmon attributes use server-only message IDs, which resolve to
> + * unrelated client commands on client platforms.
> + */
> + if (!is_client_platform()) {
> + ret = hsmp_create_sensor(dev, sock_ind);
> + if (ret)
> + dev_info(dev, "Failed to register HSMP sensors with hwmon\n");
> + }
>
> dev_set_drvdata(dev, &hsmp_pdev->sock[sock_ind]);
This series still feels misordered.
Can we like introduce is_client_platform() first, then add all these
checks before adding the client support in the first place? This
same ordering problem applies to the client/server check in patch 2 as
well.
And would structs in patch 4 also needed earlier? This is not as bad
problem as the above ordering one but I'd tend to think we'd want to
introduce the struct before HSMP_CLIENT_GET_METRICS_TABLE actually works
which is after patch 1, right?
--
i.