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

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

[Mathias Nyman]

Ports with  _UPC (USB Port Capability) ACPI objects stating they are
"not connectable" are not wired to any connector or internal device.
They only exist inside the host controller.

These ports may not have an 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(-)

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;
 }
-- 
2.25.1

[PATCH 2/2] usb: port: Don't try to peer unused USB ports based on location

[Mathias Nyman]

Unused USB ports may have bogus location data in ACPI PLD tables.
This causes port peering failures as these unused USB2 and USB3 ports
location may match.

This is seen on DELL systems where all unused ports return zeroed
location data.

Don't try to peer or match ports that have connect type set to
USB_PORT_NOT_USED.

Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
Cc: stable@vger.kernel.org
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/core/port.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index c628c1abc907..4d63496f98b6 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -573,7 +573,7 @@ static int match_location(struct usb_device *peer_hdev, void *p)
 	struct usb_hub *peer_hub = usb_hub_to_struct_hub(peer_hdev);
 	struct usb_device *hdev = to_usb_device(port_dev->dev.parent->parent);
 
-	if (!peer_hub)
+	if (!peer_hub || port_dev->connect_type == USB_PORT_NOT_USED)
 		return 0;
 
 	hcd = bus_to_hcd(hdev->bus);
@@ -584,7 +584,8 @@ static int match_location(struct usb_device *peer_hdev, void *p)
 
 	for (port1 = 1; port1 <= peer_hdev->maxchild; port1++) {
 		peer = peer_hub->ports[port1 - 1];
-		if (peer && peer->location == port_dev->location) {
+		if (peer && peer->connect_type != USB_PORT_NOT_USED &&
+		    peer->location == port_dev->location) {
 			link_peers_report(port_dev, peer);
 			return 1; /* done */
 		}
-- 
2.25.1

Re: [PATCH 2/2] usb: port: Don't try to peer unused USB ports based on location

[Greg KH]

On Thu, Feb 22, 2024 at 03:38:19PM +0200, Mathias Nyman wrote:
> Unused USB ports may have bogus location data in ACPI PLD tables.
> This causes port peering failures as these unused USB2 and USB3 ports
> location may match.
> 
> This is seen on DELL systems where all unused ports return zeroed
> location data.
> 
> Don't try to peer or match ports that have connect type set to
> USB_PORT_NOT_USED.
> 
> Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Cc: stable@vger.kernel.org
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>

What commit does this fix?  "all" of them?

thanks,

greg k-h

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

[Greg KH]

On Thu, Feb 22, 2024 at 03:38:18PM +0200, Mathias Nyman wrote:
> Ports with  _UPC (USB Port Capability) ACPI objects stating they are
> "not connectable" are not wired to any connector or internal device.
> They only exist inside the host controller.
> 
> These ports may not have an 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>

Does patch 2/2 need this?  If so, why isn't it marked for stable?

thanks,

greg k-h

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

[Mathias Nyman]

On 22.2.2024 16.06, Greg KH wrote:
> On Thu, Feb 22, 2024 at 03:38:18PM +0200, Mathias Nyman wrote:
>> Ports with  _UPC (USB Port Capability) ACPI objects stating they are
>> "not connectable" are not wired to any connector or internal device.
>> They only exist inside the host controller.
>>
>> These ports may not have an 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>
> 
> Does patch 2/2 need this?  If so, why isn't it marked for stable?

2/2 alone fixes the real world port peering problem seen.

This is something I stumbled upon while debugging that issue.
This patch just makes sure we don't skip marking some unused ports as
unused due to how we parse ACPI tables.

Thanks
Mathias

  

Re: [PATCH 2/2] usb: port: Don't try to peer unused USB ports based on location

[Mathias Nyman]

On 22.2.2024 16.06, Greg KH wrote:
> On Thu, Feb 22, 2024 at 03:38:19PM +0200, Mathias Nyman wrote:
>> Unused USB ports may have bogus location data in ACPI PLD tables.
>> This causes port peering failures as these unused USB2 and USB3 ports
>> location may match.
>>
>> This is seen on DELL systems where all unused ports return zeroed
>> location data.
>>
>> Don't try to peer or match ports that have connect type set to
>> USB_PORT_NOT_USED.
>>
>> Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> 
> What commit does this fix?  "all" of them?

Right, git blame shows the code this fixes was added 10 years ago in 3.16

Fixes: 3bfd659baec8 ("usb: find internal hub tier mismatch via acpi")
Cc: stable@vger.kernel.org # v3.16+

Thanks
Mathias

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

[Greg KH]

On Thu, Feb 22, 2024 at 04:46:16PM +0200, Mathias Nyman wrote:
> On 22.2.2024 16.06, Greg KH wrote:
> > On Thu, Feb 22, 2024 at 03:38:18PM +0200, Mathias Nyman wrote:
> > > Ports with  _UPC (USB Port Capability) ACPI objects stating they are
> > > "not connectable" are not wired to any connector or internal device.
> > > They only exist inside the host controller.
> > > 
> > > These ports may not have an 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>
> > 
> > Does patch 2/2 need this?  If so, why isn't it marked for stable?
> 
> 2/2 alone fixes the real world port peering problem seen.
> 
> This is something I stumbled upon while debugging that issue.
> This patch just makes sure we don't skip marking some unused ports as
> unused due to how we parse ACPI tables.

Ok, so should patch 1/2 go to usb-next and patch 2/2 go to usb-linus?

confused,

greg k-h

Re: [PATCH 2/2] usb: port: Don't try to peer unused USB ports based on location

[Paul Menzel]

Dear Mathias,


Thank you for your patches fixing the problem.

Am 22.02.24 um 14:38 schrieb Mathias Nyman:
> Unused USB ports may have bogus location data in ACPI PLD tables.
> This causes port peering failures as these unused USB2 and USB3 ports
> location may match.

I comment here, although it should probably be in another branch of this 
thread.

If it is a firmware issue, this check should be added to FirmWare Test 
Suite (fwts) [1] too (I can report it there), and maybe some debug log 
should report this firmware error too.

> This is seen on DELL systems where all unused ports return zeroed
> location data.

As noted in the post scriptum in [2], much more systems seem to be affected.

> Don't try to peer or match ports that have connect type set to
> USB_PORT_NOT_USED.

When grepping the git history, pasting the warning message would help 
me. Maybe:

This fixes the warning below on the affected systems:

     usb: port power management may be unreliable

If you want to add add the Linux Kernel Bugzilla URLs:

Link: https://bugzilla.kernel.org/show_bug.cgi?id=218465
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218486

I wasn’t able to test the other two systems yet, but maybe it is obvious 
from the ACPI tables/ASL code:

Link: https://bugzilla.kernel.org/show_bug.cgi?id=218487 (Dell OptiPlex 
5055)
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218490 (Dell PowerEdge 
T440)

> Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Cc: stable@vger.kernel.org
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>   drivers/usb/core/port.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index c628c1abc907..4d63496f98b6 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -573,7 +573,7 @@ static int match_location(struct usb_device *peer_hdev, void *p)
>   	struct usb_hub *peer_hub = usb_hub_to_struct_hub(peer_hdev);
>   	struct usb_device *hdev = to_usb_device(port_dev->dev.parent->parent);
>   
> -	if (!peer_hub)
> +	if (!peer_hub || port_dev->connect_type == USB_PORT_NOT_USED)
>   		return 0;
>   
>   	hcd = bus_to_hcd(hdev->bus);
> @@ -584,7 +584,8 @@ static int match_location(struct usb_device *peer_hdev, void *p)
>   
>   	for (port1 = 1; port1 <= peer_hdev->maxchild; port1++) {
>   		peer = peer_hub->ports[port1 - 1];
> -		if (peer && peer->location == port_dev->location) {
> +		if (peer && peer->connect_type != USB_PORT_NOT_USED &&
> +		    peer->location == port_dev->location) {
>   			link_peers_report(port_dev, peer);
>   			return 1; /* done */
>   		}


Thank you again and kind regards,

Paul


[1]: https://wiki.ubuntu.com/FirmwareTestSuite/
[2]: 
https://lore.kernel.org/linux-usb/5406d361-f5b7-4309-b0e6-8c94408f7d75@molgen.mpg.de/

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

[Mathias Nyman]

On 22.2.2024 17.50, Greg KH wrote:
> On Thu, Feb 22, 2024 at 04:46:16PM +0200, Mathias Nyman wrote:
>> On 22.2.2024 16.06, Greg KH wrote:
>>> On Thu, Feb 22, 2024 at 03:38:18PM +0200, Mathias Nyman wrote:
>>>> Ports with  _UPC (USB Port Capability) ACPI objects stating they are
>>>> "not connectable" are not wired to any connector or internal device.
>>>> They only exist inside the host controller.
>>>>
>>>> These ports may not have an 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>
>>>
>>> Does patch 2/2 need this?  If so, why isn't it marked for stable?
>>
>> 2/2 alone fixes the real world port peering problem seen.
>>
>> This is something I stumbled upon while debugging that issue.
>> This patch just makes sure we don't skip marking some unused ports as
>> unused due to how we parse ACPI tables.
> 
> Ok, so should patch 1/2 go to usb-next and patch 2/2 go to usb-linus?

That works as well.

Thanks
Mathias

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

[Mathias Nyman]

>>
>> Ok, so should patch 1/2 go to usb-next and patch 2/2 go to usb-linus?
> 
> That works as well.

I'll tune up those tags, split the patches, and resend

-Mathias