Re: [PATCH] ACPI: Run USB4 _OSC() first with query bit set

[Mika Westerberg <mika.westerberg@linux.intel.com>]

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.