Re: [PATCH v2 0/5] Introduce AMD PMF util layer and user-space interface for SystemDeck

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

Hi hans,

On 11/20/2025 00:55, Hans de Goede wrote:
> Hi Mario,
> 
> On 19-Nov-25 5:32 PM, Mario Limonciello wrote:
>> On 11/19/25 10:20 AM, Hans de Goede wrote:
>>> Hi Shyam,
>>>
>>> On 11-Nov-25 8:10 AM, Shyam Sundar S K wrote:
>>>> This series introduces a util layer to the AMD Platform Management
>>>> Framework (PMF) and a minimal user-space interface via a misc character
>>>> device, enabling feature discovery and smoother integration with
>>>> user-space tools. It also adds caching of BIOS output policy values to
>>>> prepare for user-space telemetry reporting via IOCTLs.
>>>>
>>>> The motivation is to provide a stable interface for user-space tools to
>>>> discover PMF features and consume selected metrics. Enable smoother
>>>> integration with AMD SystemDeck
>>>
>>> This does not really explain why you've chosen for a new character-device
>>> with IOCTLs instead of sysfs where as so far (AFAICT) all the AMD PMF code
>>> has been using sysfs APIs.
>>>
>>> Is there any specific reason why to switch to IOCTLs all of a sudden?
>>>
>>> Note that:
>>>
>>> 1. sysfs APIs can be (and must be) stable too, sysfs APIs are not allowed
>>> to be changed once shipped in a stable kernel.
>>> 2. sysfs attributes can be used with poll() to so if you want to do
>>> notifications of changes that can be done through sysfs too.
>>>
>>> Note I'm not saying you must use sysfs, but so far the PMF code has been
>>> using sysfs everywhere and this new IOCTL based API is not really consistent
>>> with this.
>>
>> Isn't there only one sysfs file for turning on/off CNQF?
> 
> Ah yes you're right somehow I thought there were more.
> 
> Still generally speaking the kernel community is trying to avoid
> adding new ioctl based interfaces / adding random new char devices
> in preference of using sysfs interface where possible.
> 
> So I've taken a better look at the actual ioctl interface
> and it seems like a really weird multiplexer interface,
> where there is only 2 ioctl commands and then the argument
> gets 1 of a ton of possible feature flags resp. info variables.
> 
> Where it also seems that none of these variables require
> a round-trip to the hardware.
> 
> Given the amount of different variables I can see some sense
> in having this as an ioctl interface, but why do the whole
> thing where userspace has to make ioctl per value it wants
> to read. That feels very sysfs-ish if you want that maybe
> just use sysfs ?
> 
> I would define a uAPI struct like this:
> 
> struct foo {
> 	u64 size;		/* in + out, all other fields out only */
> 	u64 features_supported; /* bitmask with feature info from patch 1/5 */
> 	u64 feature_version	/* from patch 1/5 */
> 	u64 power_source;	/* from patch 2/5 */
> 	...
> 	u64 bios_input[10];	/* from patch 2/5 */
> 	...
> 	etc.
> };
> 

thank you for your remarks and the UAPI struct suggestion. Let me make
this change and come back with a newer version.

> And have a copy of this struct embedded in the driver
> data struct and keep that updated (replacing the cache
> stuff) so that you can just copy_to_user that on the ioctl.
> 
> Combined with a single get-info ioctl which just fills 
> the struct, using the min of the size passed in by userspace
> + the size supported by the kernel to determine how much
> to copy and set the copied size in the struct passed
> back to userspace (to indicate for new userspace on
> old kernel that the new fields are not set).
> 
> This way for future extensions new fields can be added to
> the end of the struct and the size handling will automatically
> do the right thing.
> 
> As for Ilpo's comment about the battery info being duplicate
> with /sys/class/power_supply/BAT*, where is this info coming
> from ?  Is this a PMF specific view of the battery info,
> IOW it might be different then the power_supply calls info?
> 

with regards to this:

The function amd_pmf_get_battery_info() fills in the data that will be
sent to the TA. Inside this function, it calls
amd_pmf_get_battery_prop() to retrieve each required battery property.

For example:

/* to get the battery percentage */
in->ev_info.bat_percentage =
amd_pmf_get_battery_prop(POWER_SUPPLY_PROP_CAPACITY);


Because the PMF already obtains battery information from the
power_supply, amd_pmf_get_battery_info() simply transfers those values
into the ioctl structure so they can be returned to user space via
copy_to_user(). To do this we map the ioctl requests to the
corresponding fields, for example:

+	case IOCTL_DESIGNED_BATTERY_CAPACITY:
+		output.val = in->ev_info.bat_design;
+		break;
+	case IOCTL_FULLY_CHARGED_BATTERY_CAPACITY:
+		output.val = in->ev_info.full_charge_capacity;
+		break;
+	case IOCTL_BATTERY_DRAIN_RATE:
+		output.val = in->ev_info.drain_rate;
+		break;


> Note stil not a fan of adding new IOCTLs but given the large
> amount of variables I can see how using an IOCTL might be
> better then adding a ton of new sysfs attributes.
> 

Thank you..

Thanks,
Shyam

> Regards,
> 
> Hans
> 
> 
> 
> 
>>>> , a widely used tool for monitoring and
>>>> controlling power and thermal behavior, helping designers keep components
>>>> within thermal limits to ensure proper operation and improve system
>>>> stability and reliability.
>>>>
>>>> This series also adds a small, dependable userspace utility that leverages
>>>> the new IOCTLs to query live power and thermal telemetry. Exposing this
>>>> data in a scriptable interface helps users and tooling make informed,
>>>> workload-aware decisions and supports validation and debugging.
>>>>
>>>> v2:
>>>> ----
>>>>   - address remarks from v1
>>>>   - add a new tool that exercises the IOCTLs from PMF interface
>>>>
>>>> Shyam Sundar S K (5):
>>>>    platform/x86/amd/pmf: add util layer and user-space misc device
>>>>      interface
>>>>    platform/x86/amd/pmf: cache BIOS output values for user-space
>>>>      telemetry via util IOCTL
>>>>    Documentation/ABI: add testing entry for AMD PMF misc device interface
>>>>    platform/x86/amd/pmf: Store commonly used enums in the header file
>>>>    platform/x86/amd/pmf: Introduce AMD PMF testing tool for driver
>>>>      metrics and features
>>>>
>>>>   .../ABI/testing/misc-amdpmf_interface         |  49 +++
>>>>   MAINTAINERS                                   |   1 +
>>>>   drivers/platform/x86/amd/pmf/Kconfig          |  10 +
>>>>   drivers/platform/x86/amd/pmf/Makefile         |   2 +
>>>>   drivers/platform/x86/amd/pmf/core.c           |  19 +
>>>>   drivers/platform/x86/amd/pmf/pmf.h            |  33 +-
>>>>   drivers/platform/x86/amd/pmf/spc.c            |   1 +
>>>>   drivers/platform/x86/amd/pmf/tee-if.c         |  10 +
>>>>   drivers/platform/x86/amd/pmf/util.c           | 236 +++++++++++
>>>>   include/uapi/linux/amd-pmf.h                  |  96 +++++
>>>>   tools/testing/selftests/Makefile              |   1 +
>>>>   .../drivers/platform/x86/amd/pmf/Makefile     |   8 +
>>>>   .../drivers/platform/x86/amd/pmf/test_pmf.c   | 388 ++++++++++++++++++
>>>>   13 files changed, 832 insertions(+), 22 deletions(-)
>>>>   create mode 100644 Documentation/ABI/testing/misc-amdpmf_interface
>>>>   create mode 100644 drivers/platform/x86/amd/pmf/util.c
>>>>   create mode 100644 include/uapi/linux/amd-pmf.h
>>>>   create mode 100644 tools/testing/selftests/drivers/platform/x86/amd/pmf/Makefile
>>>>   create mode 100644 tools/testing/selftests/drivers/platform/x86/amd/pmf/test_pmf.c
>>>>
>>>
>>
>