On Tue, Nov 14, 2023 at 1:06 PM Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> ---
> 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!
