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

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

On Mon, 8 Jun 2026, Hans de Goede wrote:
> On 27-May-26 4:02 PM, 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
> > - Export amd_pmf_get_ta_custom_bios_inputs()
> > 
> > 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/pmf.h  |   1 +
> >  drivers/platform/x86/amd/pmf/spc.c  |   3 +-
> >  drivers/platform/x86/amd/pmf/util.c | 105 +++++++++++++++++++++++++++-
> >  3 files changed, 107 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> > index 269c0a4b1cae..752fa5dd2267 100644
> > --- a/drivers/platform/x86/amd/pmf/pmf.h
> > +++ b/drivers/platform/x86/amd/pmf/pmf.h
> > @@ -903,6 +903,7 @@ int amd_pmf_smartpc_apply_bios_output(struct amd_pmf_dev *dev, u32 val, u32 preq
> >  void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in);
> >  void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in);
> >  int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev);
> > +u32 amd_pmf_get_ta_custom_bios_inputs(struct ta_pmf_enact_table *in, int index);
> >  
> >  int amd_pmf_tee_init(struct amd_pmf_dev *dev, const uuid_t *uuid);
> >  void amd_pmf_tee_deinit(struct amd_pmf_dev *dev);
> > diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
> > index 6e33824ccadc..94355b435a66 100644
> > --- a/drivers/platform/x86/amd/pmf/spc.c
> > +++ b/drivers/platform/x86/amd/pmf/spc.c
> > @@ -18,7 +18,7 @@
> >  #include "pmf.h"
> >  
> >  #ifdef CONFIG_AMD_PMF_DEBUG
> > -static u32 amd_pmf_get_ta_custom_bios_inputs(struct ta_pmf_enact_table *in, int index)
> > +u32 amd_pmf_get_ta_custom_bios_inputs(struct ta_pmf_enact_table *in, int index)
> >  {
> >  	switch (index) {
> >  	case 0 ... 1:
> > @@ -29,6 +29,7 @@ static u32 amd_pmf_get_ta_custom_bios_inputs(struct ta_pmf_enact_table *in, int
> >  		return 0;
> >  	}
> >  }
> > +EXPORT_SYMBOL(amd_pmf_get_ta_custom_bios_inputs);
> >  
> >  void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> >  {
> > diff --git a/drivers/platform/x86/amd/pmf/util.c b/drivers/platform/x86/amd/pmf/util.c
> > index f8a283192ffe..30fb5c250362 100644
> > --- a/drivers/platform/x86/amd/pmf/util.c
> > +++ b/drivers/platform/x86/amd/pmf/util.c
> > @@ -9,6 +9,7 @@
> >   *	    Sanket Goswami <Sanket.Goswami@amd.com>
> >   */
> >  
> > +#include <linux/align.h>
> >  #include <linux/amd-pmf.h>
> >  #include <linux/miscdevice.h>
> >  #include <linux/mutex.h>
> > @@ -19,9 +20,111 @@
> >  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;
> > +
> > +	if (!pdev->shbuf)
> > +		return -EINVAL;
> > +
> > +	ta_sm = pdev->shbuf;
> > +	in = &ta_sm->pmf_input.enact_table;
> > +
> > +	/* Set size */
> > +	info->size = sizeof(*info);
> > +
> > +	/* 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;
> > +
> > +	/* Custom BIOS input parameters */
> > +	for (idx = 0; idx < AMD_PMF_BIOS_PARAMS_MAX; idx++)
> > +		info->bios_input[idx] = amd_pmf_get_ta_custom_bios_inputs(in, idx);
> > +
> > +	/* 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;
> > +
> > +	if (user_size > sizeof(info))
> > +		return -EINVAL;
> 
> Why? If the struct ever gets extended and a newer userspace runs on
> an older kernel this will now trigger. Instead just clamp to
> sizeof(info) .

If this is not here, some of the content userspace wanted will not be 
there so part of the struct member will not be filled. Userspace needs to 
be extremely careful which fields it can access in such a case.

> > +	if (!IS_ALIGNED(user_size, sizeof(__u64)))
> > +		return -EINVAL;
> 
> Why ?  I guess on x86_64 this will always be true, but what about
> i386 ?

user_size comes from userspace so it could be anything userspace put 
there and is unrelated to the arch. It just doesn't look useful to copy 
partial fields if userspace gives a strange user_size. Of course it never 
happens if userspace uses sizeof() to fill the size in.

> More specifically this simply seems unnecessary and I believe this
> entire check can be dropped.
>
> > +	if (user_size > sizeof(__u64)) {
> 
> This should be:
> 
> if (user_size >= (offsetof(struct amd_pmf_info, features_supported) + sizeof(features_from_user))) {
> 
> > +		__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;
> 
> Looking at amd_pmf_populate_data() features_supported is purely a kernel -> user thing,
> so why read this from userspace at all ?  And why must it be non 0 ?
>
> I believe this entire block can be dropped (instead of fixing the if condition).

Okay. Perhaps you're right and this should not be added.

My thinking when suggesting this was that they'll eventually come up 
something that is every expensive to get and then want to indicate it's 
not required to get it every time. By checking the feature flags right 
from the start like this, such an extension would be easy to add later.

And as per the code says, the non-zero feature values lead to -EINVAL, not 
the other way around.


-- 
 i.