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

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

[Adrian Huang]

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

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

[Rafael J. Wysocki]

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?

> ---
> 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;
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Dirk Brandewie]

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

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

[Rafael J. Wysocki]

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.

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

[Dirk Brandewie]

On 10/31/2013 03:47 PM, Rafael J. Wysocki wrote:
> 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?
>

I wasn't saying I was uncomfortable with the code just I am not expert
in ACPI.  From what I can tell it is doing the correct thing.

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

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

[Adrian Huang]

Re-send my reply email because my gmail account sent the email with html format. 
It was rejected by vger kernel. My bad.

BTW, the patch was verified correctly on HP ProLiant servers. Also, please see my other answers below. 

-- Adrian

於 四，2013-10-31 於 23:47 +0100，Rafael J. Wysocki 提到：
> 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?

I put hw_vendor_info vendor_info in case other vendors (Dell, Lenovo...)
have their firmware power management. Vendors should make sure their
firmware power management works properly, and they can go for adding
their vendor info to the variable.

> > 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?

Yes. HP BIOS has several power management modes (firmware, OS-control
and so on). For the OS control mode in HP BIOS, the Intel p-state driver
will be loaded. When the customer chooses the firmware power management
in HP BIOS, the Intel p-state driver will be ignored.

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

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

[Rafael J. Wysocki]

On Friday, November 01, 2013 09:29:48 AM Huang Adrian wrote:
> Yes. HP BIOS has several power management modes (firmware, OS-control and
> so on). For the OS control mode in HP BIOS, the Intel p-state driver will
> be loaded. When the customer chooses the firmware power management in HP
> BIOS, the Intel p-state driver will be ignored.
> 
> I put hw_vendor_info vendor_info in case other vendors (Dell, Lenovo...)
> have their firmware power management. Vendors should make sure their
> firmware power management works properly, and they can go for adding their
> vendor info to the variable.
> 
> And, I have verified the patch on HP ProLiant servers.  The patch worked
> correctly.

OK.  I've added the above information to the patch changelog and fixed it up
so that the driver builds for CONFIG_ACPI unset.  The result is below, please
retest.

Thanks,
Rafael


---
From: Adrian Huang <adrian.huang@hp.com>
Subject: intel_pstate: skip the driver if ACPI has power mgmt option

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.

HP BIOS has several power management modes (firmware, OS-control and
so on). For the OS control mode in HP BIOS, the Intel p-state driver
will be loaded. When the customer chooses the firmware power
management in HP BIOS, the Intel p-state driver will be ignored.

I put hw_vendor_info vendor_info in case other vendors (Dell, Lenovo...)
have their firmware power management. Vendors should make sure their
firmware power management works properly, and they can go for adding
their vendor info to the variable.

I have verified the patch on HP ProLiant servers.  The patch worked
correctly.

[rjw: Fixed up !CONFIG_ACPI build]
Signed-off-by: Adrian Huang <adrian.huang@hp.com>
---

 drivers/cpufreq/intel_pstate.c |   74 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -25,6 +25,7 @@
 #include <linux/types.h>
 #include <linux/fs.h>
 #include <linux/debugfs.h>
+#include <linux/acpi.h>
 #include <trace/events/power.h>
 
 #include <asm/div64.h>
@@ -777,6 +778,72 @@ static void copy_cpu_funcs(struct pstate
 	pstate_funcs.set       = funcs->set;
 }
 
+#if IS_ENABLED(CONFIG_ACPI)
+#include <acpi/processor.h>
+
+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;
+}
+
+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 bool intel_pstate_platform_pwr_mgmt_exists(void)
+{
+	struct acpi_table_header hdr;
+	struct hw_vendor_info *v_info;
+
+	if (acpi_disabled
+	    || 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;
+}
+#else /* CONFIG_ACPI not enabled */
+static inline bool intel_pstate_platform_pwr_mgmt_exists(void) { return false; }
+#endif /* CONFIG_ACPI */
+
 static int __init intel_pstate_init(void)
 {
 	int cpu, rc = 0;
@@ -790,6 +857,13 @@ static int __init intel_pstate_init(void
 	if (!id)
 		return -ENODEV;
 
+	/*
+	 * The Intel pstate driver will be ignored if the platform
+	 * firmware has its own power management modes.
+	 */
+	if (intel_pstate_platform_pwr_mgmt_exists())
+		return -ENODEV;
+
 	cpu_info = (struct cpu_defaults *)id->driver_data;
 
 	copy_pid_params(&cpu_info->pid_policy);

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

[Linda Knippers]

Rafael J. Wysocki wrote:
> On Friday, November 01, 2013 09:29:48 AM Huang Adrian wrote:
>> Yes. HP BIOS has several power management modes (firmware, OS-control and
>> so on). For the OS control mode in HP BIOS, the Intel p-state driver will
>> be loaded. When the customer chooses the firmware power management in HP
>> BIOS, the Intel p-state driver will be ignored.
>>
>> I put hw_vendor_info vendor_info in case other vendors (Dell, Lenovo...)
>> have their firmware power management. Vendors should make sure their
>> firmware power management works properly, and they can go for adding their
>> vendor info to the variable.
>>
>> And, I have verified the patch on HP ProLiant servers.  The patch worked
>> correctly.
> 
> OK.  I've added the above information to the patch changelog and fixed it up
> so that the driver builds for CONFIG_ACPI unset.  The result is below, please
> retest.

As Adrian has recently left HP, I retested the updated patch on an HP ProLiant
server and verified that it is behaving correctly.  When the BIOS is configured
for OS control for power management, the intel_pstate driver loads
as expected.  When the BIOS is configured to provide the power management, the
intel_pstate driver does not load and we get the pcc_cpufreq driver instead.

Since Adrian is no longer with HP, please add:

Signed-off-by: Linda Knippers <linda.knippers@hp.com>

Thanks,

-- ljk

> 
> Thanks,
> Rafael
> 
> 
> ---
> From: Adrian Huang <adrian.huang@hp.com>
> Subject: intel_pstate: skip the driver if ACPI has power mgmt option
> 
> 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.
> 
> HP BIOS has several power management modes (firmware, OS-control and
> so on). For the OS control mode in HP BIOS, the Intel p-state driver
> will be loaded. When the customer chooses the firmware power
> management in HP BIOS, the Intel p-state driver will be ignored.
> 
> I put hw_vendor_info vendor_info in case other vendors (Dell, Lenovo...)
> have their firmware power management. Vendors should make sure their
> firmware power management works properly, and they can go for adding
> their vendor info to the variable.
> 
> I have verified the patch on HP ProLiant servers.  The patch worked
> correctly.
> 
> [rjw: Fixed up !CONFIG_ACPI build]
> Signed-off-by: Adrian Huang <adrian.huang@hp.com>
> ---
> 
>  drivers/cpufreq/intel_pstate.c |   74 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -25,6 +25,7 @@
>  #include <linux/types.h>
>  #include <linux/fs.h>
>  #include <linux/debugfs.h>
> +#include <linux/acpi.h>
>  #include <trace/events/power.h>
>  
>  #include <asm/div64.h>
> @@ -777,6 +778,72 @@ static void copy_cpu_funcs(struct pstate
>  	pstate_funcs.set       = funcs->set;
>  }
>  
> +#if IS_ENABLED(CONFIG_ACPI)
> +#include <acpi/processor.h>
> +
> +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;
> +}
> +
> +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 bool intel_pstate_platform_pwr_mgmt_exists(void)
> +{
> +	struct acpi_table_header hdr;
> +	struct hw_vendor_info *v_info;
> +
> +	if (acpi_disabled
> +	    || 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;
> +}
> +#else /* CONFIG_ACPI not enabled */
> +static inline bool intel_pstate_platform_pwr_mgmt_exists(void) { return false; }
> +#endif /* CONFIG_ACPI */
> +
>  static int __init intel_pstate_init(void)
>  {
>  	int cpu, rc = 0;
> @@ -790,6 +857,13 @@ static int __init intel_pstate_init(void
>  	if (!id)
>  		return -ENODEV;
>  
> +	/*
> +	 * The Intel pstate driver will be ignored if the platform
> +	 * firmware has its own power management modes.
> +	 */
> +	if (intel_pstate_platform_pwr_mgmt_exists())
> +		return -ENODEV;
> +
>  	cpu_info = (struct cpu_defaults *)id->driver_data;
>  
>  	copy_pid_params(&cpu_info->pid_policy);
>