diff mbox series

[v2] usb: acpi: fix boot hang due to early incorrect 'tunneled' USB3 device links

Message ID 20241024131355.3836538-1-mathias.nyman@linux.intel.com (mailing list archive)
State Accepted
Commit 623dae3e7084a9504e6dc4cf0cb83f305f413b4d
Headers show
Series [v2] usb: acpi: fix boot hang due to early incorrect 'tunneled' USB3 device links | expand

Commit Message

Mathias Nyman Oct. 24, 2024, 1:13 p.m. UTC
Fix a boot hang issue triggered when a USB3 device is incorrectly assumed
to be tunneled over USB4, thus attempting to create a device link between
the USB3 "consumer" device and the USB4 "supplier" Host Interface before
the USB4 side is properly bound to a driver.

This could happen if xhci isn't capable of detecting tunneled devices,
but ACPI tables contain all info needed to assume device is tunneled.
i.e. udev->tunnel_mode == USB_LINK_UNKNOWN.

It turns out that even for actual tunneled USB3 devices it can't be
assumed that the thunderbolt driver providing the tunnel is loaded
before the tunneled USB3 device is created.
The tunnel can be created by BIOS and remain in use by thunderbolt/USB4
host driver once it loads.

Solve this by making the device link "stateless", which doesn't create
a driver presence order dependency between the supplier and consumer
drivers.
It still guarantees correct suspend/resume and shutdown ordering.

cc: Mario Limonciello <mario.limonciello@amd.com>
Fixes: f1bfb4a6fed6 ("usb: acpi: add device link between tunneled USB3 device and USB4 Host Interface")
Tested-by: Harry Wentland <harry.wentland@amd.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 v2: Use stateless device link instead of checking if driver is bound

 drivers/usb/core/usb-acpi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Mario Limonciello Oct. 24, 2024, 1:24 p.m. UTC | #1
On 10/24/2024 08:13, Mathias Nyman wrote:
> Fix a boot hang issue triggered when a USB3 device is incorrectly assumed
> to be tunneled over USB4, thus attempting to create a device link between
> the USB3 "consumer" device and the USB4 "supplier" Host Interface before
> the USB4 side is properly bound to a driver.
> 
> This could happen if xhci isn't capable of detecting tunneled devices,
> but ACPI tables contain all info needed to assume device is tunneled.
> i.e. udev->tunnel_mode == USB_LINK_UNKNOWN.
> 
> It turns out that even for actual tunneled USB3 devices it can't be
> assumed that the thunderbolt driver providing the tunnel is loaded
> before the tunneled USB3 device is created.
> The tunnel can be created by BIOS and remain in use by thunderbolt/USB4
> host driver once it loads.
> 
> Solve this by making the device link "stateless", which doesn't create
> a driver presence order dependency between the supplier and consumer
> drivers.
> It still guarantees correct suspend/resume and shutdown ordering.
> 
> cc: Mario Limonciello <mario.limonciello@amd.com>
> Fixes: f1bfb4a6fed6 ("usb: acpi: add device link between tunneled USB3 device and USB4 Host Interface")
> Tested-by: Harry Wentland <harry.wentland@amd.com>
Tested-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>   v2: Use stateless device link instead of checking if driver is bound
> 
>   drivers/usb/core/usb-acpi.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
> index 21585ed89ef8..03c22114214b 100644
> --- a/drivers/usb/core/usb-acpi.c
> +++ b/drivers/usb/core/usb-acpi.c
> @@ -170,11 +170,11 @@ static int usb_acpi_add_usb4_devlink(struct usb_device *udev)
>   	struct fwnode_handle *nhi_fwnode __free(fwnode_handle) =
>   		fwnode_find_reference(dev_fwnode(&port_dev->dev), "usb4-host-interface", 0);
>   
> -	if (IS_ERR(nhi_fwnode))
> +	if (IS_ERR(nhi_fwnode) || !nhi_fwnode->dev)
>   		return 0;
>   
>   	link = device_link_add(&port_dev->child->dev, nhi_fwnode->dev,
> -			       DL_FLAG_AUTOREMOVE_CONSUMER |
> +			       DL_FLAG_STATELESS |
>   			       DL_FLAG_RPM_ACTIVE |
>   			       DL_FLAG_PM_RUNTIME);
>   	if (!link) {
Mika Westerberg Oct. 24, 2024, 2:56 p.m. UTC | #2
On Thu, Oct 24, 2024 at 04:13:55PM +0300, Mathias Nyman wrote:
> Fix a boot hang issue triggered when a USB3 device is incorrectly assumed
> to be tunneled over USB4, thus attempting to create a device link between
> the USB3 "consumer" device and the USB4 "supplier" Host Interface before
> the USB4 side is properly bound to a driver.
> 
> This could happen if xhci isn't capable of detecting tunneled devices,
> but ACPI tables contain all info needed to assume device is tunneled.
> i.e. udev->tunnel_mode == USB_LINK_UNKNOWN.
> 
> It turns out that even for actual tunneled USB3 devices it can't be
> assumed that the thunderbolt driver providing the tunnel is loaded
> before the tunneled USB3 device is created.
> The tunnel can be created by BIOS and remain in use by thunderbolt/USB4
> host driver once it loads.
> 
> Solve this by making the device link "stateless", which doesn't create
> a driver presence order dependency between the supplier and consumer
> drivers.
> It still guarantees correct suspend/resume and shutdown ordering.
> 
> cc: Mario Limonciello <mario.limonciello@amd.com>
> Fixes: f1bfb4a6fed6 ("usb: acpi: add device link between tunneled USB3 device and USB4 Host Interface")
> Tested-by: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
diff mbox series

Patch

diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index 21585ed89ef8..03c22114214b 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -170,11 +170,11 @@  static int usb_acpi_add_usb4_devlink(struct usb_device *udev)
 	struct fwnode_handle *nhi_fwnode __free(fwnode_handle) =
 		fwnode_find_reference(dev_fwnode(&port_dev->dev), "usb4-host-interface", 0);
 
-	if (IS_ERR(nhi_fwnode))
+	if (IS_ERR(nhi_fwnode) || !nhi_fwnode->dev)
 		return 0;
 
 	link = device_link_add(&port_dev->child->dev, nhi_fwnode->dev,
-			       DL_FLAG_AUTOREMOVE_CONSUMER |
+			       DL_FLAG_STATELESS |
 			       DL_FLAG_RPM_ACTIVE |
 			       DL_FLAG_PM_RUNTIME);
 	if (!link) {