Re: [PATCH v3] cpufreq: intel_pstate: skip the driver if ACPI has power mgmt option

Rafael J. Wysocki
2013-10-31T22:34:56Z

On Thursday, October 31, 2013 03:23:24 PM Dirk Brandewie wrote:
> On 10/31/2013 03:29 PM, Rafael J. Wysocki wrote:
> > On Thursday, October 31, 2013 11:24:05 PM Adrian Huang wrote:
> >> Do not load the Intel pstate driver if the platform firmware
> >> (ACPI BIOS) supports the power management alternatives.
> >> The ACPI BIOS indicates that the OS control mode can be used
> >> if the _PSS (Performance Supported States) is defined in ACPI
> >> table. For the OS control mode, the Intel pstate driver will be
> >> loaded.
> >>
> >> Signed-off-by: Adrian Huang <adrian.huang@hp.com>
> >
> > Do I assume correctly that this has been tested on the affected hardware?
> >
> > Dirk, any objections?
>
> As long as you are comfortable with the ACPI related logic for matching
> the entry in the hw_vendor_info vendor_info[] I have no objections.

Well, is there any other way to address that?

> One question though can this be disabled via the HP BIOS if the
> customer does not want to use the firmware power management?

Good question. Adrian?

> >> ---
> >> Changes from v2:
> >> - Remove unnecessary assignments and parentheses (Thanks to Rafael)
> >>
> >> Changes from v1:
> >> - Minimize indentation levels (Thanks to Rafael)
> >> - Re-define some local variables (Thanks to by Rafael)
> >> - Return -ENODEV if platform FW has power management modes (Thanks to by Dirk)
> >>
> >> drivers/cpufreq/intel_pstate.c | 70 ++++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 70 insertions(+)
> >>
> >> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> >> index eb3fdc7..9ec1d6c 100644
> >> --- a/drivers/cpufreq/intel_pstate.c
> >> +++ b/drivers/cpufreq/intel_pstate.c
> >> @@ -26,6 +26,8 @@
> >> #include <linux/fs.h>
> >> #include <linux/debugfs.h>
> >> #include <trace/events/power.h>
> >> +#include <linux/acpi.h>
> >> +#include <acpi/processor.h>
> >>
> >> #include <asm/div64.h>
> >> #include <asm/msr.h>
> >> @@ -129,6 +131,18 @@ static struct perf_limits limits = {
> >> .max_sysfs_pct = 100,
> >> };
> >>
> >> +struct hw_vendor_info {
> >> + u16 valid;
> >> + char oem_id[ACPI_OEM_ID_SIZE];
> >> + char oem_table_id[ACPI_OEM_TABLE_ID_SIZE];
> >> +};
> >> +
> >> +/* Hardware vendor-specific info that has its own power management modes */
> >> +static struct hw_vendor_info vendor_info[] = {
> >> + {1, "HP ", "ProLiant"},
> >> + {0, "", ""},
> >> +};
> >> +
> >> static inline void pid_reset(struct _pid *pid, int setpoint, int busy,
> >> int deadband, int integral) {
> >> pid->setpoint = setpoint;
> >> @@ -698,6 +712,55 @@ static int intel_pstate_msrs_not_valid(void)
> >>
> >> return 0;
> >> }
> >> +
> >> +static bool intel_pstate_no_acpi_pss(void)
> >> +{
> >> + int i;
> >> +
> >> + for_each_possible_cpu(i) {
> >> + acpi_status status;
> >> + union acpi_object *pss;
> >> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> >> + struct acpi_processor *pr = per_cpu(processors, i);
> >> +
> >> + if (!pr)
> >> + continue;
> >> +
> >> + status = acpi_evaluate_object(pr->handle, "_PSS",
> >> + NULL, &buffer);
> >> + if (ACPI_FAILURE(status))
> >> + continue;
> >> +
> >> + pss = buffer.pointer;
> >> + if (pss && pss->type == ACPI_TYPE_PACKAGE) {
> >> + kfree(pss);
> >> + return false;
> >> + }
> >> +
> >> + kfree(pss);
> >> + }
> >> +
> >> + return true;
> >> +}
> >> +
> >> +static bool intel_pstate_platform_pwr_mgmt_exists(void)
> >> +{
> >> + struct acpi_table_header hdr;
> >> + struct hw_vendor_info *v_info;
> >> +
> >> + if (ACPI_FAILURE(acpi_get_table_header(ACPI_SIG_FADT, 0, &hdr)))
> >> + return false;
> >> +
> >> + for (v_info = vendor_info; v_info->valid; v_info++) {
> >> + if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE)
> >> + && !strncmp(hdr.oem_table_id, v_info->oem_table_id, ACPI_OEM_TABLE_ID_SIZE)
> >> + && intel_pstate_no_acpi_pss())
> >> + return true;
> >> + }
> >> +
> >> + return false;
> >> +}
> >> +
> >> static int __init intel_pstate_init(void)
> >> {
> >> int cpu, rc = 0;
> >> @@ -706,6 +769,13 @@ static int __init intel_pstate_init(void)
> >> if (no_load)
> >> return -ENODEV;
> >>
> >> + /*
> >> + * The Intel pstate driver will be ignored if the platform
> >> + * firmware has its own power management modes.
> >> + */
> >> + if (!acpi_disabled && intel_pstate_platform_pwr_mgmt_exists())
> >> + return -ENODEV;
> >> +
> >> id = x86_match_cpu(intel_pstate_cpu_ids);
> >> if (!id)
> >> return -ENODEV;
> >>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.