Re: [PATCH] cpufreq: intel_pstate: Request P-states control from SMM if needed

From: Srinivas Pandruvada
Date: Wed Nov 16 2016 - 19:05:00 EST


On Wed, 2016-11-16 at 03:36 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> Currently, intel_pstate is unable to control P-states on my
> IvyBridge-based Acer Aspire S5, because they are controlled by SMM
> on that machine by default and it is necessary to request OS control
> of P-states from it via the SMI Command register exposed in the ACPI
> FADT.ÂÂintel_pstate doesn't do that now, but acpi-cpufreq and other
> cpufreq drivers for x86 platforms do.
>
> Address this problem by making intel_pstate use the ACPI-defined
> mechanism as well.ÂÂHowever, intel_pstate is not modular and it
> doesn't need the module refcount tricks played by
> acpi_processor_notify_smm(), so export the core of this function
> to it as acpi_processor_pstate_control() and make it call that.
> [The changes in processor_perflib.c related to this should not
> make any functional difference for the acpi_processor_notify_smm()
> users].
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> Suggested-by: Srinivas Pandruvada
> <srinivas.pandruvada@xxxxxxxxxxxxxxx>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>

> ---
> Âdrivers/acpi/processor_perflib.c |ÂÂÂ45 ++++++++++++++++++++++++--
> -------------
> Âdrivers/cpufreq/intel_pstate.cÂÂÂ|ÂÂÂÂ8 ++++++
> Âinclude/acpi/processor.hÂÂÂÂÂÂÂÂÂ|ÂÂÂÂ1Â
> Â3 files changed, 37 insertions(+), 17 deletions(-)
>
> Index: linux-pm/drivers/acpi/processor_perflib.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/processor_perflib.c
> +++ linux-pm/drivers/acpi/processor_perflib.c
> @@ -465,11 +465,33 @@ int acpi_processor_get_performance_info(
> Â return result;
> Â}
> ÂEXPORT_SYMBOL_GPL(acpi_processor_get_performance_info);
> -int acpi_processor_notify_smm(struct module *calling_module)
> +
> +int acpi_processor_pstate_control(void)
> Â{
> Â acpi_status status;
> - static int is_done = 0;
> Â
> + if (!acpi_gbl_FADT.smi_command ||
> !acpi_gbl_FADT.pstate_control)
> + return 0;
> +
> + ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> + ÂÂ"Writing pstate_control [0x%x] to
> smi_command [0x%x]\n",
> + ÂÂacpi_gbl_FADT.pstate_control,
> acpi_gbl_FADT.smi_command));
> +
> + status = acpi_os_write_port(acpi_gbl_FADT.smi_command,
> + ÂÂÂÂ(u32)acpi_gbl_FADT.pstate_contro
> l, 8);
> + if (ACPI_SUCCESS(status))
> + return 1;
> +
> + ACPI_EXCEPTION((AE_INFO, status,
> + "Failed to write pstate_control [0x%x] to
> smi_command [0x%x]",
> + acpi_gbl_FADT.pstate_control,
> acpi_gbl_FADT.smi_command));
> + return -EIO;
> +}
> +
> +int acpi_processor_notify_smm(struct module *calling_module)
> +{
> + static int is_done = 0;
> + int result;
> Â
> Â if (!(acpi_processor_ppc_status & PPC_REGISTERED))
> Â return -EBUSY;
> @@ -492,26 +514,15 @@ int acpi_processor_notify_smm(struct mod
> Â
> Â is_done = -EIO;
> Â
> - /* Can't write pstate_control to smi_command if either value
> is zero */
> - if ((!acpi_gbl_FADT.smi_command) ||
> (!acpi_gbl_FADT.pstate_control)) {
> + result = acpi_processor_pstate_control();
> + if (!result) {
> Â ACPI_DEBUG_PRINT((ACPI_DB_INFO, "No SMI port or
> pstate_control\n"));
> Â module_put(calling_module);
> Â return 0;
> Â }
> -
> - ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> - ÂÂ"Writing pstate_control [0x%x] to
> smi_command [0x%x]\n",
> - ÂÂacpi_gbl_FADT.pstate_control,
> acpi_gbl_FADT.smi_command));
> -
> - status = acpi_os_write_port(acpi_gbl_FADT.smi_command,
> - ÂÂÂÂ(u32)
> acpi_gbl_FADT.pstate_control, 8);
> - if (ACPI_FAILURE(status)) {
> - ACPI_EXCEPTION((AE_INFO, status,
> - "Failed to write pstate_control
> [0x%x] to "
> - "smi_command [0x%x]",
> acpi_gbl_FADT.pstate_control,
> - acpi_gbl_FADT.smi_command));
> + if (result < 0) {
> Â module_put(calling_module);
> - return status;
> + return result;
> Â }
> Â
> Â /* Success. If there's no _PPC, we need to fear nothing, so
> Index: linux-pm/include/acpi/processor.h
> ===================================================================
> --- linux-pm.orig/include/acpi/processor.h
> +++ linux-pm/include/acpi/processor.h
> @@ -249,6 +249,7 @@ extern int acpi_processor_register_perfo
> Â ÂÂÂÂÂÂÂ*performance,
> unsigned int cpu);
> Âextern void acpi_processor_unregister_performance(unsigned int cpu);
> Â
> +int acpi_processor_pstate_control(void);
> Â/* note: this locks both the calling module and the processor module
> ÂÂÂÂÂÂÂÂÂÂif a _PPC object exists, rmmod is disallowed then */
> Âint acpi_processor_notify_smm(struct module *calling_module);
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -2034,9 +2034,15 @@ static bool __init intel_pstate_platform
> Â
> Â return false;
> Â}
> +
> +static void intel_pstate_request_control(void)
> +{
> + acpi_processor_pstate_control();
> +}
> Â#else /* CONFIG_ACPI not enabled */
> Âstatic inline bool intel_pstate_platform_pwr_mgmt_exists(void) {
> return false; }
> Âstatic inline bool intel_pstate_has_acpi_ppc(void) { return false; }
> +static inline void intel_pstate_request_control(void) {}
> Â#endif /* CONFIG_ACPI */
> Â
> Âstatic const struct x86_cpu_id hwp_support_ids[] __initconst = {
> @@ -2088,6 +2094,8 @@ hwp_cpu_matched:
> Â if (!hwp_active && hwp_only)
> Â goto out;
> Â
> + intel_pstate_request_control();
> +
> Â rc = cpufreq_register_driver(intel_pstate_driver);
> Â if (rc)
> Â goto out;
>