Re: [PATCH 1/3] platform/x86/amd/pmf: add util layer and user-space misc device interface

[Mario Limonciello <mario.limonciello@amd.com>]

On 10/17/2025 5:25 AM, Shyam Sundar S K wrote:
> Add a util layer to AMD PMF that exposes a minimal user-space interface
> via a misc character device for metrics monitoring and feature discovery.
> This creates /dev/amdpmf_interface with a basic ioctl to report support
> for PMF features (Smart PC Builder, Auto Mode, Static Power Slider,
> Dynamic Power Slider AC/DC), enabling smoother integration with
> user-space tools such as AMD SystemDeck[1].
> 
> AMD SystemDeck is a widely used user-space tool for monitoring and
> controlling power and thermal behavior. It helps designers keep major
> components within their thermal limits to ensure proper operation.
> Maintaining temperatures within specification enhances overall system
> stability and reliability.
> 
> [1] https://docs.amd.com/v/u/en-US/68773_0.50

This should be

Link: https://docs.amd.com/v/u/en-US/68773_0.50 [1]

> 
> 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/Kconfig  |  10 +++
>   drivers/platform/x86/amd/pmf/Makefile |   2 +
>   drivers/platform/x86/amd/pmf/core.c   |  19 +++++
>   drivers/platform/x86/amd/pmf/pmf.h    |  10 +++
>   drivers/platform/x86/amd/pmf/util.c   | 115 ++++++++++++++++++++++++++
>   5 files changed, 156 insertions(+)
>   create mode 100644 drivers/platform/x86/amd/pmf/util.c
> 
> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
> index 25b8f7ae3abd..81ea4934e199 100644
> --- a/drivers/platform/x86/amd/pmf/Kconfig
> +++ b/drivers/platform/x86/amd/pmf/Kconfig
> @@ -30,3 +30,13 @@ config AMD_PMF_DEBUG
>   	 in the PMF config store.
>   
>   	 Say Y here to enable more debug logs and Say N here if you are not sure.
> +
> +config AMD_PMF_UTIL_SUPPORT
> +	bool "AMD PMF Util layer support"
> +	depends on AMD_PMF
> +	help
> +	Enabling this option permits the driver to interface with user
> +	space for capturing AMD PMF metrics and updating them to the
> +	user space tool for smooth monitoring.
> +
> +	Say Y here to enable it and Say N here if you are not sure.
> diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile
> index 5978464e0eb7..bf7aad80b9e9 100644
> --- a/drivers/platform/x86/amd/pmf/Makefile
> +++ b/drivers/platform/x86/amd/pmf/Makefile
> @@ -8,3 +8,5 @@ obj-$(CONFIG_AMD_PMF)		+= amd-pmf.o
>   amd-pmf-y 			:= core.o acpi.o sps.o \
>   				   auto-mode.o cnqf.o \
>   				   tee-if.o spc.o
> +# Build util.c only when AMD_PMF_UTIL_SUPPORT is enabled
> +amd-pmf-$(CONFIG_AMD_PMF_UTIL_SUPPORT) += util.o
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index bc544a4a5266..ac2b80db019e 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -53,6 +53,19 @@ static bool force_load;
>   module_param(force_load, bool, 0444);
>   MODULE_PARM_DESC(force_load, "Force load this driver on supported older platforms (experimental)");
>   
> +#if IS_ENABLED(CONFIG_AMD_PMF_UTIL_SUPPORT)
> +struct amd_pmf_dev *pmf_handle;
> +
> +struct amd_pmf_dev *amd_pmf_get_handle(void)
> +{
> +	struct amd_pmf_dev *pmf = pmf_handle;
> +
> +	return pmf;
> +}
> +#else
> +struct amd_pmf_dev *amd_pmf_get_handle(void) { return NULL; }
> +#endif
> +
>   static int amd_pmf_pwr_src_notify_call(struct notifier_block *nb, unsigned long event, void *data)
>   {
>   	struct amd_pmf_dev *pmf = container_of(nb, struct amd_pmf_dev, pwr_src_notifier);
> @@ -465,6 +478,10 @@ static int amd_pmf_probe(struct platform_device *pdev)
>   	if (!dev->regbase)
>   		return -ENOMEM;
>   
> +#if IS_ENABLED(CONFIG_AMD_PMF_UTIL_SUPPORT)
> +	pmf_handle = dev;
> +#endif
> +
>   	mutex_init(&dev->lock);
>   	mutex_init(&dev->update_mutex);
>   	mutex_init(&dev->cb_mutex);
> @@ -477,6 +494,7 @@ static int amd_pmf_probe(struct platform_device *pdev)
>   	if (is_apmf_func_supported(dev, APMF_FUNC_SBIOS_HEARTBEAT_V2))
>   		amd_pmf_notify_sbios_heartbeat_event_v2(dev, ON_LOAD);
>   
> +	amd_pmf_cdev_register();
>   	dev_info(dev->dev, "registered PMF device successfully\n");
>   
>   	return 0;
> @@ -489,6 +507,7 @@ static void amd_pmf_remove(struct platform_device *pdev)
>   	amd_pmf_deinit_features(dev);
>   	if (is_apmf_func_supported(dev, APMF_FUNC_SBIOS_HEARTBEAT_V2))
>   		amd_pmf_notify_sbios_heartbeat_event_v2(dev, ON_UNLOAD);
> +	amd_pmf_cdev_unregister();
>   	apmf_acpi_deinit(dev);
>   	amd_pmf_dbgfs_unregister(dev);
>   	mutex_destroy(&dev->lock);
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index bd19f2a6bc78..3b1aae8a60a5 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -823,6 +823,7 @@ struct ta_pmf_shared_memory {
>   };
>   
>   /* Core Layer */
> +struct amd_pmf_dev *amd_pmf_get_handle(void);
>   int apmf_acpi_init(struct amd_pmf_dev *pmf_dev);
>   void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev);
>   int is_apmf_func_supported(struct amd_pmf_dev *pdev, unsigned long index);
> @@ -888,4 +889,13 @@ void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_tab
>   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);
>   
> +/* Util Layer */
> +#if IS_ENABLED(CONFIG_AMD_PMF_UTIL_SUPPORT)
> +int amd_pmf_cdev_register(void);
> +void amd_pmf_cdev_unregister(void);
> +#else
> +static inline int amd_pmf_cdev_register(void) { return 0; }
> +static inline void amd_pmf_cdev_unregister(void) {}
> +#endif
> +
>   #endif /* PMF_H */
> diff --git a/drivers/platform/x86/amd/pmf/util.c b/drivers/platform/x86/amd/pmf/util.c
> new file mode 100644
> index 000000000000..483e4531535e
> --- /dev/null
> +++ b/drivers/platform/x86/amd/pmf/util.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * AMD Platform Management Framework Util Layer
> + *
> + * Copyright (c) 2025, Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> + *	    Sanket Goswami <Sanket.Goswami@amd.com>
> + */
> +
> +#include <linux/miscdevice.h>
> +#include <linux/uaccess.h>
> +
> +#include "pmf.h"
> +
> +#define IOCTL_PMF_QUERY_FEATURE_SUPPORT		0
> +
> +/* AMD PMF Feature IDs */
> +#define PMF_FEATURE_AUTO_MODE		0
> +#define PMF_FEATURE_STATIC_POWER_SLIDER		2
> +#define PMF_FEATURE_POLICY_BUILDER		4
> +#define PMF_FEATURE_DYNAMIC_POWER_SLIDER_AC	5
> +#define PMF_FEATURE_DYNAMIC_POWER_SLIDER_DC	6
> +
> +#define AMD_PMF_FEATURE_VERSION         0x02
> +
> +struct amd_pmf_feature_support {
> +	bool feature_supported;
> +	unsigned long feature_version;
> +	unsigned long feature_id;
> +};
> +
> +static int amd_pmf_get_feature_status(void __user *argp, unsigned long feat_id)
> +{
> +	struct amd_pmf_feature_support info;

As nothing populates feature_id, I think you should explicitly 
initialize this structure so garbage doesn't get passed back.  If all 
the commands in the future will reply with AMD_PMF_FEATURE_VERSION 
identically you can set it here too when initializing the structure.

> +	struct amd_pmf_dev *pdev;
> +
> +	pdev = amd_pmf_get_handle();
> +	if (!pdev)
> +		return -EINVAL;
> +
> +	switch (feat_id) {
> +	case PMF_FEATURE_AUTO_MODE:
> +		info.feature_supported = is_apmf_func_supported(pdev, APMF_FUNC_AUTO_MODE);
> +		break;
> +	case PMF_FEATURE_STATIC_POWER_SLIDER:
> +		info.feature_supported = is_apmf_func_supported(pdev,
> +								APMF_FUNC_STATIC_SLIDER_GRANULAR);
> +		break;
> +	case PMF_FEATURE_POLICY_BUILDER:
> +		info.feature_supported = pdev->smart_pc_enabled;
> +		break;
> +	case PMF_FEATURE_DYNAMIC_POWER_SLIDER_AC:
> +		info.feature_supported = is_apmf_func_supported(pdev, APMF_FUNC_DYN_SLIDER_AC);
> +		break;
> +	case PMF_FEATURE_DYNAMIC_POWER_SLIDER_DC:
> +		info.feature_supported = is_apmf_func_supported(pdev, APMF_FUNC_DYN_SLIDER_DC);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	info.feature_version = AMD_PMF_FEATURE_VERSION;
> +	if (copy_to_user(argp, &info, sizeof(info)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static long amd_pmf_set_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> +	void __user *argp = (void __user *)arg;
> +	struct amd_pmf_feature_support output;
> +	int ret = -ENOTTY;

I don't see a need to initialize ret here when you set it on the next line.

> +
> +	ret = copy_from_user(&output, argp, sizeof(output));
> +	if (ret)
> +		return -EFAULT;

Shouldn't you return ret here?  If you always want to return -EFAULT and 
throw it away you should instead do:

if (copy_from_user())
	return -EFAULT;

> +
> +	switch (cmd) {
> +	case IOCTL_PMF_QUERY_FEATURE_SUPPORT:
> +		ret = amd_pmf_get_feature_status(argp, output.feature_id);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return ret;

If you're not planning on doing any other processing for future commands 
you can just instead do this:

	switch (cmd) {
	case foo:
		return bar();
	default:
		return -EINVAL;
	}
}

> +}
> +
> +static const struct file_operations pmf_if_ops = {
> +	.unlocked_ioctl = amd_pmf_set_ioctl,
> +};
> +
> +#if IS_ENABLED(CONFIG_AMD_PMF_UTIL_SUPPORT)
> +static struct miscdevice amd_pmf_util_if = {
> +	.minor		= MISC_DYNAMIC_MINOR,
> +	.name		= "amdpmf_interface",
> +	.fops		= &pmf_if_ops,
> +};
> +
> +int amd_pmf_cdev_register(void)
> +{
> +	return misc_register(&amd_pmf_util_if);
> +}
> +
> +void amd_pmf_cdev_unregister(void)
> +{
> +	misc_deregister(&amd_pmf_util_if);
> +}
> +#else
> +int amd_pmf_cdev_register(void) { return 0; }
> +void amd_pmf_cdev_unregister(void) {}
> +#endif