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 v5] ACPI: bus: For platform OSC negotiate capabilities
Date: Thu, 10 Mar 2022 20:13:54 +0100 [thread overview]
Message-ID: <CAJZ5v0j2GmwKG+pP5KPPN5j46LcqqfznsBw1o0tkLKRNJJVj9w@mail.gmail.com> (raw)
In-Reply-To: <BL1PR12MB5157A25B586DAE3E5B58F696E20B9@BL1PR12MB5157.namprd12.prod.outlook.com>
On Thu, Mar 10, 2022 at 8:09 PM Limonciello, Mario
<Mario.Limonciello@amd.com> wrote:
>
> [Public]
>
> > >
> > > According to the ACPI 6.4 spec:
> > > It is strongly recommended that the OS evaluate _OSC with the Query
> > > Support Flag set until _OSC returns the Capabilities Masked bit clear,
> > > to negotiate the set of features to be granted to the OS for native
> > > support; a platform may require a specific combination of features
> > > to be supported natively by an OS before granting native control
> > > of a given feature. After negotiation with the query flag set,
> > > the OS should evaluate without it so that any negotiated values
> > > can be made effective to hardware.
> > >
> > > Currently the code sends the exact same values in both executions of the
> > > _OSC and this leads to some problems on some AMD platforms in certain
> > > configurations.
> > >
> > > The following notable capabilities are set by OSPM when query is enabled:
> > > * OSC_SB_PR3_SUPPORT
> > > * OSC_SB_PCLPI_SUPPORT
> > > * OSC_SB_NATIVE_USB4_SUPPORT
> > >
> > > The first call to the platform OSC returns back a masked capabilities
> > > error because the firmware did not acknowledge OSC_SB_PCLPI_SUPPORT
> > but
> > > it acknolwedged the others.
> > >
> > > The second call to the platform _OSC without the query flag set then
> > > fails because the OSPM still sent the exact same values. This leads
> > > to not acknowledging OSC_SB_NATIVE_USB4_SUPPORT and later USB4
> > PCIe
> > > tunnels can't be authorized.
> > >
> > > This problem was first introduced by commit 159d8c274fd9 ("ACPI: Pass the
> > > same capabilities to the _OSC regardless of the query flag") which subtly
> > > adjusted the behavior from 719e1f5 ("ACPI: Execute platform _OSC also
> > > with query bit clear").
> > >
> > > The _OSC was called exactly 2 times:
> > > * Once to query and request from firmware
> > > * Once to commit to firmware without query
> > >
> > > To fix this problem, continue to call the _OSC until the firmware has
> > > indicated that capabilities are no longer masked or after an arbitrary
> > > number of negotiation attempts.
> > >
> > > Furthermore, to avoid the problem that commit 159d8c274fd9 ("ACPI: Pass
> > > the same capabilities to the _OSC regardless of the query flag")
> > > introduced, explicitly mark support for CPC and CPPCv2 even if they
> > > were masked by the series of query calls due to table loading order on
> > > some systems.
> > >
> > > Fixes: 159d8c274fd9 ("ACPI: Pass the same capabilities to the _OSC
> > regardless of the query flag")
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > > This series was accepted but showed a regression in another use of
> > acpi_run_osc
> > > so the series was dropped.
> > >
> > > Changes from v4->v5:
> > > * Move negotiation entirely into
> > acpi_bus_osc_negotiate_platform_control
> > > drivers/acpi/bus.c | 31 ++++++++++++++++++++++++++++---
> > > 1 file changed, 28 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > > index b96c54813886..86d88bd72c07 100644
> > > --- a/drivers/acpi/bus.c
> > > +++ b/drivers/acpi/bus.c
> > > @@ -294,6 +294,7 @@ static void
> > acpi_bus_osc_negotiate_platform_control(void)
> > > .cap.pointer = capbuf,
> > > };
> > > acpi_handle handle;
> > > + int i;
> > >
> > > capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
> > > capbuf[OSC_SUPPORT_DWORD] = OSC_SB_PR3_SUPPORT; /* _PR3 is
> > in use */
> > > @@ -329,10 +330,34 @@ static void
> > acpi_bus_osc_negotiate_platform_control(void)
> > > if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
> > > return;
> > >
> > > - if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
> > > - return;
> > > + /*
> > > + * Check if bits were masked, we need to negotiate
> > > + * prevent potential endless loop by limited number of
> > > + * negotiation cycles.
> > > + */
> > > + for (i = 0; i < 5; i++) {
> >
> > Why 5 iterations?
> >
> > Why cannot it work in analogy with the loop in acpi_pci_osc_control_set()?
>
> 5 was an arbitrary number selected just to guarantee that bad firmware couldn't
> deadlock the negotiation. It's admittedly unlikely, and if you would prefer I'll swap
> over to an endless loop design like acpi_pci_osc_control_set.
It need not be endless. Bits that are returned as clear can be
removed from the mask in the next iteration I think.
>
> >
> > > + bool retry = false;
> > > +
> > > + if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
> > > + return;
> > > + capbuf_ret = context.ret.pointer;
> > > + retry = capbuf_ret[OSC_SUPPORT_DWORD] !=
> > capbuf[OSC_SUPPORT_DWORD];
> > > + capbuf[OSC_SUPPORT_DWORD] =
> > capbuf_ret[OSC_SUPPORT_DWORD];
> > > + kfree(context.ret.pointer);
> > > + if (!retry)
> > > + break;
> > > + }
> > >
> > > - kfree(context.ret.pointer);
> > > + /*
> > > + * Avoid problems with BIOS dynamically loading tables by indicating
> > > + * support for CPPC even if it was masked.
> > > + */
> > > +#ifdef CONFIG_X86
> > > + if (boot_cpu_has(X86_FEATURE_HWP)) {
> > > + capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_SUPPORT;
> > > + capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPCV2_SUPPORT;
> > > + }
> > > +#endif
> > >
> > > /* Now run _OSC again with query flag clear */
> > > capbuf[OSC_QUERY_DWORD] = 0;
> > > --
> > > 2.34.1
> > >
prev parent reply other threads:[~2022-03-10 19:14 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-09 16:37 [PATCH v5] ACPI: bus: For platform OSC negotiate capabilities Mario Limonciello
2022-03-10 9:43 ` Mika Westerberg
2022-03-10 19:05 ` Rafael J. Wysocki
2022-03-10 19:08 ` Limonciello, Mario
2022-03-10 19:13 ` Rafael J. Wysocki [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJZ5v0j2GmwKG+pP5KPPN5j46LcqqfznsBw1o0tkLKRNJJVj9w@mail.gmail.com \
--to=rafael@kernel.org \
--cc=Aaron.Liu@amd.com \
--cc=Mario.Limonciello@amd.com \
--cc=Ray.Huang@amd.com \
--cc=Xiaomeng.Hou@amd.com \
--cc=hdegoede@redhat.com \
--cc=linux-acpi@vger.kernel.org \
--cc=mika.westerberg@linux.intel.com \
--cc=rjw@rjwysocki.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).