Re: [PATCH v3 3/5] acpi: Introduce new function callback for _OSC

["Wilczynski, Michal" <michal.wilczynski@intel.com>]

On 6/30/2023 10:46 AM, Wilczynski, Michal wrote:
> Hi,
> Thanks for the review !
>
> On 6/29/2023 1:04 PM, Rafael J. Wysocki wrote:
>> I would just say "Introduce acpi_processor_osc()" in the subject and
>> then explain its role in the changelog.
> Sure,
>
>> On Tue, Jun 13, 2023 at 6:12 PM Michal Wilczynski
>> <michal.wilczynski@intel.com> wrote:
>>> Currently in ACPI code _OSC method is already used for workaround
>>> introduced in commit a21211672c9a ("ACPI / processor: Request native
>>> thermal interrupt handling via _OSC"). Create new function, similar to
>>> already existing acpi_hwp_native_thermal_lvt_osc(). Call new function
>>> acpi_processor_osc(). Make this function fulfill the purpose previously
>>> fulfilled by the workaround plus convey OSPM processor capabilities
>>> with it by setting correct processor capability bits.
>>>
>>> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> ---
>>>  arch/x86/include/asm/acpi.h   |  3 +++
>>>  drivers/acpi/acpi_processor.c | 43 ++++++++++++++++++++++++++++++++++-
>>>  include/acpi/pdc_intel.h      |  1 +
>>>  3 files changed, 46 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
>>> index 6a498d1781e7..6c25ce2dad18 100644
>>> --- a/arch/x86/include/asm/acpi.h
>>> +++ b/arch/x86/include/asm/acpi.h
>>> @@ -112,6 +112,9 @@ static inline void arch_acpi_set_proc_cap_bits(u32 *cap)
>>>         if (cpu_has(c, X86_FEATURE_ACPI))
>>>                 *cap |= ACPI_PDC_T_FFH;
>>>
>>> +       if (cpu_has(c, X86_FEATURE_HWP))
>>> +               *cap |= ACPI_PDC_COLLAB_PROC_PERF;
>>> +
>>>         /*
>>>          * If mwait/monitor is unsupported, C2/C3_FFH will be disabled
>>>          */
>>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
>>> index 8c5d0295a042..0de0b05b6f53 100644
>>> --- a/drivers/acpi/acpi_processor.c
>>> +++ b/drivers/acpi/acpi_processor.c
>>> @@ -591,13 +591,54 @@ void __init processor_dmi_check(void)
>>>         dmi_check_system(processor_idle_dmi_table);
>>>  }
>>>
>>> +/* vendor specific UUID indicating an Intel platform */
>>> +static u8 sb_uuid_str[] = "4077A616-290C-47BE-9EBD-D87058713953";
>>>  static bool acpi_hwp_native_thermal_lvt_set;
>>> +static acpi_status __init acpi_processor_osc(acpi_handle handle, u32 lvl,
>>> +                                            void *context, void **rv)
>>> +{
>>> +       u32 capbuf[2] = {};
>>> +       acpi_status status;
>>> +       struct acpi_osc_context osc_context = {
>>> +               .uuid_str = sb_uuid_str,
>>> +               .rev = 1,
>>> +               .cap.length = 8,
>>> +               .cap.pointer = capbuf,
>>> +       };
>>> +
>>> +       if (processor_physically_present(handle) == false)
>> if (!processor_physically_present(handle))
> Sure,
>
>>> +               return AE_OK;
>>> +
>>> +       arch_acpi_set_proc_cap_bits(&capbuf[OSC_SUPPORT_DWORD]);
>>> +
>>> +       if (boot_option_idle_override == IDLE_NOMWAIT)
>>> +               capbuf[OSC_SUPPORT_DWORD] &=
>>> +                       ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
>>> +
>>> +       status = acpi_run_osc(handle, &osc_context);
>>> +       if (ACPI_FAILURE(status))
>>> +               return status;
>>> +
>>> +       if (osc_context.ret.pointer && osc_context.ret.length > 1) {
>>> +               u32 *capbuf_ret = osc_context.ret.pointer;
>>> +
>>> +               if (!acpi_hwp_native_thermal_lvt_set &&
>>> +                   capbuf_ret[1] & ACPI_PDC_COLLAB_PROC_PERF) {
>> Checking it in capbuf_ret[] if it was not set in capbuf[] is sort of
>> questionable.
>> Note that acpi_hwp_native_thermal_lvt_osc() sets it in capbuf[] before
>> calling acpi_run_osc().
> We can add condition before checking capbuf_ret i.e
>
> if (capbuf[OSC_SUPPORT_DWORD] & ACPI_PDC_COLLAB_PROC_PERF &&
>     osc_context.ret.pointer && osc_context.ret.length > 1)
>  
>
>>> +                       acpi_handle_info(handle,
>>> +                                        "_OSC native thermal LVT Acked\n");
>>> +                       acpi_hwp_native_thermal_lvt_set = true;
>>> +               }
>>> +       }
>>> +       kfree(osc_context.ret.pointer);
>>> +
>>> +       return AE_OK;
>>> +}
>>> +
>>>  static acpi_status __init acpi_hwp_native_thermal_lvt_osc(acpi_handle handle,
>>>                                                           u32 lvl,
>>>                                                           void *context,
>>>                                                           void **rv)
>>>  {
>>> -       u8 sb_uuid_str[] = "4077A616-290C-47BE-9EBD-D87058713953";
>>>         u32 capbuf[2];
>>>         struct acpi_osc_context osc_context = {
>>>                 .uuid_str = sb_uuid_str,
>>> diff --git a/include/acpi/pdc_intel.h b/include/acpi/pdc_intel.h
>>> index 967c552d1cd3..9427f639287f 100644
>>> --- a/include/acpi/pdc_intel.h
>>> +++ b/include/acpi/pdc_intel.h
>>> @@ -16,6 +16,7 @@
>>>  #define ACPI_PDC_C_C1_FFH              (0x0100)
>>>  #define ACPI_PDC_C_C2C3_FFH            (0x0200)
>>>  #define ACPI_PDC_SMP_P_HWCOORD         (0x0800)
>>> +#define ACPI_PDC_COLLAB_PROC_PERF      (0x1000)
>> I would call this ACPI_OSC_COLLAB_PROC_PERF to avoid confusion.
>>
>> It may also be a good idea to introduce ACPI_OSC_ symbols to replace
>> the existing ACPI_PDC_ ones (with the same values, respectively) and
>> get rid of the latter later.
> Sure I can do that, most likely in a separate commit preceeding this one, so
> it's easier to explain and review,

Actually on a second thought, maybe instead of creating _OSC specifc constants it would
be better to just generalize constant names ? As they're the same for both methods, they
are not really method specific and could be called as follows:

ACPI_PROC_CAP_C_C1_FFH
ACPI_PROC_CAP_C_C2C3_FFH

So instead of using OSC, or PDC, we just use PROC_CAP, which better explain what those bits
mean at the end, and removes the hassle of removing those PDC specifc constants in some far
away future.

Please let me know your thoughts,

Michał



>
>>>  #define ACPI_PDC_EST_CAPABILITY_SMP    (ACPI_PDC_SMP_C1PT | \
>>>                                          ACPI_PDC_C_C1_HALT | \
>>> --