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

Dirk Brandewie
2013-10-31T22:23:28Z

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.

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

--Dirk
>
>> ---
>> 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;
>>