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

Commit Message

Mathias Nyman Feb. 22, 2024, 1:38 p.m. UTC

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 <[e-mail]>
Cc: [e-mail]
Signed-off-by: Mathias Nyman <[e-mail]>
---
 drivers/usb/core/port.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Greg KH Feb. 22, 2024, 2:06 p.m. UTC | #1

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 <[e-mail]>
> Cc: [e-mail]
> Signed-off-by: Mathias Nyman <[e-mail]>

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

thanks,

greg k-h

Mathias Nyman Feb. 22, 2024, 3:03 p.m. UTC | #2

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 <[e-mail]>
>> Cc: [e-mail]
>> Signed-off-by: Mathias Nyman <[e-mail]>
> 
> 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: [e-mail] # v3.16+

Thanks
Mathias

Paul Menzel Feb. 22, 2024, 3:51 p.m. UTC | #3

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 <[e-mail]>
> Cc: [e-mail]
> Signed-off-by: Mathias Nyman <[e-mail]>
> ---
>   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/[e-mail]/

Patch

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 */
 		}