lkml.org 
[lkml]   [2014]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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 <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
    >



    \
     
     \ /
      Last update: 2014-11-24 17:21    [W:5.889 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site