When cpufreq driver in cppc passive mode, it has both cppc and governor
info. We need to invoke two sysctl sub-ops ("get-cpufreq-cppc" and
"get-cpufreq-para") to produce both info.

A new helper cpufreq_in_cppc_passive_mode() is introduced to tell whether
cpufreq driver supports cppc passive mode.

Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v5 -> v6
- new commit
---
 xen/drivers/acpi/pm-op.c           | 10 +++++++++-
 xen/drivers/cpufreq/cpufreq.c      |  6 ++++++
 xen/include/acpi/cpufreq/cpufreq.h |  2 ++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/acpi/pm-op.c b/xen/drivers/acpi/pm-op.c
index 0723cea34c..077efdfc5c 100644
--- a/xen/drivers/acpi/pm-op.c
+++ b/xen/drivers/acpi/pm-op.c
@@ -152,7 +152,15 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
     else
         strlcpy(op->u.get_para.scaling_driver, "Unknown", CPUFREQ_NAME_LEN);
 
-    ret = get_cpufreq_cppc(op->cpuid, &op->u.get_para.u.cppc_para);
+    /*
+     * When cpufreq driver in cppc passive mode, it has both cppc and governor
+     * info. Users could only rely on "get-cpufreq-cppc" to acquire CPPC info.
+     * And it returns governor info in "get-cpufreq-para"
+     */
+    if ( cpufreq_in_cppc_passive_mode(op->cpuid) )
+        ret = -ENODEV;
+    else
+        ret = get_cpufreq_cppc(op->cpuid, &op->u.get_para.u.cppc_para);
     if ( ret == -ENODEV )
     {
         if ( !(scaling_available_governors =
diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
index cf1fcf1d22..431f2903f8 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -962,3 +962,9 @@ int __init cpufreq_register_driver(const struct cpufreq_driver *driver_data)
 
     return 0;
 }
+
+bool cpufreq_in_cppc_passive_mode(unsigned int cpuid)
+{
+    return processor_pminfo[cpuid]->init & XEN_CPPC_INIT &&
+           cpufreq_driver.target;
+}
diff --git a/xen/include/acpi/cpufreq/cpufreq.h b/xen/include/acpi/cpufreq/cpufreq.h
index b0b22d1c9c..dd55d268c0 100644
--- a/xen/include/acpi/cpufreq/cpufreq.h
+++ b/xen/include/acpi/cpufreq/cpufreq.h
@@ -295,4 +295,6 @@ int acpi_cpufreq_register(void);
 int amd_cppc_cmdline_parse(const char *s, const char *e);
 int amd_cppc_register_driver(void);
 
+bool cpufreq_in_cppc_passive_mode(unsigned int cpuid);
+
 #endif /* __XEN_CPUFREQ_PM_H__ */
-- 
2.34.1

On 11.07.2025 05:51, Penny Zheng wrote:
> --- a/xen/drivers/acpi/pm-op.c
> +++ b/xen/drivers/acpi/pm-op.c
> @@ -152,7 +152,15 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>      else
>          strlcpy(op->u.get_para.scaling_driver, "Unknown", CPUFREQ_NAME_LEN);
>  
> -    ret = get_cpufreq_cppc(op->cpuid, &op->u.get_para.u.cppc_para);
> +    /*
> +     * When cpufreq driver in cppc passive mode, it has both cppc and governor
> +     * info. Users could only rely on "get-cpufreq-cppc" to acquire CPPC info.
> +     * And it returns governor info in "get-cpufreq-para"
> +     */

Which of the two they need to invoke to obtain a complete picture? Both?
I'm confused by you bypassing get_cpufreq_cppc() (i.e. get_hwp_para())
for yet another reason, when - aiui - some information there is relevant
in both active and passive modes.

> +    if ( cpufreq_in_cppc_passive_mode(op->cpuid) )
> +        ret = -ENODEV;
> +    else
> +        ret = get_cpufreq_cppc(op->cpuid, &op->u.get_para.u.cppc_para);

Any reason the extra check isn't put in get_cpufreq_cppc(), alongside the
hwp_active() one?

> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -962,3 +962,9 @@ int __init cpufreq_register_driver(const struct cpufreq_driver *driver_data)
>  
>      return 0;
>  }
> +
> +bool cpufreq_in_cppc_passive_mode(unsigned int cpuid)
> +{
> +    return processor_pminfo[cpuid]->init & XEN_CPPC_INIT &&

Nit: Please use parentheses when using & and && together.

Also, isn't this function going to become unreachable when PM_OP=n, thus
violating Misra rule 2.1?

Jan