| Message ID | 20210126155723.9388-5-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| 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 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. > 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 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. > > > 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 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 > > >
> I would put that information into the changelog. Thanks, @Mika Westerberg can you collapse that in when you re-spin the series? > > 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." > > > > > > > > > > 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,