On Tue, Jan 26, 2021 at 6:37 PM Limonciello, Mario
<Mario.Limonciello@xxxxxxxx> wrote:
>
> > On Tue, Jan 26, 2021 at 5:01 PM Mika Westerberg
> > <mika.westerberg@xxxxxxxxxxxxxxx> wrote:
> > >
> > > From: Mario Limonciello <mario.limonciello@xxxxxxxx>
> > >
> > > The platform _OSC can change the hardware state when query bit is not
> > > set. According to ACPI spec it is recommended that the OS runs _OSC with
> > > query bit set until the platform does not mask any of the capabilities.
> > > Then it should run it with query bit clear in order to actually commit
> > > the changes. At the moment Linux only runs the _OSC with query bit set
> >
> > And that's because there was nothing it could ask to control using the
> > _SB scope _OSC.
> >
> > Today it is just reporting what features are supported by it.
> >
> > However, with the upcoming USB4 CM support it needs to ask for the
> > control of that feature and that's why the _SB scope _OSC support
> > needs to be extended. So it is not a fix for a bug or missing spec
> > coverage, which this part of the changelog kind of implies, it's just
> > enabling a new feature.
>
> Other operating systems behave as described in the ACPI spec long before USB4 CM
> support was added. Admittedly this is semantics of whether to call it
> a "bug", but specifically the lack of this in the existing Linux kernel code
> *can* actually cause you to get into a situation where you have no functional
> USB4. This will happen if you boot between two different kernels or potentially
> two different operating systems. This is due to how the selection of FW or SW
> CM is made. If this patch "alone" was brought further backward the older kernels
> FW CM mode would be activated in those situations.
I would put that information into the changelog.
Moreover, have you looked at acpi_pci_osc_control_set()?
What it does is analogous to what you are proposing, but a bit
different, and I would like to preserve consistency between _OSC use
cases.
So would it be possible to adjust the _SB _OSC evaluation flow to
follow the PCI _OSC one? That is, if any control bits are there, pass
them along with the last evaluation of _OSC with the query flag clear.
Or is the latter defective and if so then why?
>
> >
> > > and this is going to cause problems with the USB4 CM (Connection
> > > Manager) switch that is going to commit the switch only when the OS
> > > requests control over the feature.
> > >
> > > For this reason modify the _OSC support so that we first execute it with
> > > query bit set, then use the returned valu as base of the features we
> >
> > s/valu/value/
> >
> > > want to control and run the _OSC again with query bit clear.
> > >
> > > Also rename the function to better match what it does.
> > >
> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxxx>
> > > Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > >
> > > ---
> > > drivers/acpi/bus.c | 43 +++++++++++++++++++++++++++++++------------
> > > 1 file changed, 31 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > > index 1682f8b454a2..ca7c7b2bf56e 100644
> > > --- a/drivers/acpi/bus.c
> > > +++ b/drivers/acpi/bus.c
> > > @@ -282,9 +282,9 @@ bool osc_pc_lpi_support_confirmed;
> > > EXPORT_SYMBOL_GPL(osc_pc_lpi_support_confirmed);
> > >
> > > static u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
> > > -static void acpi_bus_osc_support(void)
> > > +static void acpi_bus_osc_negotiate_platform_control(void)
> > > {
> > > - u32 capbuf[2];
> > > + u32 capbuf[2], *capbuf_ret;
> > > struct acpi_osc_context context = {
> > > .uuid_str = sb_uuid_str,
> > > .rev = 1,
> > > @@ -321,17 +321,36 @@ static void acpi_bus_osc_support(void)
> > > capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_APEI_SUPPORT;
> > > if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
> > > return;
> > > - if (ACPI_SUCCESS(acpi_run_osc(handle, &context))) {
> > > - u32 *capbuf_ret = context.ret.pointer;
> > > - if (context.ret.length > OSC_SUPPORT_DWORD) {
> > > - osc_sb_apei_support_acked =
> > > - capbuf_ret[OSC_SUPPORT_DWORD] &
> > OSC_SB_APEI_SUPPORT;
> > > - osc_pc_lpi_support_confirmed =
> > > - capbuf_ret[OSC_SUPPORT_DWORD] &
> > OSC_SB_PCLPI_SUPPORT;
> > > - }
> > > +
> > > + if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
> > > + return;
> > > +
> > > + capbuf_ret = context.ret.pointer;
> > > + if (context.ret.length <= OSC_SUPPORT_DWORD) {
> > > kfree(context.ret.pointer);
> > > + return;
> > > }
> > > - /* do we need to check other returned cap? Sounds no */
> > > +
> > > + /*
> > > + * Now run _OSC again with query flag clean and with the caps
> >
> > s/clean/clear/
> >
> > > + * both platform and OS supports.
> >
> > s/both platform and OS supports/supported by both the OS and the platform/
> >
> > > + */
> > > + capbuf[OSC_QUERY_DWORD] = 0;
> > > + capbuf[OSC_SUPPORT_DWORD] = capbuf_ret[OSC_SUPPORT_DWORD];
> > > + kfree(context.ret.pointer);
> > > +
> > > + if (ACPI_FAILURE(acpi_run_osc(handle, &context)))
> > > + return;
> > > +
> > > + capbuf_ret = context.ret.pointer;
> > > + if (context.ret.length > OSC_SUPPORT_DWORD) {
> > > + osc_sb_apei_support_acked =
> > > + capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_APEI_SUPPORT;
> > > + osc_pc_lpi_support_confirmed =
> > > + capbuf_ret[OSC_SUPPORT_DWORD] &
> > OSC_SB_PCLPI_SUPPORT;
> > > + }
> > > +
> > > + kfree(context.ret.pointer);
> > > }
> > >
> > > /* ------------------------------------------------------------------------
> > --
> > > @@ -1168,7 +1187,7 @@ static int __init acpi_bus_init(void)
> > > * _OSC method may exist in module level code,
> > > * so it must be run after ACPI_FULL_INITIALIZATION
> > > */
> > > - acpi_bus_osc_support();
> > > + acpi_bus_osc_negotiate_platform_control();
> > >
> > > /*
> > > * _PDC control method may load dynamic SSDT tables,
> > > --
> > > 2.29.2
> > >
