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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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.

> +	/* 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().

> +	}
> +
> +	/* 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).

> +	if (user_size & (sizeof(__u64) - 1))

Please use IS_ALIGNED() + check you have the include for it.

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.

> +		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)

> +
> +	/* 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)
> 

-- 
 i.