[edk2-devel] [PATCH] XhciDxe: Clean up UsbDevContext if USB slot initialization is failed
Heng Luo
heng.luo at intel.com
Tue Oct 20 03:15:00 UTC 2020
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 at intel.com>
> Sent: Tuesday, October 20, 2020 10:22 AM
> To: Luo, Heng <heng.luo at intel.com>; devel at edk2.groups.io
> Cc: Ni, Ray <ray.ni at 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 at intel.com>
> > Sent: Monday, October 19, 2020 11:04 AM
> > To: Wu, Hao A <hao.a.wu at intel.com>; devel at edk2.groups.io
> > Cc: Ni, Ray <ray.ni at 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 at intel.com>
> > > Sent: Friday, October 16, 2020 3:05 PM
> > > To: Luo, Heng <heng.luo at intel.com>; devel at edk2.groups.io
> > > Cc: Ni, Ray <ray.ni at intel.com>
> > > Subject: RE: [PATCH] XhciDxe: Clean up UsbDevContext if USB slot
> > > initialization is failed
> > >
> > > > -----Original Message-----
> > > > From: Luo, Heng <heng.luo at intel.com>
> > > > Sent: Thursday, October 15, 2020 9:49 AM
> > > > To: devel at edk2.groups.io
> > > > Cc: Ni, Ray <ray.ni at intel.com>; Wu, Hao A <hao.a.wu at 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 at intel.com>
> > > > Cc: Hao A Wu <hao.a.wu at intel.com>
> > > > Signed-off-by: Heng Luo <heng.luo at 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66440): https://edk2.groups.io/g/devel/message/66440
Mute This Topic: https://groups.io/mt/77519924/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list