* [PATCH] XhciDxe: Clean up UsbDevContext if USB slot initialization is failed
@ 2020-10-15 1:49 Heng Luo
2020-10-16 7:04 ` Wu, Hao A
0 siblings, 1 reply; 5+ messages in thread
From: Heng Luo @ 2020-10-15 1:49 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Hao A Wu
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3007
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@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Heng Luo <heng.luo@intel.com>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] XhciDxe: Clean up UsbDevContext if USB slot initialization is failed
2020-10-15 1:49 [PATCH] XhciDxe: Clean up UsbDevContext if USB slot initialization is failed Heng Luo
@ 2020-10-16 7:04 ` Wu, Hao A
2020-10-19 3:03 ` Heng Luo
0 siblings, 1 reply; 5+ messages in thread
From: Wu, Hao A @ 2020-10-16 7:04 UTC (permalink / raw)
To: Luo, Heng, devel@edk2.groups.io; +Cc: Ni, Ray
> -----Original Message-----
> From: Luo, Heng <heng.luo@intel.com>
> Sent: Thursday, October 15, 2020 9:49 AM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> 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@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Signed-off-by: Heng Luo <heng.luo@intel.com>
> ---
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] XhciDxe: Clean up UsbDevContext if USB slot initialization is failed
2020-10-16 7:04 ` Wu, Hao A
@ 2020-10-19 3:03 ` Heng Luo
2020-10-20 2:22 ` Wu, Hao A
0 siblings, 1 reply; 5+ messages in thread
From: Heng Luo @ 2020-10-19 3:03 UTC (permalink / raw)
To: Wu, Hao A, devel@edk2.groups.io; +Cc: Ni, Ray
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.
+ } 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.
Thanks,
Heng
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Friday, October 16, 2020 3:05 PM
> To: Luo, Heng <heng.luo@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: RE: [PATCH] XhciDxe: Clean up UsbDevContext if USB slot
> initialization is failed
>
> > -----Original Message-----
> > From: Luo, Heng <heng.luo@intel.com>
> > Sent: Thursday, October 15, 2020 9:49 AM
> > To: devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> > 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@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Signed-off-by: Heng Luo <heng.luo@intel.com>
> > ---
> > 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] XhciDxe: Clean up UsbDevContext if USB slot initialization is failed
2020-10-19 3:03 ` Heng Luo
@ 2020-10-20 2:22 ` Wu, Hao A
2020-10-20 3:15 ` Heng Luo
0 siblings, 1 reply; 5+ messages in thread
From: Wu, Hao A @ 2020-10-20 2:22 UTC (permalink / raw)
To: Luo, Heng, devel@edk2.groups.io; +Cc: Ni, Ray
Thanks Heng,
Inline comments below:
> -----Original Message-----
> From: Luo, Heng <heng.luo@intel.com>
> Sent: Monday, October 19, 2020 11:04 AM
> To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>
> 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@intel.com>
> > Sent: Friday, October 16, 2020 3:05 PM
> > To: Luo, Heng <heng.luo@intel.com>; devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>
> > Subject: RE: [PATCH] XhciDxe: Clean up UsbDevContext if USB slot
> > initialization is failed
> >
> > > -----Original Message-----
> > > From: Luo, Heng <heng.luo@intel.com>
> > > Sent: Thursday, October 15, 2020 9:49 AM
> > > To: devel@edk2.groups.io
> > > Cc: Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> > > 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@intel.com>
> > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > Signed-off-by: Heng Luo <heng.luo@intel.com>
> > > ---
> > > 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] XhciDxe: Clean up UsbDevContext if USB slot initialization is failed
2020-10-20 2:22 ` Wu, Hao A
@ 2020-10-20 3:15 ` Heng Luo
0 siblings, 0 replies; 5+ messages in thread
From: Heng Luo @ 2020-10-20 3:15 UTC (permalink / raw)
To: Wu, Hao A, devel@edk2.groups.io; +Cc: Ni, Ray
Thanks Hao,
Sorry I don't know how to test USB on PEI phase.
I will send patch V2 following your comments.
Thanks,
Heng
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Tuesday, October 20, 2020 10:22 AM
> To: Luo, Heng <heng.luo@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: RE: [PATCH] XhciDxe: Clean up UsbDevContext if USB slot
> initialization is failed
>
> Thanks Heng,
>
> Inline comments below:
>
> > -----Original Message-----
> > From: Luo, Heng <heng.luo@intel.com>
> > Sent: Monday, October 19, 2020 11:04 AM
> > To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>
> > 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@intel.com>
> > > Sent: Friday, October 16, 2020 3:05 PM
> > > To: Luo, Heng <heng.luo@intel.com>; devel@edk2.groups.io
> > > Cc: Ni, Ray <ray.ni@intel.com>
> > > Subject: RE: [PATCH] XhciDxe: Clean up UsbDevContext if USB slot
> > > initialization is failed
> > >
> > > > -----Original Message-----
> > > > From: Luo, Heng <heng.luo@intel.com>
> > > > Sent: Thursday, October 15, 2020 9:49 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> > > > 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@intel.com>
> > > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > > Signed-off-by: Heng Luo <heng.luo@intel.com>
> > > > ---
> > > > 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-20 3:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-15 1:49 [PATCH] XhciDxe: Clean up UsbDevContext if USB slot initialization is failed Heng Luo
2020-10-16 7:04 ` Wu, Hao A
2020-10-19 3:03 ` Heng Luo
2020-10-20 2:22 ` Wu, Hao A
2020-10-20 3:15 ` Heng Luo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox