| Message ID | 20210129083241.72497-1-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | thunderbolt / ACPI: Add support for USB4 _OSC | expand |
Hi Rafael, I wonder if you are OK with this patch? Thanks! On Fri, Jan 29, 2021 at 11:32:39AM +0300, Mika Westerberg 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. Linux has not been doing this for the reasons that there > has not been anything to commit, until now. > > The ACPI 6.4 introduced _OSC for USB4 to allow the OS to negotiate > native control over USB4 tunneling. The platform might implement this so > that it only activates the software connection manager path when the OS > calls the _OSC with the query bit clear. Otherwise it may default to the > firmware connection manager, for instance. > > For this reason modify the _OSC support so that we first execute it with > query bit set, then use the returned value as base of the features we > want to control and run the _OSC again with query bit clear. This also > follows what Windows is doing. > > 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..a52cb28c40d8 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 clear and with the caps > + * 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 Wed, Feb 3, 2021 at 9:16 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > Hi Rafael, > > I wonder if you are OK with this patch? It looks good to me now, please feel free to add my ACK to it. Thanks! > > On Fri, Jan 29, 2021 at 11:32:39AM +0300, Mika Westerberg 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. Linux has not been doing this for the reasons that there > > has not been anything to commit, until now. > > > > The ACPI 6.4 introduced _OSC for USB4 to allow the OS to negotiate > > native control over USB4 tunneling. The platform might implement this so > > that it only activates the software connection manager path when the OS > > calls the _OSC with the query bit clear. Otherwise it may default to the > > firmware connection manager, for instance. > > > > For this reason modify the _OSC support so that we first execute it with > > query bit set, then use the returned value as base of the features we > > want to control and run the _OSC again with query bit clear. This also > > follows what Windows is doing. > > > > 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..a52cb28c40d8 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 clear and with the caps > > + * 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 Wed, Feb 03, 2021 at 02:56:25PM +0100, Rafael J. Wysocki wrote: > On Wed, Feb 3, 2021 at 9:16 AM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > > > Hi Rafael, > > > > I wonder if you are OK with this patch? > > It looks good to me now, please feel free to add my ACK to it. Thanks!
On Fri, Jan 29, 2021 at 11:32:36AM +0300, Mika Westerberg wrote: > Hi all, > > The just released ACPI 6.4 spec [1] adds a new _OSC method that is used to > negotiate OS support for native USB4 features such as PCIe tunneling. This > patch series adds Linux support for the new _OSC and modifies the > Thunderbolt/USB4 driver accordingly to enable/disable tunneling of > different protocols. > > There is an additional setting in the firmware connection manager that > allows the BIOS to disable PCIe tunneling, so we add support for this and > also make the software connection manager to switch to this "nopcie" > security level when the _OSC does not allow PCIe tunneling. > > This applies on top of thunderbolt.git/next. > > [1] https://uefi.org/sites/default/files/resources/ACPI_Spec_6_4_Jan22.pdf > > The previous version of the patch series can be found here: > > https://lore.kernel.org/linux-usb/20210126155723.9388-1-mika.westerberg@linux.intel.com/ > > Changes from v1: > > * Dropped patch 1/6. I already applied it to thunderbolt.git/fixes > * Added ACK from Yehezkel to TBT patches > * Updated changelog of patch 1/5 and fixed typos too > * Added Rafael's tag to patch 4/5. > > Mario Limonciello (1): > ACPI: Execute platform _OSC also with query bit clear > > Mika Westerberg (4): > thunderbolt: Add support for PCIe tunneling disabled (SL5) > thunderbolt: Allow disabling XDomain protocol > ACPI: Add support for native USB4 control _OSC > thunderbolt: Add support for native USB4 _OSC All applied to thunderbolt.git/next.
Hi Mika, There have some machines be found on openSUSE Tumbleweed that this patch causes that SSDT tables can not be dynamic loaded. The symptom is that dmesg shows '_CPC not found' because SSDT table did not dynamic load. [ 1.149107] ACPI BIOS Error (bug): Could not resolve symbol [\_PR.PR00._CPC], AE_NOT_FOUND (20210105/psargs-330) Looks that the firmware didn't response OSC_SB_CPCV2_SUPPORT after kernel changed to new behavior. The openSUSE bug is here: Bug 1185513 - ACPI BIOS Error after upgrade to 5.12.0-1-default https://bugzilla.suse.com/show_bug.cgi?id=1185513 Could you please help to give any suggestion? Thanks a lot! Joey Lee On Wed, Feb 03, 2021 at 10:14:15AM +0200, Mika Westerberg wrote: > Hi Rafael, > > I wonder if you are OK with this patch? > > Thanks! > > On Fri, Jan 29, 2021 at 11:32:39AM +0300, Mika Westerberg 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. Linux has not been doing this for the reasons that there > > has not been anything to commit, until now. > > > > The ACPI 6.4 introduced _OSC for USB4 to allow the OS to negotiate > > native control over USB4 tunneling. The platform might implement this so > > that it only activates the software connection manager path when the OS > > calls the _OSC with the query bit clear. Otherwise it may default to the > > firmware connection manager, for instance. > > > > For this reason modify the _OSC support so that we first execute it with > > query bit set, then use the returned value as base of the features we > > want to control and run the _OSC again with query bit clear. This also > > follows what Windows is doing. > > > > 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..a52cb28c40d8 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 clear and with the caps > > + * 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
Hi, On Mon, Jun 07, 2021 at 08:31:10PM +0800, joeyli wrote: > Hi Mika, > > There have some machines be found on openSUSE Tumbleweed that this patch > causes that SSDT tables can not be dynamic loaded. The symptom is that > dmesg shows '_CPC not found' because SSDT table did not dynamic load. > > [ 1.149107] ACPI BIOS Error (bug): Could not resolve symbol [\_PR.PR00._CPC], AE_NOT_FOUND (20210105/psargs-330) > > Looks that the firmware didn't response OSC_SB_CPCV2_SUPPORT after > kernel changed to new behavior. The openSUSE bug is here: > > Bug 1185513 - ACPI BIOS Error after upgrade to 5.12.0-1-default > https://bugzilla.suse.com/show_bug.cgi?id=1185513 > > Could you please help to give any suggestion? There is another one that Red Hat reported here: https://bugzilla.kernel.org/show_bug.cgi?id=213023 The Bugzilla entry also has a patch attached [1] from Hans, can you try it out and see if that fixes the issue? [1] https://bugzilla.kernel.org/attachment.cgi?id=297195 Thanks!
Hi Mika, On Mon, Jun 07, 2021 at 07:10:59PM +0300, Mika Westerberg wrote: > Hi, > > On Mon, Jun 07, 2021 at 08:31:10PM +0800, joeyli wrote: > > Hi Mika, > > > > There have some machines be found on openSUSE Tumbleweed that this patch > > causes that SSDT tables can not be dynamic loaded. The symptom is that > > dmesg shows '_CPC not found' because SSDT table did not dynamic load. > > > > [ 1.149107] ACPI BIOS Error (bug): Could not resolve symbol [\_PR.PR00._CPC], AE_NOT_FOUND (20210105/psargs-330) > > > > Looks that the firmware didn't response OSC_SB_CPCV2_SUPPORT after > > kernel changed to new behavior. The openSUSE bug is here: > > > > Bug 1185513 - ACPI BIOS Error after upgrade to 5.12.0-1-default > > https://bugzilla.suse.com/show_bug.cgi?id=1185513 > > > > Could you please help to give any suggestion? > > There is another one that Red Hat reported here: > > https://bugzilla.kernel.org/show_bug.cgi?id=213023 > > The Bugzilla entry also has a patch attached [1] from Hans, can you try it > out and see if that fixes the issue? > > [1] https://bugzilla.kernel.org/attachment.cgi?id=297195 > Thanks for your information! Hans's patch looks reasonable. I will backport to openSUSE TW and let bug reporter helps to test it. We will add comment on kernel.org bugzilla. Thanks a lot! Joey Lee