Re: [PATCH] XhciDxe: Clean up UsbDevContext if USB slot initialization is failed


Wu, Hao A
 

Thanks Heng,

Inline comments below:

-----Original Message-----
From: Luo, Heng <heng.luo@...>
Sent: Monday, October 19, 2020 11:04 AM
To: Wu, Hao A <hao.a.wu@...>; devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>
Subject: RE: [PATCH] XhciDxe: Clean up UsbDevContext if USB slot
initialization is failed

Hi Hao,
Thank you for your review.
I think the slot Id is allocated by HW because the pointer EvtTrb point to
EvtRing->EventRingDequeue, EventRingDequeue is filled by XHCI
controller(correct me if I am wrong):

XhcDequeue = (UINT64)(LShiftU64((UINT64)High, 32) | Low);

PhyAddr = UsbHcGetPciAddrForHostAddr (Xhc->MemPool, Xhc-
EventRing.EventRingDequeue, sizeof (TRB_TEMPLATE));
if ((XhcDequeue & (~0x0F)) != (PhyAddr & (~0x0F))) {
//
// Some 3rd party XHCI external cards don't support single 64-bytes width
register access,
// So divide it to two 32-bytes width register access.
//
XhcWriteRuntimeReg (Xhc, XHC_ERDP_OFFSET, XHC_LOW_32BIT (PhyAddr)
| BIT3);
XhcWriteRuntimeReg (Xhc, XHC_ERDP_OFFSET + 4, XHC_HIGH_32BIT
(PhyAddr));
}

So it should be better to use XhcDisableSlotCmd to disable slot but not
directly clean up UsbDevContext, I will submit new patch if you agree.

I agree with the above proposal, please help to send a V2 patch.
For the V2 patch, could you help to rename the title to:
MdeModulePkg/XhciDxe: Error handle for USB slot initialization failure
The package name is needed for title for easy reference.

If you are able to test the PEI Xhci stack, could you help to check if a
similar issue exists under: MdeModulePkg\Bus\Pci\XhciPei?
If not, then only changing the XhciDxe will be fine.

Also, could you help to provide the information on what tests have been done
for the patch?

Thanks in advance.


+ } else {
+ DEBUG ((DEBUG_INFO, " Address %d assigned unsuccessfully\n"));
+ Status = XhcDisableSlotCmd (Xhc, SlotId);
}

Notice that even if a slot have been disable, but it is not reused. I have a try:
1. the USB keyboard is port 0, slot 1:
UsbEnumeratePort: new device connected at port 0 ...
Enable Slot Successfully, The Slot ID = 0x1 2. remove USB keyboard, slot 1 is
disabled:
Disable device slot 1!
...
UsbEnumeratePort: device disconnected event on port 0 3. plug in USB
keyboard in port 0 again, but slot ID is 4 now.
UsbEnumeratePort: new device connected at port 0 Enable Slot Successfully,
The Slot ID = 0x4

So I think we can reuse slot ID unless previous slot ID is 255.

I think it is the controller's behavior to return which slot ID when a
'Enable Slot' command is received. The current proposal of using 'Disable Slot'
to inform the xHCI that a Device Slot is no longer needed looks good to me.

Best Regards,
Hao Wu



Thanks,
Heng

-----Original Message-----
From: Wu, Hao A <hao.a.wu@...>
Sent: Friday, October 16, 2020 3:05 PM
To: Luo, Heng <heng.luo@...>; devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>
Subject: RE: [PATCH] XhciDxe: Clean up UsbDevContext if USB slot
initialization is failed

-----Original Message-----
From: Luo, Heng <heng.luo@...>
Sent: Thursday, October 15, 2020 9:49 AM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>; Wu, Hao A <hao.a.wu@...>
Subject: [PATCH] XhciDxe: Clean up UsbDevContext if USB slot
initialization is failed

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3007

Thanks for the patch Heng.

After looking into the analysis at
https://bugzilla.tianocore.org/show_bug.cgi?id=3007#c1:
|> Enable Slot Successfully, The Slot ID = 0x3
|> Address 3 assigned successfully
|> UsbEnumerateNewDev: hub port 15 is reset
|> UsbEnumerateNewDev: device is of 3 speed
|> UsbEnumerateNewDev: device uses translator (0, 0)
|> XhcControlTransfer: SlotId == 2 DeviceAddress=0

The wrong 'SlotId' is used for the control transfer command, where
SlotId 2 is for the device failed to be initialized.
Heng, could you help to check whether it is possible for the next
device to use SlotId 2 again? Thanks in advance.

Best Regards,
Hao Wu



Currently UsbDevContext is not cleaned up if USB slot initialization
is failed, the wrong context data will affect next USB devices and
the USB devices can not be enumerated.
Need to clean up UsbDevContext if USB slot initialization is failed.

Cc: Ray Ni <ray.ni@...>
Cc: Hao A Wu <hao.a.wu@...>
Signed-off-by: Heng Luo <heng.luo@...>
---
MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
index 9cb115363c..1e8430ac34 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
@@ -2,7 +2,7 @@
XHCI transfer scheduling routines. -Copyright (c) 2011 - 2018,
Intel Corporation. All rights reserved.<BR>+Copyright (c) 2011 -
2020, Intel Corporation. All rights reserved.<BR> Copyright (c)
Microsoft Corporation.<BR> SPDX-License-Identifier:
BSD-2-Clause-Patent @@
-2279,6
+2279,9 @@ XhcInitializeDeviceSlot (
DeviceAddress = (UINT8) ((DEVICE_CONTEXT *) OutputContext)-
Slot.DeviceAddress; DEBUG ((EFI_D_INFO, " Address %d assigned
successfully\n", DeviceAddress)); Xhc-
UsbDevContext[SlotId].XhciDevAddr = DeviceAddress;+ } else {+
DEBUG
((DEBUG_INFO, " Address %d assigned unsuccessfully, clean up context
data.\n"));+ ZeroMem (&Xhc->UsbDevContext[SlotId], sizeof
(USB_DEV_CONTEXT)); } return Status;@@ -2489,7 +2492,11 @@
XhcInitializeDeviceSlot64 (
DeviceAddress = (UINT8) ((DEVICE_CONTEXT_64 *) OutputContext)-
Slot.DeviceAddress; DEBUG ((EFI_D_INFO, " Address %d assigned
successfully\n", DeviceAddress)); Xhc-
UsbDevContext[SlotId].XhciDevAddr = DeviceAddress;+ } else {+
DEBUG
((DEBUG_INFO, " Address %d assigned unsuccessfully, clean up context
data.\n"));+ ZeroMem (&Xhc->UsbDevContext[SlotId], sizeof
(USB_DEV_CONTEXT)); }+ return Status; } --
2.24.0.windows.2

Join devel@edk2.groups.io to automatically receive all group messages.