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@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! I think v6.8 is fine.
