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

["Heng Luo" <heng.luo@intel.com>]

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