| Message ID | 1416299826-15813-2-git-send-email-ethan.zhao@oracle.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 <ethan.zhao@oracle.com> > --- > 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) > -- 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
Linda? > ? 2014?11?20??04:22?Linda Knippers <linda.knippers@hp.com> ??? > >> 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 <ethan.zhao@oracle.com> >> --- >> 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) >> > -- 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
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 <ethan.zhao@oracle.com> >> --- >> 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) >> > -- 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
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 <ethan.zhao@oracle.com> >>> --- >>> 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) >>> >> > -- 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
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 <ethan.zhao@oracle.com> >>>> --- >>>> 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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
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 <ethan.zhao@oracle.com> >>>>> --- >>>>> 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 majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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
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 <ethan.zhao@oracle.com> >>>>>> --- >>>>>> 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 majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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
Linda? > ? 2014?11?24??23:54?Linda Knippers <linda.knippers@hp.com> ??? > >> 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 <ethan.zhao@oracle.com> >>>>>>> --- >>>>>>> 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(). > thanks?will do that. Ethan > -- 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 majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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
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)
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 <ethan.zhao@oracle.com> --- drivers/cpufreq/intel_pstate.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)