Re: [PATCH 4/6] ACPI: Execute platform _OSC also with query bit clear

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> > >



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux