| Message ID | 20231114120611.128054-1-mika.westerberg@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | 6d392d8daa7514a431678521c2af8be10fc31bc1 |
| Headers | show |
| Series | ACPI: Run USB4 _OSC() first with query bit set | expand |
On Tue, Nov 14, 2023 at 1:06 PM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > The platform can deny certain tunneling from the OS and it does that by > clearing the control bits it does not want the OS to get and returning > with OSC_CAPABILITIES_MASK_ERROR bit set. Currently we do not handle > this properly so if this happens, for example when the platform denies > PCIe tunneling, we just fail the whole negotiation and revert back to > what the Thunderbolt driver is doing to figure out whether the > controller is running firmware connection manager or not. However, we > should honor what the platform returns. > > For this reason run the USB4 _OSC() first with query bit set, and then > use the returned control double word (that may contain some of the bits > cleared by the platform) and run it second time with query bit clear. > > While there, remove an extra space from the assignment of the control > double word. > > Reported-by: NaamaX Shachar <naamax.shachar@intel.com> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > drivers/acpi/bus.c | 32 +++++++++++++++++++++++++++++--- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c > index 72e64c0718c9..569bd15f211b 100644 > --- a/drivers/acpi/bus.c > +++ b/drivers/acpi/bus.c > @@ -408,7 +408,7 @@ static void acpi_bus_decode_usb_osc(const char *msg, u32 bits) > static u8 sb_usb_uuid_str[] = "23A0D13A-26AB-486C-9C5F-0FFA525A575A"; > static void acpi_bus_osc_negotiate_usb_control(void) > { > - u32 capbuf[3]; > + u32 capbuf[3], *capbuf_ret; > struct acpi_osc_context context = { > .uuid_str = sb_usb_uuid_str, > .rev = 1, > @@ -428,7 +428,12 @@ static void acpi_bus_osc_negotiate_usb_control(void) > control = OSC_USB_USB3_TUNNELING | OSC_USB_DP_TUNNELING | > OSC_USB_PCIE_TUNNELING | OSC_USB_XDOMAIN; > > - capbuf[OSC_QUERY_DWORD] = 0; > + /* > + * Run _OSC first with query bit set, trying to get control over > + * all tunneling. The platform can then clear out bits in the > + * control dword that it does not want to grant to the OS. > + */ > + capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE; > capbuf[OSC_SUPPORT_DWORD] = 0; > capbuf[OSC_CONTROL_DWORD] = control; > > @@ -441,8 +446,29 @@ static void acpi_bus_osc_negotiate_usb_control(void) > goto out_free; > } > > + /* > + * Run _OSC again now with query bit clear and the control dword > + * matching what the platform granted (which may not have all > + * the control bits set). > + */ > + capbuf_ret = context.ret.pointer; > + > + capbuf[OSC_QUERY_DWORD] = 0; > + capbuf[OSC_CONTROL_DWORD] = capbuf_ret[OSC_CONTROL_DWORD]; > + > + kfree(context.ret.pointer); > + > + status = acpi_run_osc(handle, &context); > + if (ACPI_FAILURE(status)) > + return; > + > + if (context.ret.length != sizeof(capbuf)) { > + pr_info("USB4 _OSC: returned invalid length buffer\n"); > + goto out_free; > + } > + > osc_sb_native_usb4_control = > - control & acpi_osc_ctx_get_pci_control(&context); > + control & acpi_osc_ctx_get_pci_control(&context); > > acpi_bus_decode_usb_osc("USB4 _OSC: OS supports", control); > acpi_bus_decode_usb_osc("USB4 _OSC: OS controls", > -- Applied as 6.8 material, but if you want me to push this for 6.7-rc, please let me know (in which case it would be nice to have a Fixes: tag to put on it). Thanks!
On Mon, Nov 20, 2023 at 05:54:28PM +0100, Rafael J. Wysocki wrote: > On Tue, Nov 14, 2023 at 1:06 PM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > > > The platform can deny certain tunneling from the OS and it does that by > > clearing the control bits it does not want the OS to get and returning > > with OSC_CAPABILITIES_MASK_ERROR bit set. Currently we do not handle > > this properly so if this happens, for example when the platform denies > > PCIe tunneling, we just fail the whole negotiation and revert back to > > what the Thunderbolt driver is doing to figure out whether the > > controller is running firmware connection manager or not. However, we > > should honor what the platform returns. > > > > For this reason run the USB4 _OSC() first with query bit set, and then > > use the returned control double word (that may contain some of the bits > > cleared by the platform) and run it second time with query bit clear. > > > > While there, remove an extra space from the assignment of the control > > double word. > > > > Reported-by: NaamaX Shachar <naamax.shachar@intel.com> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > --- > > drivers/acpi/bus.c | 32 +++++++++++++++++++++++++++++--- > > 1 file changed, 29 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c > > index 72e64c0718c9..569bd15f211b 100644 > > --- a/drivers/acpi/bus.c > > +++ b/drivers/acpi/bus.c > > @@ -408,7 +408,7 @@ static void acpi_bus_decode_usb_osc(const char *msg, u32 bits) > > static u8 sb_usb_uuid_str[] = "23A0D13A-26AB-486C-9C5F-0FFA525A575A"; > > static void acpi_bus_osc_negotiate_usb_control(void) > > { > > - u32 capbuf[3]; > > + u32 capbuf[3], *capbuf_ret; > > struct acpi_osc_context context = { > > .uuid_str = sb_usb_uuid_str, > > .rev = 1, > > @@ -428,7 +428,12 @@ static void acpi_bus_osc_negotiate_usb_control(void) > > control = OSC_USB_USB3_TUNNELING | OSC_USB_DP_TUNNELING | > > OSC_USB_PCIE_TUNNELING | OSC_USB_XDOMAIN; > > > > - capbuf[OSC_QUERY_DWORD] = 0; > > + /* > > + * Run _OSC first with query bit set, trying to get control over > > + * all tunneling. The platform can then clear out bits in the > > + * control dword that it does not want to grant to the OS. > > + */ > > + capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE; > > capbuf[OSC_SUPPORT_DWORD] = 0; > > capbuf[OSC_CONTROL_DWORD] = control; > > > > @@ -441,8 +446,29 @@ static void acpi_bus_osc_negotiate_usb_control(void) > > goto out_free; > > } > > > > + /* > > + * Run _OSC again now with query bit clear and the control dword > > + * matching what the platform granted (which may not have all > > + * the control bits set). > > + */ > > + capbuf_ret = context.ret.pointer; > > + > > + capbuf[OSC_QUERY_DWORD] = 0; > > + capbuf[OSC_CONTROL_DWORD] = capbuf_ret[OSC_CONTROL_DWORD]; > > + > > + kfree(context.ret.pointer); > > + > > + status = acpi_run_osc(handle, &context); > > + if (ACPI_FAILURE(status)) > > + return; > > + > > + if (context.ret.length != sizeof(capbuf)) { > > + pr_info("USB4 _OSC: returned invalid length buffer\n"); > > + goto out_free; > > + } > > + > > osc_sb_native_usb4_control = > > - control & acpi_osc_ctx_get_pci_control(&context); > > + control & acpi_osc_ctx_get_pci_control(&context); > > > > acpi_bus_decode_usb_osc("USB4 _OSC: OS supports", control); > > acpi_bus_decode_usb_osc("USB4 _OSC: OS controls", > > -- > > Applied as 6.8 material, but if you want me to push this for 6.7-rc, > please let me know (in which case it would be nice to have a Fixes: > tag to put on it). Thanks! I think v6.8 is fine.
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index 72e64c0718c9..569bd15f211b 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -408,7 +408,7 @@ static void acpi_bus_decode_usb_osc(const char *msg, u32 bits) static u8 sb_usb_uuid_str[] = "23A0D13A-26AB-486C-9C5F-0FFA525A575A"; static void acpi_bus_osc_negotiate_usb_control(void) { - u32 capbuf[3]; + u32 capbuf[3], *capbuf_ret; struct acpi_osc_context context = { .uuid_str = sb_usb_uuid_str, .rev = 1, @@ -428,7 +428,12 @@ static void acpi_bus_osc_negotiate_usb_control(void) control = OSC_USB_USB3_TUNNELING | OSC_USB_DP_TUNNELING | OSC_USB_PCIE_TUNNELING | OSC_USB_XDOMAIN; - capbuf[OSC_QUERY_DWORD] = 0; + /* + * Run _OSC first with query bit set, trying to get control over + * all tunneling. The platform can then clear out bits in the + * control dword that it does not want to grant to the OS. + */ + capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE; capbuf[OSC_SUPPORT_DWORD] = 0; capbuf[OSC_CONTROL_DWORD] = control; @@ -441,8 +446,29 @@ static void acpi_bus_osc_negotiate_usb_control(void) goto out_free; } + /* + * Run _OSC again now with query bit clear and the control dword + * matching what the platform granted (which may not have all + * the control bits set). + */ + capbuf_ret = context.ret.pointer; + + capbuf[OSC_QUERY_DWORD] = 0; + capbuf[OSC_CONTROL_DWORD] = capbuf_ret[OSC_CONTROL_DWORD]; + + kfree(context.ret.pointer); + + status = acpi_run_osc(handle, &context); + if (ACPI_FAILURE(status)) + return; + + if (context.ret.length != sizeof(capbuf)) { + pr_info("USB4 _OSC: returned invalid length buffer\n"); + goto out_free; + } + osc_sb_native_usb4_control = - control & acpi_osc_ctx_get_pci_control(&context); + control & acpi_osc_ctx_get_pci_control(&context); acpi_bus_decode_usb_osc("USB4 _OSC: OS supports", control); acpi_bus_decode_usb_osc("USB4 _OSC: OS controls",
The platform can deny certain tunneling from the OS and it does that by clearing the control bits it does not want the OS to get and returning with OSC_CAPABILITIES_MASK_ERROR bit set. Currently we do not handle this properly so if this happens, for example when the platform denies PCIe tunneling, we just fail the whole negotiation and revert back to what the Thunderbolt driver is doing to figure out whether the controller is running firmware connection manager or not. However, we should honor what the platform returns. For this reason run the USB4 _OSC() first with query bit set, and then use the returned control double word (that may contain some of the bits cleared by the platform) and run it second time with query bit clear. While there, remove an extra space from the assignment of the control double word. Reported-by: NaamaX Shachar <naamax.shachar@intel.com> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/acpi/bus.c | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)