Re: [PATCH 4/7] platform/x86/amd/hsmp: Leave the hwmon power sensors off on client platforms
From: M K, Muralidhara
Date: Thu Jul 30 2026 - 06:29:04 EST
On 7/30/2026 3:05 AM, Mario Limonciello wrote:
You are right and that sentence was wrong; it is fixed in v2. The metric
On 7/29/26 11:40, Muralidhara M K wrote:
hwmon.c issues the server power message IDs unconditionally, so on the
Family 1Ah client platforms the sensors address whatever the Ryzen
Master SMC set puts at those numbers:
power1_input 04h HSMP_CLIENT_GET_METRICS_TABLE_VER
power1_cap 06h HSMP_CLIENT_GET_METRICS_TABLE_DRAM_ADDR
Both match the num_args and response_sz that the sensors ask for, so
validate_message() accepts them and the two files report a metrics-table
version and the low half of a DRAM address as microwatts.
The other two accesses are already rejected, as they land on a client
message taking a different num_args: power1_cap_max on 07h
(HSMP_CLIENT_SET_CORE_PSM_MARGIN) and the power1_cap write path, which
sends HSMP_SET_SOCKET_POWER_LIMIT on 05h
(HSMP_CLIENT_GET_METRICS_TABLE). No unintended firmware write is
possible either way.
The client message set has no power telemetry to report instead
Is that right? I thought the metrics table has accumulator values.
table does carry power - system_power_acc, apu_power_acc, npu_power_acc
and the per-rail vddcr_*_telemetry_power fields.
What I should have written is narrower. The problem is not that the
client has no power figure, it is that hwmon's power1_* attributes reach
for it by issuing the server power message IDs, and those numbers are
metric-table queries in the client set. I registered the sensors on the
Model 80h system with the guard removed to show exactly what comes out:
power1_input 5000 = table version 5, x1000
power1_cap 1574441292000 = low word of the table DRAM
address 0x5DD8114C, x1000
power1_cap_max -EINVAL
That is 5 mW and 1.57 MW. Both correlate exactly with what those message
IDs return on this part (04h -> 0x5, 06h -> 0x5DD8114C).
I also checked the write path, since that is the one that would matter:
writing power1_cap sends HSMP_SET_SOCKET_POWER_LIMIT (05h), which is
GET_METRICS_TABLE on the client and takes a different argument count, so
validate_message() rejects it with -EINVAL before a message is built.
dmesg stays empty across the attempt - nothing reaches firmware.
Getting a real reading is not a matter of substituting a message ID: the
table's power fields are monotonic accumulators, so power1_input would
have to read the table, difference two snapshots and normalise by
accumulation_counter. That is a different shape of sensor from the
one-message-per-read hwmon path here, so I would rather add it
deliberately on top than bend this patch into it. This one just keeps the misleading files from appearing.
Which brings me to my next question - where is the format of the metrics table declared?Agreed on the principle - userspace should not have to guess the layout.
It's probably (but not guaranteed) the same metrics table format used in PMF. IE this:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers- x86.git/commit/?h=review-ilpo- next&id=641b41a7a12537f8898b1e14c62e0d85a8b872c2
But if userspace does and HSMP message to get the metrics table I think we need a kernel header that shows them how to decode the structure.
It is not the PMF layout. I compared against smu_pmf_metrics_v2: that is
a table of instantaneous u16 values in mW, MHz and centi-C delivered over the PMF-SMU mailbox. The HSMP client table is a different structure - u64 accumulators, per-CCX per-core arrays for residency, frequency,
temperature and power, per-rail set and telemetry voltages, per-throttler limit and residency groups, and the overclocking state. Same SoC, but different producers and different layouts, so sharing a struct would be wrong.
Where I would push back is on declaring it in a kernel header. There is
no single layout to declare: the model variants this series enables carry different telemetry tables, and each new model can add another. A header that covered them would need one large struct per table version, named after the version because there is nothing else to name it after, which reads badly and has to grow with every part. The driver decodes none of it - it copies the table out as bytes - so those declarations would exist only for userspace, in a header the kernel itself cannot validate against the firmware that produces the data.
The AMD Public PPR documents the telemetry table per model, with the
field order, widths and units. That is the reference a tool should decode against, and it stays correct as models are added without the kernel tracking a firmware data structure it never interprets. The driver's part is getting the bytes out, which the metric table ioctl already does.
amd_hsmp.rst says this in v2: it describes how to obtain the table, and
for the client sends the reader to the telemetry table for the model in
the PPR to decode it.
, so gate
the registration on the platform in both drivers rather than teach
hwmon.c two message sets. Server behaviour is unchanged.
The ACPI sysfs attributes are left alone. They are hardcoded to server
message IDs too, but on the client each one is either meaningful or
rejected: 02h and 03h are the SMU and interface version queries in both
message sets, and the num_args and response_sz checks in
validate_message() reject the rest, including every ID that lands on a
client SET.
Signed-off-by: Muralidhara M K <muralidhara.mk@xxxxxxx>
---
drivers/platform/x86/amd/hsmp/acpi.c | 13 ++++++++++---
drivers/platform/x86/amd/hsmp/plat.c | 15 +++++++++++----
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/ x86/amd/hsmp/acpi.c
index bbf2b9a8a408..57758df9d353 100644
--- a/drivers/platform/x86/amd/hsmp/acpi.c
+++ b/drivers/platform/x86/amd/hsmp/acpi.c
@@ -670,9 +670,16 @@ 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");
+ /*
+ * The hwmon power messages are numbered as metric table queries in the
+ * client message set, so the sensors would report those responses as
+ * power readings. Leave them off there, as plat.c does.
+ */
+ 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]);
diff --git a/drivers/platform/x86/amd/hsmp/plat.c b/drivers/platform/ x86/amd/hsmp/plat.c
index bc29b0ec18f7..17877e25f43e 100644
--- a/drivers/platform/x86/amd/hsmp/plat.c
+++ b/drivers/platform/x86/amd/hsmp/plat.c
@@ -196,10 +196,17 @@ static int init_platform_device(struct device *dev)
dev_info(dev, "Failed to init metric table\n");
}
- /* Register with hwmon interface for reporting power */
- ret = hsmp_create_sensor(dev, i);
- if (ret)
- dev_info(dev, "Failed to register HSMP sensors with hwmon\n");
+ /*
+ * Register with hwmon interface for reporting power. Skipped
+ * on the client, where the power messages the sensors issue
+ * are numbered as metric table queries and would report those
+ * responses as power readings.
+ */
+ if (!is_client_platform()) {
+ ret = hsmp_create_sensor(dev, i);
+ if (ret)
+ dev_info(dev, "Failed to register HSMP sensors with hwmon\n");
+ }
}
return 0;