| Message ID | 20210126155723.9388-5-mika.westerberg@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | thunderbolt / ACPI: Add support for USB4 _OSC | expand |
On Tue, Jan 26, 2021 at 5:57 PM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > From: Mario Limonciello <mario.limonciello@dell.com> > > 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 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 Totally out of my depth here, but just noticed the typo (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@intel.com> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > ---
On Tue, Jan 26, 2021 at 6:37 PM Limonciello, Mario <Mario.Limonciello@dell.com> wrote: > > > On Tue, Jan 26, 2021 at 5:01 PM Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > > > > > > From: Mario Limonciello <mario.limonciello@dell.com> > > > > > > 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@intel.com> > > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > > > --- > > > 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 > > >
On Tue, Jan 26, 2021 at 10:43:32PM +0000, Limonciello, Mario wrote: > > I would put that information into the changelog. > > Thanks, @Mika Westerberg can you collapse that in when you re-spin the > series? Sure. > > > > 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? > > Basically the only difference is another line cloning OSC_CONTROL_DWORD from > capbuf_ret to capbuf? > > Yes, this actually sounds like it better adheres to the spec to me. > > Quoting spec: > " If the OS is granted control of a feature in the Control Field in one call to > _OSC, then it must preserve the set state of that bit (requesting that feature) > in all subsequent calls." However, the platform wide _OSC does not actually have this OSC_CONTROL_DWORD at all ;-) I think what we do in this patch is already equivalent to what the PCI _OSC is doing: 1. Query bit set _OSC 2. Take the returned OSC_SUPPORT_DWORD buffer and 3. Pass it to the _OSC with query bit clear. I may be missing something, though.
On Wed, Jan 27, 2021 at 1:49 PM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > On Tue, Jan 26, 2021 at 10:43:32PM +0000, Limonciello, Mario wrote: > > > I would put that information into the changelog. > > > > Thanks, @Mika Westerberg can you collapse that in when you re-spin the > > series? > > Sure. > > > > > > > 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? > > > > Basically the only difference is another line cloning OSC_CONTROL_DWORD from > > capbuf_ret to capbuf? > > > > Yes, this actually sounds like it better adheres to the spec to me. > > > > Quoting spec: > > " If the OS is granted control of a feature in the Control Field in one call to > > _OSC, then it must preserve the set state of that bit (requesting that feature) > > in all subsequent calls." > > However, the platform wide _OSC does not actually have this > OSC_CONTROL_DWORD at all ;-) Right. > I think what we do in this patch is already equivalent to what the PCI > _OSC is doing: > > 1. Query bit set _OSC > 2. Take the returned OSC_SUPPORT_DWORD buffer and > 3. Pass it to the _OSC with query bit clear. Yes, it is. Given the way the USB4 _OSC protocol is defined (which admittedly confused me somewhat), the code changes in this patch are fine by me. Thanks and sorry for the confusion.
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 + * both platform and OS supports. + */ + 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,