diff mbox series

[RFT,1/2] usb: usb-acpi: Set port connect type of not connectable ports correctly

Message ID 20240216141230.3924677-1-mathias.nyman@linux.intel.com
State Superseded
Headers show
Series [RFT,1/2] usb: usb-acpi: Set port connect type of not connectable ports correctly | expand

Commit Message

Mathias Nyman Feb. 16, 2024, 2:12 p.m. UTC 
Ports with  _UPC (USB Port Capability) ACPI objects stating they are
"not connectable" are not wired to any connector or internal device.
They only exists inside the host controller.

These ports may not have a ACPI _PLD (Physical Location of Device) object

Rework the code so that _UPC is read even if _PLD does not exist, and
make sure the port->connect_type is set to "USB_PORT_NOT_USED" instead of
"USB_PORT_CONNECT_TYPE_UNKNOWN".

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/core/usb-acpi.c | 46 ++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

Comments

Mathias Nyman Feb. 19, 2024, 7:44 a.m. UTC | #1 
On 16.2.2024 17.20, Alan Stern wrote:
> On Fri, Feb 16, 2024 at 04:12:30PM +0200, Mathias Nyman wrote:
>> Several unused ports may share the same bogus location data in ACPI
>> PLD tables. This causes port peering failures as these several unused
>> USB2 and USB3 ports suddenly match based on their location.
>>
>> Don't print the "usb: port power management may be unreliable" warning,
>> or block port power-off in case peering failed for two ports with
>> connect type set to USB_PORT_NOT_USED.
> 
> What if one port is marked USB_PORT_NOT_USED but its bogus location data
> causes the system to think it should peer with a port that _is_ in use
> (and maybe is already peered with a different port)?
> 

This could happen, but based on the DELL case it seems unlikely.

The bogus location entries are filled with zeroes, while the usable ports
start their location group position with a "1".

If neither is peered and one is marked USB_PORT_NOT_USED, and their location match,
then they will peer successfully. No change due to this patch.

If one port is already peered to some other port then peering will fail,
warning message is printed and usb_port_block_power_off = 1; is set.
No change here either.

Also two USB_PORT_NOT_USED ports with bogus matching location will
peer successfully as long as neither one is peered with some other port.
No change here either.

Could make sense to skip both peering and matching with ports that are
not used.

Thanks
Mathias
diff mbox series

Patch

diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index a34b22537d7c..f250dfc3b14d 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -142,12 +142,19 @@  int usb_acpi_set_power_state(struct usb_device *hdev, int index, bool enable)
 }
 EXPORT_SYMBOL_GPL(usb_acpi_set_power_state);
 
-static enum usb_port_connect_type usb_acpi_get_connect_type(acpi_handle handle,
-		struct acpi_pld_info *pld)
+/*
+ * Private to usb-acpi, all the core needs to know is that
+ * port_dev->location is non-zero when it has been set by the firmware.
+ */
+#define USB_ACPI_LOCATION_VALID (1 << 31)
+
+static void
+usb_acpi_get_connect_type(struct usb_port *port_dev, acpi_handle *handle)
 {
 	enum usb_port_connect_type connect_type = USB_PORT_CONNECT_TYPE_UNKNOWN;
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 	union acpi_object *upc = NULL;
+	struct acpi_pld_info *pld;
 	acpi_status status;
 
 	/*
@@ -158,6 +165,12 @@  static enum usb_port_connect_type usb_acpi_get_connect_type(acpi_handle handle,
 	 * a usb device is directly hard-wired to the port. If no visible and
 	 * no connectable, the port would be not used.
 	 */
+
+	status = acpi_get_physical_device_location(handle, &pld);
+	if (ACPI_SUCCESS(status) && pld)
+		port_dev->location = USB_ACPI_LOCATION_VALID |
+			pld->group_token << 8 | pld->group_position;
+
 	status = acpi_evaluate_object(handle, "_UPC", NULL, &buffer);
 	if (ACPI_FAILURE(status))
 		goto out;
@@ -166,25 +179,22 @@  static enum usb_port_connect_type usb_acpi_get_connect_type(acpi_handle handle,
 	if (!upc || (upc->type != ACPI_TYPE_PACKAGE) || upc->package.count != 4)
 		goto out;
 
+	/* UPC states port is connectable */
 	if (upc->package.elements[0].integer.value)
-		if (pld->user_visible)
+		if (!pld)
+			; /* keep connect_type as unknown */
+		else if (pld->user_visible)
 			connect_type = USB_PORT_CONNECT_TYPE_HOT_PLUG;
 		else
 			connect_type = USB_PORT_CONNECT_TYPE_HARD_WIRED;
-	else if (!pld->user_visible)
+	else
 		connect_type = USB_PORT_NOT_USED;
 out:
+	port_dev->connect_type = connect_type;
 	kfree(upc);
-	return connect_type;
+	ACPI_FREE(pld);
 }
 
-
-/*
- * Private to usb-acpi, all the core needs to know is that
- * port_dev->location is non-zero when it has been set by the firmware.
- */
-#define USB_ACPI_LOCATION_VALID (1 << 31)
-
 static struct acpi_device *
 usb_acpi_get_companion_for_port(struct usb_port *port_dev)
 {
@@ -222,22 +232,12 @@  static struct acpi_device *
 usb_acpi_find_companion_for_port(struct usb_port *port_dev)
 {
 	struct acpi_device *adev;
-	struct acpi_pld_info *pld;
-	acpi_handle *handle;
-	acpi_status status;
 
 	adev = usb_acpi_get_companion_for_port(port_dev);
 	if (!adev)
 		return NULL;
 
-	handle = adev->handle;
-	status = acpi_get_physical_device_location(handle, &pld);
-	if (ACPI_SUCCESS(status) && pld) {
-		port_dev->location = USB_ACPI_LOCATION_VALID
-			| pld->group_token << 8 | pld->group_position;
-		port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
-		ACPI_FREE(pld);
-	}
+	usb_acpi_get_connect_type(port_dev, adev->handle);
 
 	return adev;
 }