LinuxLists.cc - [PATCH 0/3] intel_pstate: allow to be built as module and handle Sun server power capping.

[permalink] [raw]

Subject: [PATCH 0/3] intel_pstate: allow to be built as module and handle Sun server power capping.

2014-11-18 08:37:53

[permalink] [raw]

Subject: [PATCH 3/3] intel_pstate: add module and kernel command line parameter to ignore ACPI _PPC

Add kernel command line parameter
intel_pstate = ignore_acpi_ppc
and module parameter
ignore_acpi_ppc = 1
to allow driver to ignore the ACPI _PPC existence even for Sun x86 servers.
These parameter could be used for debug\test\workaround etc purpose.

Signed-off-by: Ethan Zhao <[email protected]>
---
Documentation/kernel-parameters.txt | 3 +++
drivers/cpufreq/intel_pstate.c | 8 +++++++-
2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 4c81a86..f502b85 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1446,6 +1446,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
disable
Do not enable intel_pstate as the default
scaling driver for the supported processors
+ ignore_acpi_ppc
+ Ignore the existence of ACPI method _PPC for Sun x86 servers
+ and load the driver.

intremap= [X86-64, Intel-IOMMU]
on enable Interrupt Remapping (default)
diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 7c5faea..388387b 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -870,6 +870,7 @@ static struct cpufreq_driver intel_pstate_driver = {
};

static int __initdata no_load;
+static unsigned int ignore_acpi_ppc;

static int intel_pstate_msrs_not_valid(void)
{
@@ -990,7 +991,7 @@ static bool intel_pstate_platform_pwr_mgmt_exists(void)
intel_pstate_no_acpi_pss())
return true;
if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
- intel_pstate_has_acpi_ppc())
+ intel_pstate_has_acpi_ppc() && !ignore_acpi_ppc)
return true;
}

@@ -1066,11 +1067,16 @@ static int __init intel_pstate_setup(char *str)

if (!strcmp(str, "disable"))
no_load = 1;
+ if (!strcmp(str, "ignore_acpi_ppc"))
+ ignore_acpi_ppc = 1;
return 0;
}
early_param("intel_pstate", intel_pstate_setup);
#endif

+module_param(ignore_acpi_ppc, uint, 0644);
+MODULE_PARM_DESC(ignore_acpi_ppc,
+ "value 0 or non-zero. non-zero -> ignore ACPI _PPC and load this driver");
MODULE_AUTHOR("Dirk Brandewie <[email protected]>");
MODULE_DESCRIPTION("'intel_pstate' - P state driver Intel Core processors");
MODULE_LICENSE("GPL");
--
1.8.3.1

2014-11-18 08:38:19

[permalink] [raw]

Subject: [PATCH 2/3] intel_pstate: allow driver to be built as a module

From: Brian Maly <[email protected]>

To provide the flexibility of module, allow this driver to
be configured and built as a module.

Signed-off-by: Brian Maly <[email protected]>
Signed-off-by: Ethan Zhao <[email protected]>
---
drivers/cpufreq/Kconfig.x86 | 2 +-
drivers/cpufreq/intel_pstate.c | 6 ++++++
2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86
index 89ae88f..94c9e6b 100644
--- a/drivers/cpufreq/Kconfig.x86
+++ b/drivers/cpufreq/Kconfig.x86
@@ -3,7 +3,7 @@
#

config X86_INTEL_PSTATE
- bool "Intel P state control"
+ tristate "Intel P state control"
depends on X86
help
This driver provides a P state for Intel core processors.
diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 5498eb0..7c5faea 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -590,7 +590,9 @@ static void intel_pstate_set_pstate(struct cpudata *cpu, int pstate)
if (pstate == cpu->pstate.current_pstate)
return;

+#ifndef MODULE
trace_cpu_frequency(pstate * cpu->pstate.scaling, cpu->cpu);
+#endif

cpu->pstate.current_pstate = pstate;

@@ -705,12 +707,14 @@ static void intel_pstate_timer_func(unsigned long __data)

intel_pstate_adjust_busy_pstate(cpu);

+#ifndef MODULE
trace_pstate_sample(fp_toint(sample->core_pct_busy),
fp_toint(intel_pstate_get_scaled_busy(cpu)),
cpu->pstate.current_pstate,
sample->mperf,
sample->aperf,
sample->freq);
+#endif

intel_pstate_set_sample_time(cpu);
}
@@ -1054,6 +1058,7 @@ out:
}
device_initcall(intel_pstate_init);

+#ifndef MODULE
static int __init intel_pstate_setup(char *str)
{
if (!str)
@@ -1064,6 +1069,7 @@ static int __init intel_pstate_setup(char *str)
return 0;
}
early_param("intel_pstate", intel_pstate_setup);
+#endif

MODULE_AUTHOR("Dirk Brandewie <[email protected]>");
MODULE_DESCRIPTION("'intel_pstate' - P state driver Intel Core processors");
--
1.8.3.1

2014-11-18 08:37:47

[permalink] [raw]

Subject: [PATCH 3/3] intel_pstate: add module and kernel command line parameter to ignore ACPI _PPC

2014-11-18 08:39:42

[permalink] [raw]

Subject: [PATCH 1/3] intel_pstate: skip the driver if Sun server has ACPI _PPC method

Oracle Sun X86 servers have dynamic power capping capability that works via
ACPI _PPC method etc, so skip loading this driver if Sun server has ACPI _PPC
enabled.

Signed-off-by: Ethan Zhao <[email protected]>
---
drivers/cpufreq/intel_pstate.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 27bb6d3..5498eb0 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -943,6 +943,21 @@ static bool intel_pstate_no_acpi_pss(void)
return true;
}

+static bool intel_pstate_has_acpi_ppc(void)
+{
+ int i;
+
+ for_each_possible_cpu(i) {
+ struct acpi_processor *pr = per_cpu(processors, i);
+
+ if (!pr)
+ continue;
+ if (acpi_has_method(pr->handle, "_PPC"))
+ return true;
+ }
+ return false;
+}
+
struct hw_vendor_info {
u16 valid;
char oem_id[ACPI_OEM_ID_SIZE];
@@ -952,6 +967,7 @@ struct hw_vendor_info {
/* Hardware vendor-specific info that has its own power management modes */
static struct hw_vendor_info vendor_info[] = {
{1, "HP ", "ProLiant"},
+ {1, "ORACLE", ""},
{0, "", ""},
};

@@ -969,12 +985,16 @@ static bool intel_pstate_platform_pwr_mgmt_exists(void)
!strncmp(hdr.oem_table_id, v_info->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
intel_pstate_no_acpi_pss())
return true;
+ if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
+ intel_pstate_has_acpi_ppc())
+ return true;
}

return false;
}
#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; }
#endif /* CONFIG_ACPI */

static int __init intel_pstate_init(void)
--
1.8.3.1

2014-11-18 08:42:31

[permalink] [raw]

Subject: [PATCH 2/3] intel_pstate: allow driver to be built as a module

2014-11-18 08:42:52

[permalink] [raw]

Subject: [PATCH 1/3] intel_pstate: skip the driver if Sun server has ACPI _PPC method

2014-11-18 20:15:25

by Rafael J. Wysocki

[permalink] [raw]

Subject: Re: [PATCH 2/3] intel_pstate: allow driver to be built as a module

On Tuesday, November 18, 2014 05:37:01 PM Ethan Zhao wrote:
> From: Brian Maly <[email protected]>
>
> To provide the flexibility of module, allow this driver to
> be configured and built as a module.
>
> Signed-off-by: Brian Maly <[email protected]>
> Signed-off-by: Ethan Zhao <[email protected]>
> ---
> drivers/cpufreq/Kconfig.x86 | 2 +-
> drivers/cpufreq/intel_pstate.c | 6 ++++++
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86
> index 89ae88f..94c9e6b 100644
> --- a/drivers/cpufreq/Kconfig.x86
> +++ b/drivers/cpufreq/Kconfig.x86
> @@ -3,7 +3,7 @@
> #
>
> config X86_INTEL_PSTATE
> - bool "Intel P state control"
> + tristate "Intel P state control"
> depends on X86
> help
> This driver provides a P state for Intel core processors.
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 5498eb0..7c5faea 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -590,7 +590,9 @@ static void intel_pstate_set_pstate(struct cpudata *cpu, int pstate)
> if (pstate == cpu->pstate.current_pstate)
> return;
>
> +#ifndef MODULE

This is not acceptable in any code that I maintain, sorry.

> trace_cpu_frequency(pstate * cpu->pstate.scaling, cpu->cpu);
> +#endif
>
> cpu->pstate.current_pstate = pstate;
>
> @@ -705,12 +707,14 @@ static void intel_pstate_timer_func(unsigned long __data)
>
> intel_pstate_adjust_busy_pstate(cpu);
>
> +#ifndef MODULE
> trace_pstate_sample(fp_toint(sample->core_pct_busy),
> fp_toint(intel_pstate_get_scaled_busy(cpu)),
> cpu->pstate.current_pstate,
> sample->mperf,
> sample->aperf,
> sample->freq);
> +#endif
>
> intel_pstate_set_sample_time(cpu);
> }
> @@ -1054,6 +1058,7 @@ out:
> }
> device_initcall(intel_pstate_init);
>
> +#ifndef MODULE
> static int __init intel_pstate_setup(char *str)
> {
> if (!str)
> @@ -1064,6 +1069,7 @@ static int __init intel_pstate_setup(char *str)
> return 0;
> }
> early_param("intel_pstate", intel_pstate_setup);
> +#endif
>
> MODULE_AUTHOR("Dirk Brandewie <[email protected]>");
> MODULE_DESCRIPTION("'intel_pstate' - P state driver Intel Core processors");
>

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

2014-11-19 15:00:15

by Dirk Brandewie

[permalink] [raw]

Subject: Re: [PATCH 1/3] intel_pstate: skip the driver if Sun server has ACPI _PPC method

On 11/18/2014 12:37 AM, Ethan Zhao wrote:
> Oracle Sun X86 servers have dynamic power capping capability that works via
> ACPI _PPC method etc, so skip loading this driver if Sun server has ACPI _PPC
> enabled.
>
> Signed-off-by: Ethan Zhao <[email protected]>
> ---
> drivers/cpufreq/intel_pstate.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 27bb6d3..5498eb0 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -943,6 +943,21 @@ static bool intel_pstate_no_acpi_pss(void)
> return true;
> }
>
> +static bool intel_pstate_has_acpi_ppc(void)
> +{
> + int i;
> +
> + for_each_possible_cpu(i) {
> + struct acpi_processor *pr = per_cpu(processors, i);
> +
> + if (!pr)
> + continue;
> + if (acpi_has_method(pr->handle, "_PPC"))
> + return true;
> + }
> + return false;
> +}
> +
> struct hw_vendor_info {
> u16 valid;
> char oem_id[ACPI_OEM_ID_SIZE];
> @@ -952,6 +967,7 @@ struct hw_vendor_info {
> /* Hardware vendor-specific info that has its own power management modes */
> static struct hw_vendor_info vendor_info[] = {
> {1, "HP ", "ProLiant"},
> + {1, "ORACLE", ""},
> {0, "", ""},
> };
>

Does this apply to ALL oracle systems?

Is the presence or absense of the _PPC method configurable in the oracle BIOS?

> @@ -969,12 +985,16 @@ static bool intel_pstate_platform_pwr_mgmt_exists(void)
> !strncmp(hdr.oem_table_id, v_info->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
> intel_pstate_no_acpi_pss())
> return true;
> + if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
> + intel_pstate_has_acpi_ppc())
> + return true;
> }
>
> return false;
> }
> #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; }
> #endif /* CONFIG_ACPI */
>
> static int __init intel_pstate_init(void)
>

2014-11-19 18:58:36

by Kristen Carlson Accardi

[permalink] [raw]

Subject: Re: [PATCH 2/3] intel_pstate: allow driver to be built as a module

On Tue, 18 Nov 2014 17:37:05 +0900
Ethan Zhao <[email protected]> wrote:

> From: Brian Maly <[email protected]>
>
> To provide the flexibility of module, allow this driver to
> be configured and built as a module.
>
> Signed-off-by: Brian Maly <[email protected]>
> Signed-off-by: Ethan Zhao <[email protected]>

I believe the entire concept of being able to use intel_pstate as a
module just isn't going to work. There are load order issues - and
additionally the driver doesn't clean up after itself in any way.

> ---
> drivers/cpufreq/Kconfig.x86 | 2 +-
> drivers/cpufreq/intel_pstate.c | 6 ++++++
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86
> index 89ae88f..94c9e6b 100644
> --- a/drivers/cpufreq/Kconfig.x86
> +++ b/drivers/cpufreq/Kconfig.x86
> @@ -3,7 +3,7 @@
> #
>
> config X86_INTEL_PSTATE
> - bool "Intel P state control"
> + tristate "Intel P state control"
> depends on X86
> help
> This driver provides a P state for Intel core processors.
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 5498eb0..7c5faea 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -590,7 +590,9 @@ static void intel_pstate_set_pstate(struct cpudata *cpu, int pstate)
> if (pstate == cpu->pstate.current_pstate)
> return;
>
> +#ifndef MODULE
> trace_cpu_frequency(pstate * cpu->pstate.scaling, cpu->cpu);
> +#endif
>
> cpu->pstate.current_pstate = pstate;
>
> @@ -705,12 +707,14 @@ static void intel_pstate_timer_func(unsigned long __data)
>
> intel_pstate_adjust_busy_pstate(cpu);
>
> +#ifndef MODULE
> trace_pstate_sample(fp_toint(sample->core_pct_busy),
> fp_toint(intel_pstate_get_scaled_busy(cpu)),
> cpu->pstate.current_pstate,
> sample->mperf,
> sample->aperf,
> sample->freq);
> +#endif
>
> intel_pstate_set_sample_time(cpu);
> }
> @@ -1054,6 +1058,7 @@ out:
> }
> device_initcall(intel_pstate_init);
>
> +#ifndef MODULE
> static int __init intel_pstate_setup(char *str)
> {
> if (!str)
> @@ -1064,6 +1069,7 @@ static int __init intel_pstate_setup(char *str)
> return 0;
> }
> early_param("intel_pstate", intel_pstate_setup);
> +#endif
>
> MODULE_AUTHOR("Dirk Brandewie <[email protected]>");
> MODULE_DESCRIPTION("'intel_pstate' - P state driver Intel Core processors");

2014-11-19 19:05:54

by Kristen Carlson Accardi

[permalink] [raw]

Subject: Re: [PATCH 3/3] intel_pstate: add module and kernel command line parameter to ignore ACPI _PPC

On Tue, 18 Nov 2014 17:37:06 +0900
Ethan Zhao <[email protected]> wrote:

> Add kernel command line parameter
> intel_pstate = ignore_acpi_ppc
> and module parameter
> ignore_acpi_ppc = 1
> to allow driver to ignore the ACPI _PPC existence even for Sun x86 servers.
> These parameter could be used for debug\test\workaround etc purpose.
>
> Signed-off-by: Ethan Zhao <[email protected]>

What if we used a more generic parameter like "force" that would bypass
any vendor specific checks and just load anyway? This way we don't have
to add new parameters everything some new thing shows up that we want to
ignore.

> ---
> Documentation/kernel-parameters.txt | 3 +++
> drivers/cpufreq/intel_pstate.c | 8 +++++++-
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 4c81a86..f502b85 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1446,6 +1446,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> disable
> Do not enable intel_pstate as the default
> scaling driver for the supported processors
> + ignore_acpi_ppc
> + Ignore the existence of ACPI method _PPC for Sun x86 servers
> + and load the driver.
>
> intremap= [X86-64, Intel-IOMMU]
> on enable Interrupt Remapping (default)
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 7c5faea..388387b 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -870,6 +870,7 @@ static struct cpufreq_driver intel_pstate_driver = {
> };
>
> static int __initdata no_load;
> +static unsigned int ignore_acpi_ppc;
>
> static int intel_pstate_msrs_not_valid(void)
> {
> @@ -990,7 +991,7 @@ static bool intel_pstate_platform_pwr_mgmt_exists(void)
> intel_pstate_no_acpi_pss())
> return true;
> if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
> - intel_pstate_has_acpi_ppc())
> + intel_pstate_has_acpi_ppc() && !ignore_acpi_ppc)
> return true;
> }
>
> @@ -1066,11 +1067,16 @@ static int __init intel_pstate_setup(char *str)
>
> if (!strcmp(str, "disable"))
> no_load = 1;
> + if (!strcmp(str, "ignore_acpi_ppc"))
> + ignore_acpi_ppc = 1;
> return 0;
> }
> early_param("intel_pstate", intel_pstate_setup);
> #endif
>
> +module_param(ignore_acpi_ppc, uint, 0644);
> +MODULE_PARM_DESC(ignore_acpi_ppc,
> + "value 0 or non-zero. non-zero -> ignore ACPI _PPC and load this driver");
> MODULE_AUTHOR("Dirk Brandewie <[email protected]>");
> MODULE_DESCRIPTION("'intel_pstate' - P state driver Intel Core processors");
> MODULE_LICENSE("GPL");

2014-11-19 20:22:16

[permalink] [raw]

Subject: Re: [PATCH 1/3] intel_pstate: skip the driver if Sun server has ACPI _PPC method

On 11/18/2014 3:37 AM, Ethan Zhao wrote:
> Oracle Sun X86 servers have dynamic power capping capability that works via
> ACPI _PPC method etc, so skip loading this driver if Sun server has ACPI _PPC
> enabled.
>
> Signed-off-by: Ethan Zhao <[email protected]>
> ---
> drivers/cpufreq/intel_pstate.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 27bb6d3..5498eb0 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -943,6 +943,21 @@ static bool intel_pstate_no_acpi_pss(void)
> return true;
> }
>
> +static bool intel_pstate_has_acpi_ppc(void)
> +{
> + int i;
> +
> + for_each_possible_cpu(i) {
> + struct acpi_processor *pr = per_cpu(processors, i);
> +
> + if (!pr)
> + continue;
> + if (acpi_has_method(pr->handle, "_PPC"))
> + return true;
> + }
> + return false;
> +}
> +
> struct hw_vendor_info {
> u16 valid;
> char oem_id[ACPI_OEM_ID_SIZE];
> @@ -952,6 +967,7 @@ struct hw_vendor_info {
> /* Hardware vendor-specific info that has its own power management modes */
> static struct hw_vendor_info vendor_info[] = {
> {1, "HP ", "ProLiant"},
> + {1, "ORACLE", ""},
> {0, "", ""},
> };
>
> @@ -969,12 +985,16 @@ static bool intel_pstate_platform_pwr_mgmt_exists(void)
> !strncmp(hdr.oem_table_id, v_info->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
> intel_pstate_no_acpi_pss())
> return true;
> + if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
> + intel_pstate_has_acpi_ppc())

We need try this on a few platforms to make sure this patch doesn't break the
HP platforms that may or may not need this driver, depending on the BIOS settings.

I don't suppose you tested on a ProLiant too?

-- ljk

> + return true;
> }
>
> return false;
> }
> #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; }
> #endif /* CONFIG_ACPI */
>
> static int __init intel_pstate_init(void)
>

2014-11-20 00:52:07

[permalink] [raw]

Subject: Re: [PATCH 1/3] intel_pstate: skip the driver if Sun server has ACPI _PPC method

Linda??

> ?? 2014??11??20?գ?04:22??Linda Knippers <[email protected]> д????
>
>> On 11/18/2014 3:37 AM, Ethan Zhao wrote:
>> Oracle Sun X86 servers have dynamic power capping capability that works via
>> ACPI _PPC method etc, so skip loading this driver if Sun server has ACPI _PPC
>> enabled.
>>
>> Signed-off-by: Ethan Zhao <[email protected]>
>> ---
>> drivers/cpufreq/intel_pstate.c | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>> index 27bb6d3..5498eb0 100644
>> --- a/drivers/cpufreq/intel_pstate.c
>> +++ b/drivers/cpufreq/intel_pstate.c
>> @@ -943,6 +943,21 @@ static bool intel_pstate_no_acpi_pss(void)
>> return true;
>> }
>>
>> +static bool intel_pstate_has_acpi_ppc(void)
>> +{
>> + int i;
>> +
>> + for_each_possible_cpu(i) {
>> + struct acpi_processor *pr = per_cpu(processors, i);
>> +
>> + if (!pr)
>> + continue;
>> + if (acpi_has_method(pr->handle, "_PPC"))
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> struct hw_vendor_info {
>> u16 valid;
>> char oem_id[ACPI_OEM_ID_SIZE];
>> @@ -952,6 +967,7 @@ struct hw_vendor_info {
>> /* Hardware vendor-specific info that has its own power management modes */
>> static struct hw_vendor_info vendor_info[] = {
>> {1, "HP ", "ProLiant"},
>> + {1, "ORACLE", ""},
>> {0, "", ""},
>> };
>>
>> @@ -969,12 +985,16 @@ static bool intel_pstate_platform_pwr_mgmt_exists(void)
>> !strncmp(hdr.oem_table_id, v_info->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
>> intel_pstate_no_acpi_pss())
>> return true;
>> + if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
>> + intel_pstate_has_acpi_ppc())
>
> We need try this on a few platforms to make sure this patch doesn't break the
> HP platforms that may or may not need this driver, depending on the BIOS settings.
>
> I don't suppose you tested on a ProLiant too?
I am very glad you could catch this though I missed to CC you.
The code only touch oem_id 'ORACLE' platform, wouldn't break HP's.
But it is good you could test on ProLiant.(not find one around yet)

Thanks,
Ethan

>
> -- ljk
>
>> + return true;
>> }
>>
>> return false;
>> }
>> #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; }
>> #endif /* CONFIG_ACPI */
>>
>> static int __init intel_pstate_init(void)
>>
>

2014-11-20 00:57:39

[permalink] [raw]

Subject: Re: [PATCH 3/3] intel_pstate: add module and kernel command line parameter to ignore ACPI _PPC

> ?? 2014??11??20?գ?03:05??Kristen Carlson Accardi <[email protected]> д????
>
> On Tue, 18 Nov 2014 17:37:06 +0900
> Ethan Zhao <[email protected]> wrote:
>
>> Add kernel command line parameter
>> intel_pstate = ignore_acpi_ppc
>> and module parameter
>> ignore_acpi_ppc = 1
>> to allow driver to ignore the ACPI _PPC existence even for Sun x86 servers.
>> These parameter could be used for debug\test\workaround etc purpose.
>>
>> Signed-off-by: Ethan Zhao <[email protected]>
>
> What if we used a more generic parameter like "force" that would bypass
> any vendor specific checks and just load anyway? This way we don't have
> to add new parameters everything some new thing shows up that we want to
> ignore.
>
To be honest, I prefer more generic parameter. But to avoid the possible negative affect
To another vendors. I back to this way.

Thanks,
Ethan
>> ---
>> Documentation/kernel-parameters.txt | 3 +++
>> drivers/cpufreq/intel_pstate.c | 8 +++++++-
>> 2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>> index 4c81a86..f502b85 100644
>> --- a/Documentation/kernel-parameters.txt
>> +++ b/Documentation/kernel-parameters.txt
>> @@ -1446,6 +1446,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>> disable
>> Do not enable intel_pstate as the default
>> scaling driver for the supported processors
>> + ignore_acpi_ppc
>> + Ignore the existence of ACPI method _PPC for Sun x86 servers
>> + and load the driver.
>>
>> intremap= [X86-64, Intel-IOMMU]
>> on enable Interrupt Remapping (default)
>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>> index 7c5faea..388387b 100644
>> --- a/drivers/cpufreq/intel_pstate.c
>> +++ b/drivers/cpufreq/intel_pstate.c
>> @@ -870,6 +870,7 @@ static struct cpufreq_driver intel_pstate_driver = {
>> };
>>
>> static int __initdata no_load;
>> +static unsigned int ignore_acpi_ppc;
>>
>> static int intel_pstate_msrs_not_valid(void)
>> {
>> @@ -990,7 +991,7 @@ static bool intel_pstate_platform_pwr_mgmt_exists(void)
>> intel_pstate_no_acpi_pss())
>> return true;
>> if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
>> - intel_pstate_has_acpi_ppc())
>> + intel_pstate_has_acpi_ppc() && !ignore_acpi_ppc)
>> return true;
>> }
>>
>> @@ -1066,11 +1067,16 @@ static int __init intel_pstate_setup(char *str)
>>
>> if (!strcmp(str, "disable"))
>> no_load = 1;
>> + if (!strcmp(str, "ignore_acpi_ppc"))
>> + ignore_acpi_ppc = 1;
>> return 0;
>> }
>> early_param("intel_pstate", intel_pstate_setup);
>> #endif
>>
>> +module_param(ignore_acpi_ppc, uint, 0644);
>> +MODULE_PARM_DESC(ignore_acpi_ppc,
>> + "value 0 or non-zero. non-zero -> ignore ACPI _PPC and load this driver");
>> MODULE_AUTHOR("Dirk Brandewie <[email protected]>");
>> MODULE_DESCRIPTION("'intel_pstate' - P state driver Intel Core processors");
>> MODULE_LICENSE("GPL");
>

2014-11-20 01:01:31

[permalink] [raw]

Subject: Re: [PATCH 2/3] intel_pstate: allow driver to be built as a module

Kristen,
> ?? 2014??11??20?գ?02:58??Kristen Carlson Accardi <[email protected]> д????
>
> On Tue, 18 Nov 2014 17:37:05 +0900
> Ethan Zhao <[email protected]> wrote:
>
>> From: Brian Maly <[email protected]>
>>
>> To provide the flexibility of module, allow this driver to
>> be configured and built as a module.
>>
>> Signed-off-by: Brian Maly <[email protected]>
>> Signed-off-by: Ethan Zhao <[email protected]>
>
> I believe the entire concept of being able to use intel_pstate as a
> module just isn't going to work. There are load order issues - and
> additionally the driver doesn't clean up after itself in any way.
>
Roger.

Thanks,
Ethan
>> ---
>> drivers/cpufreq/Kconfig.x86 | 2 +-
>> drivers/cpufreq/intel_pstate.c | 6 ++++++
>> 2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86
>> index 89ae88f..94c9e6b 100644
>> --- a/drivers/cpufreq/Kconfig.x86
>> +++ b/drivers/cpufreq/Kconfig.x86
>> @@ -3,7 +3,7 @@
>> #
>>
>> config X86_INTEL_PSTATE
>> - bool "Intel P state control"
>> + tristate "Intel P state control"
>> depends on X86
>> help
>> This driver provides a P state for Intel core processors.
>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>> index 5498eb0..7c5faea 100644
>> --- a/drivers/cpufreq/intel_pstate.c
>> +++ b/drivers/cpufreq/intel_pstate.c
>> @@ -590,7 +590,9 @@ static void intel_pstate_set_pstate(struct cpudata *cpu, int pstate)
>> if (pstate == cpu->pstate.current_pstate)
>> return;
>>
>> +#ifndef MODULE
>> trace_cpu_frequency(pstate * cpu->pstate.scaling, cpu->cpu);
>> +#endif
>>
>> cpu->pstate.current_pstate = pstate;
>>
>> @@ -705,12 +707,14 @@ static void intel_pstate_timer_func(unsigned long __data)
>>
>> intel_pstate_adjust_busy_pstate(cpu);
>>
>> +#ifndef MODULE
>> trace_pstate_sample(fp_toint(sample->core_pct_busy),
>> fp_toint(intel_pstate_get_scaled_busy(cpu)),
>> cpu->pstate.current_pstate,
>> sample->mperf,
>> sample->aperf,
>> sample->freq);
>> +#endif
>>
>> intel_pstate_set_sample_time(cpu);
>> }
>> @@ -1054,6 +1058,7 @@ out:
>> }
>> device_initcall(intel_pstate_init);
>>
>> +#ifndef MODULE
>> static int __init intel_pstate_setup(char *str)
>> {
>> if (!str)
>> @@ -1064,6 +1069,7 @@ static int __init intel_pstate_setup(char *str)
>> return 0;
>> }
>> early_param("intel_pstate", intel_pstate_setup);
>> +#endif
>>
>> MODULE_AUTHOR("Dirk Brandewie <[email protected]>");
>> MODULE_DESCRIPTION("'intel_pstate' - P state driver Intel Core processors");
>

2014-11-20 01:07:54

[permalink] [raw]

Subject: Re: [PATCH 1/3] intel_pstate: skip the driver if Sun server has ACPI _PPC method

Dirk,

> ?? 2014??11??19?գ?22:59??Dirk Brandewie <[email protected]> д????
>
>> On 11/18/2014 12:37 AM, Ethan Zhao wrote:
>> Oracle Sun X86 servers have dynamic power capping capability that works via
>> ACPI _PPC method etc, so skip loading this driver if Sun server has ACPI _PPC
>> enabled.
>>
>> Signed-off-by: Ethan Zhao <[email protected]>
>> ---
>> drivers/cpufreq/intel_pstate.c | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>> index 27bb6d3..5498eb0 100644
>> --- a/drivers/cpufreq/intel_pstate.c
>> +++ b/drivers/cpufreq/intel_pstate.c
>> @@ -943,6 +943,21 @@ static bool intel_pstate_no_acpi_pss(void)
>> return true;
>> }
>>
>> +static bool intel_pstate_has_acpi_ppc(void)
>> +{
>> + int i;
>> +
>> + for_each_possible_cpu(i) {
>> + struct acpi_processor *pr = per_cpu(processors, i);
>> +
>> + if (!pr)
>> + continue;
>> + if (acpi_has_method(pr->handle, "_PPC"))
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> struct hw_vendor_info {
>> u16 valid;
>> char oem_id[ACPI_OEM_ID_SIZE];
>> @@ -952,6 +967,7 @@ struct hw_vendor_info {
>> /* Hardware vendor-specific info that has its own power management modes */
>> static struct hw_vendor_info vendor_info[] = {
>> {1, "HP ", "ProLiant"},
>> + {1, "ORACLE", ""},
>> {0, "", ""},
>> };
>
> Does this apply to ALL oracle systems?
Yes, as I know, all recent oracle x86 servers have power capping functions.
>
> Is the presence or absense of the _PPC method configurable in the oracle BIOS?
There is no option in BIOS to enable or disable _PPC. But they configure power limit
Functions via SP(service processors).

Thanks,
Ethan
>
>> @@ -969,12 +985,16 @@ static bool intel_pstate_platform_pwr_mgmt_exists(void)
>> !strncmp(hdr.oem_table_id, v_info->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
>> intel_pstate_no_acpi_pss())
>> return true;
>> + if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
>> + intel_pstate_has_acpi_ppc())
>> + return true;
>> }
>>
>> return false;
>> }
>> #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; }
>> #endif /* CONFIG_ACPI */
>>
>> static int __init intel_pstate_init(void)
>

2014-11-20 16:50:13

by Dirk Brandewie

[permalink] [raw]

Subject: Re: [PATCH 1/3] intel_pstate: skip the driver if Sun server has ACPI _PPC method

On 11/19/2014 12:22 PM, Linda Knippers wrote:
> On 11/18/2014 3:37 AM, Ethan Zhao wrote:
>> Oracle Sun X86 servers have dynamic power capping capability that works via
>> ACPI _PPC method etc, so skip loading this driver if Sun server has ACPI _PPC
>> enabled.
>>
>> Signed-off-by: Ethan Zhao <[email protected]>
>> ---
>> drivers/cpufreq/intel_pstate.c | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>> index 27bb6d3..5498eb0 100644
>> --- a/drivers/cpufreq/intel_pstate.c
>> +++ b/drivers/cpufreq/intel_pstate.c
>> @@ -943,6 +943,21 @@ static bool intel_pstate_no_acpi_pss(void)
>> return true;
>> }
>>
>> +static bool intel_pstate_has_acpi_ppc(void)
>> +{
>> + int i;
>> +
>> + for_each_possible_cpu(i) {
>> + struct acpi_processor *pr = per_cpu(processors, i);
>> +
>> + if (!pr)
>> + continue;
>> + if (acpi_has_method(pr->handle, "_PPC"))
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> struct hw_vendor_info {
>> u16 valid;
>> char oem_id[ACPI_OEM_ID_SIZE];
>> @@ -952,6 +967,7 @@ struct hw_vendor_info {
>> /* Hardware vendor-specific info that has its own power management modes */
>> static struct hw_vendor_info vendor_info[] = {
>> {1, "HP ", "ProLiant"},
>> + {1, "ORACLE", ""},
>> {0, "", ""},
>> };
>>
>> @@ -969,12 +985,16 @@ static bool intel_pstate_platform_pwr_mgmt_exists(void)
>> !strncmp(hdr.oem_table_id, v_info->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
>> intel_pstate_no_acpi_pss())
>> return true;
>> + if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
>> + intel_pstate_has_acpi_ppc())
>
> We need try this on a few platforms to make sure this patch doesn't break the
> HP platforms that may or may not need this driver, depending on the BIOS settings.
>

It looks like HP systems would get swept up in this check too if they have _PPC

What about extending the hw_vendor_info struct to include whether _PSS or
_PPC should be done for the platform since it appears that oracle and HP
have implemented similar functionality using two different methods.

> I don't suppose you tested on a ProLiant too?
>
> -- ljk
>
>> + return true;
>> }
>>
>> return false;
>> }
>> #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; }
>> #endif /* CONFIG_ACPI */
>>
>> static int __init intel_pstate_init(void)
>>
>

2014-11-20 21:23:21

by Kristen Carlson Accardi

[permalink] [raw]

Subject: Re: [PATCH 3/3] intel_pstate: add module and kernel command line parameter to ignore ACPI _PPC

On Thu, 20 Nov 2014 08:57:34 +0800
ethan <[email protected]> wrote:

>
>
> > ?? 2014??11??20?գ?03:05??Kristen Carlson Accardi <[email protected]> д????
> >
> > On Tue, 18 Nov 2014 17:37:06 +0900
> > Ethan Zhao <[email protected]> wrote:
> >
> >> Add kernel command line parameter
> >> intel_pstate = ignore_acpi_ppc
> >> and module parameter
> >> ignore_acpi_ppc = 1
> >> to allow driver to ignore the ACPI _PPC existence even for Sun x86 servers.
> >> These parameter could be used for debug\test\workaround etc purpose.
> >>
> >> Signed-off-by: Ethan Zhao <[email protected]>
> >
> > What if we used a more generic parameter like "force" that would bypass
> > any vendor specific checks and just load anyway? This way we don't have
> > to add new parameters everything some new thing shows up that we want to
> > ignore.
> >
> To be honest, I prefer more generic parameter. But to avoid the possible negative affect
> To another vendors. I back to this way.

Well, your parameter can still impact other vendors as it is. it
is pretty typical to assume that using a parameter like "force" means
you know what you are doing and accept the risks. Especially if its
documented as such.

>
> Thanks,
> Ethan
> >> ---
> >> Documentation/kernel-parameters.txt | 3 +++
> >> drivers/cpufreq/intel_pstate.c | 8 +++++++-
> >> 2 files changed, 10 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> >> index 4c81a86..f502b85 100644
> >> --- a/Documentation/kernel-parameters.txt
> >> +++ b/Documentation/kernel-parameters.txt
> >> @@ -1446,6 +1446,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> >> disable
> >> Do not enable intel_pstate as the default
> >> scaling driver for the supported processors
> >> + ignore_acpi_ppc
> >> + Ignore the existence of ACPI method _PPC for Sun x86 servers
> >> + and load the driver.
> >>
> >> intremap= [X86-64, Intel-IOMMU]
> >> on enable Interrupt Remapping (default)
> >> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> >> index 7c5faea..388387b 100644
> >> --- a/drivers/cpufreq/intel_pstate.c
> >> +++ b/drivers/cpufreq/intel_pstate.c
> >> @@ -870,6 +870,7 @@ static struct cpufreq_driver intel_pstate_driver = {
> >> };
> >>
> >> static int __initdata no_load;
> >> +static unsigned int ignore_acpi_ppc;
> >>
> >> static int intel_pstate_msrs_not_valid(void)
> >> {
> >> @@ -990,7 +991,7 @@ static bool intel_pstate_platform_pwr_mgmt_exists(void)
> >> intel_pstate_no_acpi_pss())
> >> return true;
> >> if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
> >> - intel_pstate_has_acpi_ppc())
> >> + intel_pstate_has_acpi_ppc() && !ignore_acpi_ppc)
> >> return true;
> >> }
> >>
> >> @@ -1066,11 +1067,16 @@ static int __init intel_pstate_setup(char *str)
> >>
> >> if (!strcmp(str, "disable"))
> >> no_load = 1;
> >> + if (!strcmp(str, "ignore_acpi_ppc"))
> >> + ignore_acpi_ppc = 1;
> >> return 0;
> >> }
> >> early_param("intel_pstate", intel_pstate_setup);
> >> #endif
> >>
> >> +module_param(ignore_acpi_ppc, uint, 0644);
> >> +MODULE_PARM_DESC(ignore_acpi_ppc,
> >> + "value 0 or non-zero. non-zero -> ignore ACPI _PPC and load this driver");
> >> MODULE_AUTHOR("Dirk Brandewie <[email protected]>");
> >> MODULE_DESCRIPTION("'intel_pstate' - P state driver Intel Core processors");
> >> MODULE_LICENSE("GPL");
> >

2014-11-21 00:37:25

[permalink] [raw]

Subject: Re: [PATCH 1/3] intel_pstate: skip the driver if Sun server has ACPI _PPC method

Dirk,

On 2014/11/21 0:50, Dirk Brandewie wrote:
> On 11/19/2014 12:22 PM, Linda Knippers wrote:
>> On 11/18/2014 3:37 AM, Ethan Zhao wrote:
>>> Oracle Sun X86 servers have dynamic power capping capability that
>>> works via
>>> ACPI _PPC method etc, so skip loading this driver if Sun server has
>>> ACPI _PPC
>>> enabled.
>>>
>>> Signed-off-by: Ethan Zhao <[email protected]>
>>> ---
>>> drivers/cpufreq/intel_pstate.c | 20 ++++++++++++++++++++
>>> 1 file changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/cpufreq/intel_pstate.c
>>> b/drivers/cpufreq/intel_pstate.c
>>> index 27bb6d3..5498eb0 100644
>>> --- a/drivers/cpufreq/intel_pstate.c
>>> +++ b/drivers/cpufreq/intel_pstate.c
>>> @@ -943,6 +943,21 @@ static bool intel_pstate_no_acpi_pss(void)
>>> return true;
>>> }
>>>
>>> +static bool intel_pstate_has_acpi_ppc(void)
>>> +{
>>> + int i;
>>> +
>>> + for_each_possible_cpu(i) {
>>> + struct acpi_processor *pr = per_cpu(processors, i);
>>> +
>>> + if (!pr)
>>> + continue;
>>> + if (acpi_has_method(pr->handle, "_PPC"))
>>> + return true;
>>> + }
>>> + return false;
>>> +}
>>> +
>>> struct hw_vendor_info {
>>> u16 valid;
>>> char oem_id[ACPI_OEM_ID_SIZE];
>>> @@ -952,6 +967,7 @@ struct hw_vendor_info {
>>> /* Hardware vendor-specific info that has its own power management
>>> modes */
>>> static struct hw_vendor_info vendor_info[] = {
>>> {1, "HP ", "ProLiant"},
>>> + {1, "ORACLE", ""},
>>> {0, "", ""},
>>> };
>>>
>>> @@ -969,12 +985,16 @@ static bool
>>> intel_pstate_platform_pwr_mgmt_exists(void)
>>> !strncmp(hdr.oem_table_id, v_info->oem_table_id,
>>> ACPI_OEM_TABLE_ID_SIZE) &&
>>> intel_pstate_no_acpi_pss())
>>> return true;
>>> + if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
>>> + intel_pstate_has_acpi_ppc())
>>
>> We need try this on a few platforms to make sure this patch doesn't
>> break the
>> HP platforms that may or may not need this driver, depending on the
>> BIOS settings.
>>
>
> It looks like HP systems would get swept up in this check too if they
> have _PPC
No , this patch checks the oem_id against 'ORACLE" first, will not
affect other vendors
even they have _PPC.

>
> What about extending the hw_vendor_info struct to include whether _PSS or
Except refer to ACPI DSDT, I don't know how to fill such info.
> _PPC should be done for the platform since it appears that oracle and HP
> have implemented similar functionality using two different methods.
Maybe Linda could answer this whether HP also has _PPC and should be
wept out.
But that doesn't happen with on the same box at the same time.

Thanks,
Ethan
>
>
>> I don't suppose you tested on a ProLiant too?
>>
>> -- ljk
>>
>>> + return true;
>>> }
>>>
>>> return false;
>>> }
>>> #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; }
>>> #endif /* CONFIG_ACPI */
>>>
>>> static int __init intel_pstate_init(void)
>>>
>>
>

2014-11-21 03:07:24

[permalink] [raw]

Subject: Re: [PATCH 3/3] intel_pstate: add module and kernel command line parameter to ignore ACPI _PPC

Kristen,
Whatever I would like there is a way to load intel_pstate and give
it a try even it does not support all the PM features.
I think 'force' is OK.
Linda,
Do you like it ? if the 'intel_pstate=force' would force the driver
to be loaded on to HP too ?

Thanks,
Ethan

On Fri, Nov 21, 2014 at 5:23 AM, Kristen Carlson Accardi
<[email protected]> wrote:
> On Thu, 20 Nov 2014 08:57:34 +0800
> ethan <[email protected]> wrote:
>
>>
>>
>> > 在 2014年11月20日，03:05，Kristen Carlson Accardi <[email protected]> 写道：
>> >
>> > On Tue, 18 Nov 2014 17:37:06 +0900
>> > Ethan Zhao <[email protected]> wrote:
>> >
>> >> Add kernel command line parameter
>> >> intel_pstate = ignore_acpi_ppc
>> >> and module parameter
>> >> ignore_acpi_ppc = 1
>> >> to allow driver to ignore the ACPI _PPC existence even for Sun x86 servers.
>> >> These parameter could be used for debug\test\workaround etc purpose.
>> >>
>> >> Signed-off-by: Ethan Zhao <[email protected]>
>> >
>> > What if we used a more generic parameter like "force" that would bypass
>> > any vendor specific checks and just load anyway? This way we don't have
>> > to add new parameters everything some new thing shows up that we want to
>> > ignore.
>> >
>> To be honest, I prefer more generic parameter. But to avoid the possible negative affect
>> To another vendors. I back to this way.
>
> Well, your parameter can still impact other vendors as it is. it
> is pretty typical to assume that using a parameter like "force" means
> you know what you are doing and accept the risks. Especially if its
> documented as such.
>
>>
>> Thanks,
>> Ethan
>> >> ---
>> >> Documentation/kernel-parameters.txt | 3 +++
>> >> drivers/cpufreq/intel_pstate.c | 8 +++++++-
>> >> 2 files changed, 10 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>> >> index 4c81a86..f502b85 100644
>> >> --- a/Documentation/kernel-parameters.txt
>> >> +++ b/Documentation/kernel-parameters.txt
>> >> @@ -1446,6 +1446,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>> >> disable
>> >> Do not enable intel_pstate as the default
>> >> scaling driver for the supported processors
>> >> + ignore_acpi_ppc
>> >> + Ignore the existence of ACPI method _PPC for Sun x86 servers
>> >> + and load the driver.
>> >>
>> >> intremap= [X86-64, Intel-IOMMU]
>> >> on enable Interrupt Remapping (default)
>> >> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>> >> index 7c5faea..388387b 100644
>> >> --- a/drivers/cpufreq/intel_pstate.c
>> >> +++ b/drivers/cpufreq/intel_pstate.c
>> >> @@ -870,6 +870,7 @@ static struct cpufreq_driver intel_pstate_driver = {
>> >> };
>> >>
>> >> static int __initdata no_load;
>> >> +static unsigned int ignore_acpi_ppc;
>> >>
>> >> static int intel_pstate_msrs_not_valid(void)
>> >> {
>> >> @@ -990,7 +991,7 @@ static bool intel_pstate_platform_pwr_mgmt_exists(void)
>> >> intel_pstate_no_acpi_pss())
>> >> return true;
>> >> if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
>> >> - intel_pstate_has_acpi_ppc())
>> >> + intel_pstate_has_acpi_ppc() && !ignore_acpi_ppc)
>> >> return true;
>> >> }
>> >>
>> >> @@ -1066,11 +1067,16 @@ static int __init intel_pstate_setup(char *str)
>> >>
>> >> if (!strcmp(str, "disable"))
>> >> no_load = 1;
>> >> + if (!strcmp(str, "ignore_acpi_ppc"))
>> >> + ignore_acpi_ppc = 1;
>> >> return 0;
>> >> }
>> >> early_param("intel_pstate", intel_pstate_setup);
>> >> #endif
>> >>
>> >> +module_param(ignore_acpi_ppc, uint, 0644);
>> >> +MODULE_PARM_DESC(ignore_acpi_ppc,
>> >> + "value 0 or non-zero. non-zero -> ignore ACPI _PPC and load this driver");
>> >> MODULE_AUTHOR("Dirk Brandewie <[email protected]>");
>> >> MODULE_DESCRIPTION("'intel_pstate' - P state driver Intel Core processors");
>> >> MODULE_LICENSE("GPL");
>> >
>

2014-11-21 04:45:01

[permalink] [raw]

Subject: Re: [PATCH 1/3] intel_pstate: skip the driver if Sun server has ACPI _PPC method

On 11/20/2014 07:37 PM, ethan zhao wrote:
> Dirk,
>
> On 2014/11/21 0:50, Dirk Brandewie wrote:
>> On 11/19/2014 12:22 PM, Linda Knippers wrote:
>>> On 11/18/2014 3:37 AM, Ethan Zhao wrote:
>>>> Oracle Sun X86 servers have dynamic power capping capability that
>>>> works via
>>>> ACPI _PPC method etc, so skip loading this driver if Sun server has
>>>> ACPI _PPC
>>>> enabled.
>>>>
>>>> Signed-off-by: Ethan Zhao <[email protected]>
>>>> ---
>>>> drivers/cpufreq/intel_pstate.c | 20 ++++++++++++++++++++
>>>> 1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/cpufreq/intel_pstate.c
>>>> b/drivers/cpufreq/intel_pstate.c
>>>> index 27bb6d3..5498eb0 100644
>>>> --- a/drivers/cpufreq/intel_pstate.c
>>>> +++ b/drivers/cpufreq/intel_pstate.c
>>>> @@ -943,6 +943,21 @@ static bool intel_pstate_no_acpi_pss(void)
>>>> return true;
>>>> }
>>>>
>>>> +static bool intel_pstate_has_acpi_ppc(void)
>>>> +{
>>>> + int i;
>>>> +
>>>> + for_each_possible_cpu(i) {
>>>> + struct acpi_processor *pr = per_cpu(processors, i);
>>>> +
>>>> + if (!pr)
>>>> + continue;
>>>> + if (acpi_has_method(pr->handle, "_PPC"))
>>>> + return true;
>>>> + }
>>>> + return false;
>>>> +}
>>>> +
>>>> struct hw_vendor_info {
>>>> u16 valid;
>>>> char oem_id[ACPI_OEM_ID_SIZE];
>>>> @@ -952,6 +967,7 @@ struct hw_vendor_info {
>>>> /* Hardware vendor-specific info that has its own power management
>>>> modes */
>>>> static struct hw_vendor_info vendor_info[] = {
>>>> {1, "HP ", "ProLiant"},
>>>> + {1, "ORACLE", ""},
>>>> {0, "", ""},
>>>> };
>>>>
>>>> @@ -969,12 +985,16 @@ static bool
>>>> intel_pstate_platform_pwr_mgmt_exists(void)
>>>> !strncmp(hdr.oem_table_id, v_info->oem_table_id,
>>>> ACPI_OEM_TABLE_ID_SIZE) &&
>>>> intel_pstate_no_acpi_pss())
>>>> return true;
>>>> + if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
>>>> + intel_pstate_has_acpi_ppc())
>>>
>>> We need try this on a few platforms to make sure this patch doesn't
>>> break the
>>> HP platforms that may or may not need this driver, depending on the
>>> BIOS settings.
>>>
>>
>> It looks like HP systems would get swept up in this check too if they
>> have _PPC

Right. This patch breaks HP ProLiant platforms when they are
configured to have the OS do power management. In that case,
the firmware exposes _PPC information.

> No , this patch checks the oem_id against 'ORACLE" first, will not
> affect other vendors even they have _PPC.

I don't think that's how your code works. This patch will match any
vendor that is in the table, not just "ORACLE".
>
>>
>> What about extending the hw_vendor_info struct to include whether _PSS or
> Except refer to ACPI DSDT, I don't know how to fill such info.
>> _PPC should be done for the platform since it appears that oracle and HP
>> have implemented similar functionality using two different methods.
> Maybe Linda could answer this whether HP also has _PPC and should be
> wept out.
> But that doesn't happen with on the same box at the same time.

I don't know how an Oracle box works but on a ProLiant, customers can
choose to have platform power management or OS power management.
When the platform is managing the power, we don't provide the _PSS
information. Since our oem information is in the table and there is
no _PSS, the intel_pstate driver doesn't stay loaded. That's what we want.

When the platform configured to have the OS do the power management,
the firmware has _PSS and _PPC and we want the intel_pstate driver,
That's what your patch breaks. With your patch, the driver won't
stay loaded because our platform is in the table and the check for
_PPC passes.

How does an Oracle box work?

-- ljk

>
> Thanks,
> Ethan
>>
>>
>>> I don't suppose you tested on a ProLiant too?
>>>
>>> -- ljk
>>>
>>>> + return true;
>>>> }
>>>>
>>>> return false;
>>>> }
>>>> #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; }
>>>> #endif /* CONFIG_ACPI */
>>>>
>>>> static int __init intel_pstate_init(void)
>>>>
>>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-11-21 05:00:51

[permalink] [raw]

Subject: Re: [PATCH 3/3] intel_pstate: add module and kernel command line parameter to ignore ACPI _PPC

On 11/20/2014 10:07 PM, Ethan Zhao wrote:
> Kristen,
> Whatever I would like there is a way to load intel_pstate and give
> it a try even it does not support all the PM features.
> I think 'force' is OK.
> Linda,
> Do you like it ? if the 'intel_pstate=force' would force the driver
> to be loaded on to HP too ?

I'd prefer that it didn't. If you force the intel_pstate driver when
the platform thinks it's doing power management, then the OS and the
firmware are trying to manage the power at the same time. That's a
mess. If you want that for testing or debugging, what are you actually
testing or debugging? On an Oracle box, the firmware wouldn't stop
doing whatever it's doing just because the intel_pstate driver is
loaded, would it?

I also wonder what it means to "force" the intel_pstate driver
on systems with processors that aren't supported by the intel_pstate
driver. It wouldn't really be forced, would it?

-- ljk

>
> Thanks,
> Ethan
>
> On Fri, Nov 21, 2014 at 5:23 AM, Kristen Carlson Accardi
> <[email protected]> wrote:
>> On Thu, 20 Nov 2014 08:57:34 +0800
>> ethan <[email protected]> wrote:
>>
>>>
>>>
>>>> 在 2014年11月20日，03:05，Kristen Carlson Accardi <[email protected]> 写道：
>>>>
>>>> On Tue, 18 Nov 2014 17:37:06 +0900
>>>> Ethan Zhao <[email protected]> wrote:
>>>>
>>>>> Add kernel command line parameter
>>>>> intel_pstate = ignore_acpi_ppc
>>>>> and module parameter
>>>>> ignore_acpi_ppc = 1
>>>>> to allow driver to ignore the ACPI _PPC existence even for Sun x86 servers.
>>>>> These parameter could be used for debug\test\workaround etc purpose.
>>>>>
>>>>> Signed-off-by: Ethan Zhao <[email protected]>
>>>>
>>>> What if we used a more generic parameter like "force" that would bypass
>>>> any vendor specific checks and just load anyway? This way we don't have
>>>> to add new parameters everything some new thing shows up that we want to
>>>> ignore.
>>>>
>>> To be honest, I prefer more generic parameter. But to avoid the possible negative affect
>>> To another vendors. I back to this way.
>>
>> Well, your parameter can still impact other vendors as it is. it
>> is pretty typical to assume that using a parameter like "force" means
>> you know what you are doing and accept the risks. Especially if its
>> documented as such.
>>
>>>
>>> Thanks,
>>> Ethan
>>>>> ---
>>>>> Documentation/kernel-parameters.txt | 3 +++
>>>>> drivers/cpufreq/intel_pstate.c | 8 +++++++-
>>>>> 2 files changed, 10 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>>>>> index 4c81a86..f502b85 100644
>>>>> --- a/Documentation/kernel-parameters.txt
>>>>> +++ b/Documentation/kernel-parameters.txt
>>>>> @@ -1446,6 +1446,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>>>>> disable
>>>>> Do not enable intel_pstate as the default
>>>>> scaling driver for the supported processors
>>>>> + ignore_acpi_ppc
>>>>> + Ignore the existence of ACPI method _PPC for Sun x86 servers
>>>>> + and load the driver.
>>>>>
>>>>> intremap= [X86-64, Intel-IOMMU]
>>>>> on enable Interrupt Remapping (default)
>>>>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>>>>> index 7c5faea..388387b 100644
>>>>> --- a/drivers/cpufreq/intel_pstate.c
>>>>> +++ b/drivers/cpufreq/intel_pstate.c
>>>>> @@ -870,6 +870,7 @@ static struct cpufreq_driver intel_pstate_driver = {
>>>>> };
>>>>>
>>>>> static int __initdata no_load;
>>>>> +static unsigned int ignore_acpi_ppc;
>>>>>
>>>>> static int intel_pstate_msrs_not_valid(void)
>>>>> {
>>>>> @@ -990,7 +991,7 @@ static bool intel_pstate_platform_pwr_mgmt_exists(void)
>>>>> intel_pstate_no_acpi_pss())
>>>>> return true;
>>>>> if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
>>>>> - intel_pstate_has_acpi_ppc())
>>>>> + intel_pstate_has_acpi_ppc() && !ignore_acpi_ppc)
>>>>> return true;
>>>>> }
>>>>>
>>>>> @@ -1066,11 +1067,16 @@ static int __init intel_pstate_setup(char *str)
>>>>>
>>>>> if (!strcmp(str, "disable"))
>>>>> no_load = 1;
>>>>> + if (!strcmp(str, "ignore_acpi_ppc"))
>>>>> + ignore_acpi_ppc = 1;
>>>>> return 0;
>>>>> }
>>>>> early_param("intel_pstate", intel_pstate_setup);
>>>>> #endif
>>>>>
>>>>> +module_param(ignore_acpi_ppc, uint, 0644);
>>>>> +MODULE_PARM_DESC(ignore_acpi_ppc,
>>>>> + "value 0 or non-zero. non-zero -> ignore ACPI _PPC and load this driver");
>>>>> MODULE_AUTHOR("Dirk Brandewie <[email protected]>");
>>>>> MODULE_DESCRIPTION("'intel_pstate' - P state driver Intel Core processors");
>>>>> MODULE_LICENSE("GPL");
>>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2014-11-24 01:41:41

[permalink] [raw]

Subject: Re: [PATCH 1/3] intel_pstate: skip the driver if Sun server has ACPI _PPC method

Linda,

On 2014/11/21 12:44, Linda Knippers wrote:
>
> On 11/20/2014 07:37 PM, ethan zhao wrote:
>> Dirk,
>>
>> On 2014/11/21 0:50, Dirk Brandewie wrote:
>>> On 11/19/2014 12:22 PM, Linda Knippers wrote:
>>>> On 11/18/2014 3:37 AM, Ethan Zhao wrote:
>>>>> Oracle Sun X86 servers have dynamic power capping capability that
>>>>> works via
>>>>> ACPI _PPC method etc, so skip loading this driver if Sun server has
>>>>> ACPI _PPC
>>>>> enabled.
>>>>>
>>>>> Signed-off-by: Ethan Zhao <[email protected]>
>>>>> ---
>>>>> drivers/cpufreq/intel_pstate.c | 20 ++++++++++++++++++++
>>>>> 1 file changed, 20 insertions(+)
>>>>>
>>>>> diff --git a/drivers/cpufreq/intel_pstate.c
>>>>> b/drivers/cpufreq/intel_pstate.c
>>>>> index 27bb6d3..5498eb0 100644
>>>>> --- a/drivers/cpufreq/intel_pstate.c
>>>>> +++ b/drivers/cpufreq/intel_pstate.c
>>>>> @@ -943,6 +943,21 @@ static bool intel_pstate_no_acpi_pss(void)
>>>>> return true;
>>>>> }
>>>>>
>>>>> +static bool intel_pstate_has_acpi_ppc(void)
>>>>> +{
>>>>> + int i;
>>>>> +
>>>>> + for_each_possible_cpu(i) {
>>>>> + struct acpi_processor *pr = per_cpu(processors, i);
>>>>> +
>>>>> + if (!pr)
>>>>> + continue;
>>>>> + if (acpi_has_method(pr->handle, "_PPC"))
>>>>> + return true;
>>>>> + }
>>>>> + return false;
>>>>> +}
>>>>> +
>>>>> struct hw_vendor_info {
>>>>> u16 valid;
>>>>> char oem_id[ACPI_OEM_ID_SIZE];
>>>>> @@ -952,6 +967,7 @@ struct hw_vendor_info {
>>>>> /* Hardware vendor-specific info that has its own power management
>>>>> modes */
>>>>> static struct hw_vendor_info vendor_info[] = {
>>>>> {1, "HP ", "ProLiant"},
>>>>> + {1, "ORACLE", ""},
>>>>> {0, "", ""},
>>>>> };
>>>>>
>>>>> @@ -969,12 +985,16 @@ static bool
>>>>> intel_pstate_platform_pwr_mgmt_exists(void)
>>>>> !strncmp(hdr.oem_table_id, v_info->oem_table_id,
>>>>> ACPI_OEM_TABLE_ID_SIZE) &&
>>>>> intel_pstate_no_acpi_pss())
>>>>> return true;
>>>>> + if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
>>>>> + intel_pstate_has_acpi_ppc())
>>>> We need try this on a few platforms to make sure this patch doesn't
>>>> break the
>>>> HP platforms that may or may not need this driver, depending on the
>>>> BIOS settings.
>>>>
>>> It looks like HP systems would get swept up in this check too if they
>>> have _PPC
> Right. This patch breaks HP ProLiant platforms when they are
> configured to have the OS do power management. In that case,
> the firmware exposes _PPC information.
Okay, got it, The HP ProLiant has an option in BIOS could be enabled
to "OS PM", so
will export _PSS, _PPC, and this patch break this case.

>
>> No , this patch checks the oem_id against 'ORACLE" first, will not
>> affect other vendors even they have _PPC.
> I don't think that's how your code works. This patch will match any
> vendor that is in the table, not just "ORACLE".
Will change patch to match the oem-id out of the loop, such as
following , how about it ?

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

if (!strncmp(hdr.oem_id, v_info[1]->oem_id, ACPI_OEM_ID_SIZE) &&
intel_pstate_has_acpi_ppc())
return true;

return false;
}

>>> What about extending the hw_vendor_info struct to include whether _PSS or
>> Except refer to ACPI DSDT, I don't know how to fill such info.
>>> _PPC should be done for the platform since it appears that oracle and HP
>>> have implemented similar functionality using two different methods.
>> Maybe Linda could answer this whether HP also has _PPC and should be
>> wept out.
>> But that doesn't happen with on the same box at the same time.
> I don't know how an Oracle box works but on a ProLiant, customers can
> choose to have platform power management or OS power management.
> When the platform is managing the power, we don't provide the _PSS
> information. Since our oem information is in the table and there is
> no _PSS, the intel_pstate driver doesn't stay loaded. That's what we want.
>
> When the platform configured to have the OS do the power management,
> the firmware has _PSS and _PPC and we want the intel_pstate driver,
> That's what your patch breaks. With your patch, the driver won't
> stay loaded because our platform is in the table and the check for
> _PPC passes.
>
> How does an Oracle box work?
Oracle Sun servers (X86) don't have the option in BIOS to change the
PM mode to firmware/OS,
The BIOS always has _PSS and _PPC exported to OS whatever 'soft power
capping' or 'hard power capping' enabled
in SP configuration web page. if the power policy violation happened,
firmware will notify OS via SCI with the changed _PPC
number.

Thanks,
Ethan
>
> -- ljk
>
>> Thanks,
>> Ethan
>>>
>>>> I don't suppose you tested on a ProLiant too?
>>>>
>>>> -- ljk
>>>>
>>>>> + return true;
>>>>> }
>>>>>
>>>>> return false;
>>>>> }
>>>>> #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; }
>>>>> #endif /* CONFIG_ACPI */
>>>>>
>>>>> static int __init intel_pstate_init(void)
>>>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-11-24 01:56:49

[permalink] [raw]

Subject: Re: [PATCH 3/3] intel_pstate: add module and kernel command line parameter to ignore ACPI _PPC

Linda,

On 2014/11/21 13:00, Linda Knippers wrote:
>
> On 11/20/2014 10:07 PM, Ethan Zhao wrote:
>> Kristen,
>> Whatever I would like there is a way to load intel_pstate and give
>> it a try even it does not support all the PM features.
>> I think 'force' is OK.
>> Linda,
>> Do you like it ? if the 'intel_pstate=force' would force the driver
>> to be loaded on to HP too ?
> I'd prefer that it didn't. If you force the intel_pstate driver when
> the platform thinks it's doing power management, then the OS and the
> firmware are trying to manage the power at the same time. That's a
> mess. If you want that for testing or debugging, what are you actually
> testing or debugging? On an Oracle box, the firmware wouldn't stop
> doing whatever it's doing just because the intel_pstate driver is
> loaded, would it?
Yes, the platform wouldn't stop doing PM and the SCI even the intel_pstate
was loaded.
That option just give someone a chance who said "whatever I like
intel_pstate
On Oracle Servers"
>
> I also wonder what it means to "force" the intel_pstate driver
> on systems with processors that aren't supported by the intel_pstate
> driver. It wouldn't really be forced, would it?
Yes, wouldn't, such as AMD cpu-id and some old intel CPUs released
before SandyBridge.
So the best way to to do the 'force' should be specific enough for
"Oracle Sun server"

Thanks,
Ethan
>
> -- ljk
>
>> Thanks,
>> Ethan
>>
>> On Fri, Nov 21, 2014 at 5:23 AM, Kristen Carlson Accardi
>> <[email protected]> wrote:
>>> On Thu, 20 Nov 2014 08:57:34 +0800
>>> ethan <[email protected]> wrote:
>>>
>>>>
>>>>> 在 2014年11月20日，03:05，Kristen Carlson Accardi <[email protected]> 写道：
>>>>>
>>>>> On Tue, 18 Nov 2014 17:37:06 +0900
>>>>> Ethan Zhao <[email protected]> wrote:
>>>>>
>>>>>> Add kernel command line parameter
>>>>>> intel_pstate = ignore_acpi_ppc
>>>>>> and module parameter
>>>>>> ignore_acpi_ppc = 1
>>>>>> to allow driver to ignore the ACPI _PPC existence even for Sun x86 servers.
>>>>>> These parameter could be used for debug\test\workaround etc purpose.
>>>>>>
>>>>>> Signed-off-by: Ethan Zhao <[email protected]>
>>>>> What if we used a more generic parameter like "force" that would bypass
>>>>> any vendor specific checks and just load anyway? This way we don't have
>>>>> to add new parameters everything some new thing shows up that we want to
>>>>> ignore.
>>>>>
>>>> To be honest, I prefer more generic parameter. But to avoid the possible negative affect
>>>> To another vendors. I back to this way.
>>> Well, your parameter can still impact other vendors as it is. it
>>> is pretty typical to assume that using a parameter like "force" means
>>> you know what you are doing and accept the risks. Especially if its
>>> documented as such.
>>>
>>>> Thanks,
>>>> Ethan
>>>>>> ---
>>>>>> Documentation/kernel-parameters.txt | 3 +++
>>>>>> drivers/cpufreq/intel_pstate.c | 8 +++++++-
>>>>>> 2 files changed, 10 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>>>>>> index 4c81a86..f502b85 100644
>>>>>> --- a/Documentation/kernel-parameters.txt
>>>>>> +++ b/Documentation/kernel-parameters.txt
>>>>>> @@ -1446,6 +1446,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>>>>>> disable
>>>>>> Do not enable intel_pstate as the default
>>>>>> scaling driver for the supported processors
>>>>>> + ignore_acpi_ppc
>>>>>> + Ignore the existence of ACPI method _PPC for Sun x86 servers
>>>>>> + and load the driver.
>>>>>>
>>>>>> intremap= [X86-64, Intel-IOMMU]
>>>>>> on enable Interrupt Remapping (default)
>>>>>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>>>>>> index 7c5faea..388387b 100644
>>>>>> --- a/drivers/cpufreq/intel_pstate.c
>>>>>> +++ b/drivers/cpufreq/intel_pstate.c
>>>>>> @@ -870,6 +870,7 @@ static struct cpufreq_driver intel_pstate_driver = {
>>>>>> };
>>>>>>
>>>>>> static int __initdata no_load;
>>>>>> +static unsigned int ignore_acpi_ppc;
>>>>>>
>>>>>> static int intel_pstate_msrs_not_valid(void)
>>>>>> {
>>>>>> @@ -990,7 +991,7 @@ static bool intel_pstate_platform_pwr_mgmt_exists(void)
>>>>>> intel_pstate_no_acpi_pss())
>>>>>> return true;
>>>>>> if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
>>>>>> - intel_pstate_has_acpi_ppc())
>>>>>> + intel_pstate_has_acpi_ppc() && !ignore_acpi_ppc)
>>>>>> return true;
>>>>>> }
>>>>>>
>>>>>> @@ -1066,11 +1067,16 @@ static int __init intel_pstate_setup(char *str)
>>>>>>
>>>>>> if (!strcmp(str, "disable"))
>>>>>> no_load = 1;
>>>>>> + if (!strcmp(str, "ignore_acpi_ppc"))
>>>>>> + ignore_acpi_ppc = 1;
>>>>>> return 0;
>>>>>> }
>>>>>> early_param("intel_pstate", intel_pstate_setup);
>>>>>> #endif
>>>>>>
>>>>>> +module_param(ignore_acpi_ppc, uint, 0644);
>>>>>> +MODULE_PARM_DESC(ignore_acpi_ppc,
>>>>>> + "value 0 or non-zero. non-zero -> ignore ACPI _PPC and load this driver");
>>>>>> MODULE_AUTHOR("Dirk Brandewie <[email protected]>");
>>>>>> MODULE_DESCRIPTION("'intel_pstate' - P state driver Intel Core processors");
>>>>>> MODULE_LICENSE("GPL");
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>

2014-11-24 15:54:28

[permalink] [raw]

Subject: Re: [PATCH 1/3] intel_pstate: skip the driver if Sun server has ACPI _PPC method

On 11/23/2014 8:41 PM, ethan zhao wrote:
> Linda,
>
> On 2014/11/21 12:44, Linda Knippers wrote:
>>
>> On 11/20/2014 07:37 PM, ethan zhao wrote:
>>> Dirk,
>>>
>>> On 2014/11/21 0:50, Dirk Brandewie wrote:
>>>> On 11/19/2014 12:22 PM, Linda Knippers wrote:
>>>>> On 11/18/2014 3:37 AM, Ethan Zhao wrote:
>>>>>> Oracle Sun X86 servers have dynamic power capping capability that
>>>>>> works via
>>>>>> ACPI _PPC method etc, so skip loading this driver if Sun server has
>>>>>> ACPI _PPC
>>>>>> enabled.
>>>>>>
>>>>>> Signed-off-by: Ethan Zhao <[email protected]>
>>>>>> ---
>>>>>> drivers/cpufreq/intel_pstate.c | 20 ++++++++++++++++++++
>>>>>> 1 file changed, 20 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/cpufreq/intel_pstate.c
>>>>>> b/drivers/cpufreq/intel_pstate.c
>>>>>> index 27bb6d3..5498eb0 100644
>>>>>> --- a/drivers/cpufreq/intel_pstate.c
>>>>>> +++ b/drivers/cpufreq/intel_pstate.c
>>>>>> @@ -943,6 +943,21 @@ static bool intel_pstate_no_acpi_pss(void)
>>>>>> return true;
>>>>>> }
>>>>>>
>>>>>> +static bool intel_pstate_has_acpi_ppc(void)
>>>>>> +{
>>>>>> + int i;
>>>>>> +
>>>>>> + for_each_possible_cpu(i) {
>>>>>> + struct acpi_processor *pr = per_cpu(processors, i);
>>>>>> +
>>>>>> + if (!pr)
>>>>>> + continue;
>>>>>> + if (acpi_has_method(pr->handle, "_PPC"))
>>>>>> + return true;
>>>>>> + }
>>>>>> + return false;
>>>>>> +}
>>>>>> +
>>>>>> struct hw_vendor_info {
>>>>>> u16 valid;
>>>>>> char oem_id[ACPI_OEM_ID_SIZE];
>>>>>> @@ -952,6 +967,7 @@ struct hw_vendor_info {
>>>>>> /* Hardware vendor-specific info that has its own power management
>>>>>> modes */
>>>>>> static struct hw_vendor_info vendor_info[] = {
>>>>>> {1, "HP ", "ProLiant"},
>>>>>> + {1, "ORACLE", ""},
>>>>>> {0, "", ""},
>>>>>> };
>>>>>>
>>>>>> @@ -969,12 +985,16 @@ static bool
>>>>>> intel_pstate_platform_pwr_mgmt_exists(void)
>>>>>> !strncmp(hdr.oem_table_id, v_info->oem_table_id,
>>>>>> ACPI_OEM_TABLE_ID_SIZE) &&
>>>>>> intel_pstate_no_acpi_pss())
>>>>>> return true;
>>>>>> + if (!strncmp(hdr.oem_id, v_info->oem_id, ACPI_OEM_ID_SIZE) &&
>>>>>> + intel_pstate_has_acpi_ppc())
>>>>> We need try this on a few platforms to make sure this patch doesn't
>>>>> break the
>>>>> HP platforms that may or may not need this driver, depending on the
>>>>> BIOS settings.
>>>>>
>>>> It looks like HP systems would get swept up in this check too if they
>>>> have _PPC
>> Right. This patch breaks HP ProLiant platforms when they are
>> configured to have the OS do power management. In that case,
>> the firmware exposes _PPC information.
> Okay, got it, The HP ProLiant has an option in BIOS could be enabled to "OS
> PM", so
> will export _PSS, _PPC, and this patch break this case.
>
>>
>>> No , this patch checks the oem_id against 'ORACLE" first, will not
>>> affect other vendors even they have _PPC.
>> I don't think that's how your code works. This patch will match any
>> vendor that is in the table, not just "ORACLE".
> Will change patch to match the oem-id out of the loop, such as following , how
> about it ?
>
> 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;
> }
>
> if (!strncmp(hdr.oem_id, v_info[1]->oem_id, ACPI_OEM_ID_SIZE) &&
> intel_pstate_has_acpi_ppc())

I really don't think you want to hard code a 1 there.

I think you need to do what Dirk suggested, which is to expand the
hw_vendor_info structure to specify the check that needs to be done
for each entry. For a ProLiant, it would be to call intel_pstate_no_acpi_pss()
and for an Oracle box, it would be to call intel_pstate_has_acpi_ppc().

-- ljk

> return true;
>
> return false;
> }
>
>>>> What about extending the hw_vendor_info struct to include whether _PSS or
>>> Except refer to ACPI DSDT, I don't know how to fill such info.
>>>> _PPC should be done for the platform since it appears that oracle and HP
>>>> have implemented similar functionality using two different methods.
>>> Maybe Linda could answer this whether HP also has _PPC and should be
>>> wept out.
>>> But that doesn't happen with on the same box at the same time.
>> I don't know how an Oracle box works but on a ProLiant, customers can
>> choose to have platform power management or OS power management.
>> When the platform is managing the power, we don't provide the _PSS
>> information. Since our oem information is in the table and there is
>> no _PSS, the intel_pstate driver doesn't stay loaded. That's what we want.
>>
>> When the platform configured to have the OS do the power management,
>> the firmware has _PSS and _PPC and we want the intel_pstate driver,
>> That's what your patch breaks. With your patch, the driver won't
>> stay loaded because our platform is in the table and the check for
>> _PPC passes.
>>
>> How does an Oracle box work?
> Oracle Sun servers (X86) don't have the option in BIOS to change the PM mode
> to firmware/OS,
> The BIOS always has _PSS and _PPC exported to OS whatever 'soft power capping'
> or 'hard power capping' enabled
> in SP configuration web page. if the power policy violation happened, firmware
> will notify OS via SCI with the changed _PPC
> number.
>
> Thanks,
> Ethan
>>
>> -- ljk
>>
>>> Thanks,
>>> Ethan
>>>>
>>>>> I don't suppose you tested on a ProLiant too?
>>>>>
>>>>> -- ljk
>>>>>
>>>>>> + return true;
>>>>>> }
>>>>>>
>>>>>> return false;
>>>>>> }
>>>>>> #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; }
>>>>>> #endif /* CONFIG_ACPI */
>>>>>>
>>>>>> static int __init intel_pstate_init(void)
>>>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2014-11-25 00:33:18