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

[Rafael J. Wysocki]

On Mon, Jul 3, 2023 at 11:52 AM Wilczynski, Michal
<michal.wilczynski@xxxxxxxxx> wrote:
>
>
>
> On 6/30/2023 11:23 AM, Wilczynski, Michal wrote:
> >
> > On 6/30/2023 11:10 AM, Rafael J. Wysocki wrote:
> >> On Fri, Jun 30, 2023 at 11:02 AM Wilczynski, Michal
> >> <michal.wilczynski@xxxxxxxxx> wrote:
> >>>
> >>> On 6/29/2023 3:15 PM, Rafael J. Wysocki wrote:
> >>>> On Thu, Jun 29, 2023 at 1:04 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> >>>>> I would just say "Introduce acpi_processor_osc()" in the subject and
> >>>>> then explain its role in the changelog.
> >>>>>
> >>>>> On Tue, Jun 13, 2023 at 6:12 PM Michal Wilczynski
> >>>>> <michal.wilczynski@xxxxxxxxx> 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@xxxxxxxxx>
> >>>>>> Signed-off-by: Michal Wilczynski <michal.wilczynski@xxxxxxxxx>
> >>>>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> >>>>>> ---
> >>>>>>  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))
> >>>>>
> >>>>>> +               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().
> >>>> So you moved setting it to arch_acpi_set_proc_cap_bits(), but then it
> >>>> should also be checked by the arch code.  That is, add an arch
> >>>> function to check if a given bit is set in the returned capabilities
> >>>> buffer (passed as an argument).
> >>> Hmm, maybe that's true, but the only reason we check that is so we can print
> >>> a debug message
> >> No, it is not the only reason.  The more important part is to set
> >> acpi_hwp_native_thermal_lvt_set if it is still unset at that point.
> > Yeah, that too. Okay I'll modify the code
> >
> >>>  - that's pretty much a leftover after a workaround. Introducing
> >>> more arch code to accommodate this seemed wasteful, since in my understanding
> >>> all workarounds are meant to be removed at some point, even if it takes a long time
> >>> to do so.
> >> Not really, until the systems needing them are in use.
> > Yeah I suspect it might take a very long time, and I guess it's very hard to definitely
> > say that some piece of hardware is not used by **anyone**
> >
> >>>> Also it can be argued that ACPI_PDC_C_C2C3_FFH and ACPI_PDC_C_C1_FFH
> >>>> should be set by the arch code too.
> >>> That makes sense, but technically is also a workaround, since we're basically
> >>> checking for some specific DMI's and then we disable mwait for them.
> >> But boot_option_idle_override is set by the arch code, isn't it?
> >>
> >> So the arch code may as well do the quirk in accordance with it.
> > I think so, I'll modify the code to move setting those bits to the arch part
>
> I looked into that, and I'm still not sure whether setting those constants in arch
> specific code is a good idea. Basically OSC and PDC are supported on two architectures
> ia64 and x86, so that would introduce unnecessary code duplication, as this mechanic
> is present regardless of an architecture, and in this particular case boot_option_idle_override
> is set by acpi_processor.c function set_no_mwait().

Which is x86-specific AFAICS.

> One could argue theoretically that system defined in processor_dmi_table[] is an x86 so there
> is no need to add any logic to ia64,

Good observation!

> but to me this is confusing.

Why is it so?

> If we have a workaround in the acpi_processor, maybe entire workaround should stay there
> instead of dragging innocent arch code into it.

And maybe it would be better to move it to arch code as a whole.