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

[Hans de Goede <hansg@kernel.org>]

Hi,

On 20-Nov-25 08:49, Shyam Sundar S K wrote:
> 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);

Ok, so this is duplicating the information from /sys/class/power_supply .

Is there a reason why your userspace app cannot just directly get
these values from /sys/class/power_supply ?

I guess it might be convenient to have this all in the same ioctl,
I understand that the PMF code already needs to retrieve it regardless.

Maybe just have an ioctl which just memcpy-s the entire
ta_pmf_condition_info struct to userspace?  I think that is already
part of the kernel <-> PMF-firmware API so making that struct uAPI
should be fine since it is already something which cannot be changed
because it is also firmware API ?

It seems just being able to copy the cached ev_info the driver
already has to userspace is better then defining a new struct with
similar fields and needing to manually copy over everything field
by field ?

And then instead of an ioctl it could even just be a binary sysfs
file, avoiding the need to add a somewhat random new chardev
to /dev  ?

Regards,

Hans