Re: [PATCH 4/7] platform/x86/amd/hsmp: Leave the hwmon power sensors off on client platforms

On 7/30/2026 3:05 AM, Mario Limonciello wrote:
> 
> 
> 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.
> 
You are right and that sentence was wrong; it is fixed in v2. The metric
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?
> 
> 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.
> 
Agreed on the principle - userspace should not have to guess the layout.

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 <[email protected]>
>> ---
>>   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;
>