Re: [PATCH v6] ACPI: bus: For platform OSC negotiate capabilities

From: "Rafael J. Wysocki" <rafael@kernel.org>
To: "Limonciello, Mario" <Mario.Limonciello@amd.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	"Hou, Xiaomeng (Matthew)" <Xiaomeng.Hou@amd.com>,
	"Liu, Aaron" <Aaron.Liu@amd.com>,
	"Huang, Ray" <Ray.Huang@amd.com>,
	Hans de Goede <hdegoede@redhat.com>
Subject: Re: [PATCH v6] ACPI: bus: For platform OSC negotiate capabilities
Date: Wed, 16 Mar 2022 11:20:00 +0100	[thread overview]
Message-ID: <CAJZ5v0iQOG0-6SOfDwVm2QFOm1C0Nmtb=+2PHrMEt4ycCzOLtw@mail.gmail.com> (raw)
In-Reply-To: <BL1PR12MB515702B2DB9FED265B7B41BAE2109@BL1PR12MB5157.namprd12.prod.outlook.com>

On Tue, Mar 15, 2022 at 9:32 PM Limonciello, Mario
<Mario.Limonciello@amd.com> wrote:
>
> [Public]
>
> > > > > That latter piece appears to be the bug in question here and IMO
> > > > > fixing it doesn't even require calling acpi_run_osc() with the query
> > > > > flag set for multiple times.
> > > >
> > > > I think just taking the results will re-introduce the CPC bug though
> > > > won't it?  So how to avoid it but also to take the results?
> > >
> > > I think that the OS should not ask for the control of the CPPC bits if
> > > they are masked by the firmware and it should avoid invoking _CPC
> > > then.
> > >
> > > Otherwise we risk breaking legitimate cases in which the firmware
> > > actually doesn't want the OS to control those bits.
> >
> > I'm basically talking about reverting commit 159d8c274fd9, as the part
> > of the _OSC definition in the spec it is based on appears to be bogus
> > (that will be addressed separately via the ACPI spec process), and
> > applying the attached change on top of that.
> >
> > If this looks good to you, I'll take care of it.
>
> Yes, that looks great and I checked with 159d8c274fd9 reverted and that applying
> the problem does not exist.  I do think it has the possibility to cause CPPC to not
> be enabled outside of Intel though since HWP is only set there so I would suggest
> this other change on top of it:

Good point.

I'll make it x86-specific for the time being and that can be changed later.

> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 4df749b82568..e61dbd7f7108 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -314,10 +314,8 @@ static void acpi_bus_osc_negotiate_platform_control(void)
>  #endif
>  #ifdef CONFIG_X86
>         capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_GENERIC_INITIATOR_SUPPORT;
> -       if (boot_cpu_has(X86_FEATURE_HWP)) {
> -               capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_SUPPORT;
> -               capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPCV2_SUPPORT;
> -       }
> +       capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_SUPPORT;
> +       capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPCV2_SUPPORT;
>  #endif
>
>         if (IS_ENABLED(CONFIG_SCHED_MC_PRIO))