Re: [PATCH v5 6/8] platform/x86/amd/pmf: Implement util layer ioctl handler

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

On 5/21/2026 16:28, Ilpo Järvinen wrote:
> On Thu, 21 May 2026, Shyam Sundar S K wrote:
> 
>> Implement the ioctl handler for the util layer character device. This
>> support adds the actual functionality to populate PMF metrics from the
>> TA shared memory buffer and return them to userspace.
>>
>> The implementation includes:
>> - amd_pmf_populate_data() to extract metrics from TA shared memory
>> - amd_pmf_set_ioctl() to handle userspace ioctl requests
>> - Size negotiation for forward/backward compatibility
>> - Feature-based population of struct fields
>>
>> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>  drivers/platform/x86/amd/pmf/util.c | 90 ++++++++++++++++++++++++++++-
>>  1 file changed, 89 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/util.c b/drivers/platform/x86/amd/pmf/util.c
>> index 0052f0b6a7a5..eb9a02e135a9 100644
>> --- a/drivers/platform/x86/amd/pmf/util.c
>> +++ b/drivers/platform/x86/amd/pmf/util.c
>> @@ -19,9 +19,97 @@
>>  static struct amd_pmf_dev *pmf_dev_handle;
>>  static DEFINE_MUTEX(pmf_util_lock);
>>  
>> +static int amd_pmf_populate_data(struct amd_pmf_dev *pdev, struct amd_pmf_info *info)
>> +{
>> +	struct ta_pmf_shared_memory *ta_sm = NULL;
>> +	struct ta_pmf_enact_table *in = NULL;
>> +	int idx;
>> +
>> +	if (!pdev || !info)
>> +		return -EINVAL;
>> +
>> +	ta_sm = pdev->shbuf;
>> +	in = &ta_sm->pmf_input.enact_table;
>> +
>> +	/* Set size and version */
>> +	info->size = sizeof(struct amd_pmf_info);
> 
> sizeof(*info)
> 
> version ???
> 
>> +
>> +	/* PMF Feature support flags */
>> +	if (is_apmf_func_supported(pdev, APMF_FUNC_AUTO_MODE))
>> +		info->features_supported |= AMD_PMF_FEAT_AUTO_MODE;
>> +	if (is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR))
>> +		info->features_supported |= AMD_PMF_FEAT_STATIC_POWER_SLIDER;
>> +	if (pdev->smart_pc_enabled)
>> +		info->features_supported |= AMD_PMF_FEAT_POLICY_BUILDER;
>> +	if (is_apmf_func_supported(pdev, APMF_FUNC_DYN_SLIDER_AC))
>> +		info->features_supported |= AMD_PMF_FEAT_DYNAMIC_POWER_SLIDER_AC;
>> +	if (is_apmf_func_supported(pdev, APMF_FUNC_DYN_SLIDER_DC))
>> +		info->features_supported |= AMD_PMF_FEAT_DYNAMIC_POWER_SLIDER_DC;
>> +
>> +	/* Device States */
>> +	info->platform_type = in->ev_info.platform_type;
>> +	info->laptop_placement = in->ev_info.device_state;
>> +	info->lid_state = in->ev_info.lid_state;
>> +	info->user_presence = in->ev_info.user_present;
>> +	info->slider_position = in->ev_info.power_slider;
>> +
>> +	/* Thermal and Power Metrics */
>> +	info->power_source = in->ev_info.power_source;
>> +	info->skin_temp = in->ev_info.skin_temperature;
>> +	info->gfx_busy = in->ev_info.gfx_busy;
>> +	info->ambient_light = in->ev_info.ambient_light;
>> +	info->avg_c0_residency = in->ev_info.avg_c0residency;
>> +	info->max_c0_residency = in->ev_info.max_c0residency;
>> +	info->socket_power = in->ev_info.socket_power;
> 
> I've no big problem with this, though I seem to now recall Hans also was 
> suggesting the in-kernel data would be layouted such that this copy would 
> be easier (but please check).
> 
> In any case, my plan is to ask Hans to check the next version of this 
> series now that it will be hopefully ready/almost ready.

Sure. We can gather feedback from Hans once you are comfortable with
the changes in v6.

> 
>> +	/* Custom BIOS input parameters */
>> +	for (idx = 0; idx < AMD_PMF_BIOS_PARAMS_MAX; idx++) {
>> +		if (idx < 2)
>> +			info->bios_input[idx] = in->ev_info.bios_input_1[idx];
>> +		else
>> +			info->bios_input[idx] = in->ev_info.bios_input_2[idx - 2];
> 
> This seems to duplicate amd_pmf_get_ta_custom_bios_inputs().

Not sure how they are duplicates..

amd_pmf_get_ta_custom_bios_inputs() -> just returns the index of the
bios input

But, here in this code snippet, we store the values of the bios inputs
to propagate back to the userland.

> 
>> +	}
>> +
>> +	/* BIOS output parameters */
>> +	for (idx = 0; idx < AMD_PMF_BIOS_PARAMS_MAX; idx++)
>> +		info->bios_output[idx] = pdev->bios_output[idx];
>> +
>> +	return 0;
>> +}
>> +
>>  static long amd_pmf_set_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>  {
>> -	return -ENOTTY;
>> +	struct amd_pmf_dev *pdev = filp->private_data;
>> +	void __user *argp = (void __user *)arg;
>> +	struct amd_pmf_info info = {};
>> +	size_t copy_size;
>> +	__u64 user_size;
>> +	int ret;
>> +
>> +	if (cmd != IOCTL_AMD_PMF_POPULATE_DATA)
>> +		return -ENOTTY;
>> +
>> +	/* First read just the size field from userspace */
>> +	if (copy_from_user(&user_size, argp, sizeof(user_size)))
>> +		return -EFAULT;
> 
> Just an interface though, if features are ever needed as userspace -> 
> kernel comminucation channel, it should be handled properly right from 
> the start, with -EINVAL being returned for invalid (unknown) values.
> 
> This is not meant to say, you must act on this, you have better idea how 
> this interface may evolve over the years than I do. But if an ability to 
> query a set of features only would be useful at some point, it would be 
> beneficial to take account now as we cannot change it later to not break
> ABI rules (if userspace passes struct that has only user_size initalized 
> and pseudogarbage in ->features, we cannot start to return -EINVAL because 
> of that later).

The thought process was to have an interface where the userspace can
query what features are/were supported and as unidirectional
communication channel.

But, we never know what might actually come up in future. So, would
like to address your comment now.

Can you please check if this is what you are expecting?

if (user_size > sizeof(__u64)) {
    __u32 features_from_user = 0;

    if (copy_from_user(&features_from_user, argp + offsetof(struct
amd_pmf_info, features_supported),
sizeof(features_from_user)))
        return -EFAULT;

    // Reject non-zero values NOW
    if (features_from_user != 0)
        return -EINVAL;  // ← Forces userspace to zero-initialize!
}

Kindly let me know if you think otherwise.

> 
>> +	if (user_size & (sizeof(__u64) - 1))
> 
> Please use IS_ALIGNED() + check you have the include for it.

Ack.

> 
> I wonder if user_size > sizeof(*info) is a bit dangerous condition and 
> should also result in -EFAULT. It result in leaving the rest of the struct 
> uninitialized (from userspace's PoV) when userspace and this kernel 
> version disagree what's the size of the struct.
> 

So,

+	if (!IS_ALIGNED(user_size, sizeof(__u64)))
+		return -EINVAL;

Should help address your comment right?

If no, I think I am missing your feedback. Can you elaborate?

>> +		return -EINVAL;
>> +
>> +	guard(mutex)(&pmf_util_lock);
>> +	ret = amd_pmf_populate_data(pdev, &info);
>> +	if (ret)
>> +		return ret;
>> +
>> +	copy_size = min_t(size_t, user_size, sizeof(struct amd_pmf_info));
> 
> sizeof(*info)

Ack.

Thanks,
Shyam

> 
>> +
>> +	/* Set actual size being copied */
>> +	info.size = copy_size;
>> +
>> +	if (copy_to_user(argp, &info, copy_size))
>> +		return -EFAULT;
>> +
>> +	return 0;
>>  }
>>  
>>  static int amd_pmf_open(struct inode *inode, struct file *filp)
>>
>