1. Patchew
  2. EDK2
  3. View series

[edk2-devel] [PATCH] XhciDxe: Clean up UsbDevContext if USB slot initialization is failed

Heng Luo posted 1 patch 3 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/edk2 tags/patchew/20201015014901.1154-1-heng.luo@intel.com
MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
[edk2-devel] [PATCH] XhciDxe: Clean up UsbDevContext if USB slot initialization is failed
Posted by Heng Luo 3 years, 6 months ago 
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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66232): https://edk2.groups.io/g/devel/message/66232
Mute This Topic: https://groups.io/mt/77519924/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] XhciDxe: Clean up UsbDevContext if USB slot initialization is failed
Posted by Wu, Hao A 3 years, 6 months ago 
> -----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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66314): https://edk2.groups.io/g/devel/message/66314
Mute This Topic: https://groups.io/mt/77519924/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] XhciDxe: Clean up UsbDevContext if USB slot initialization is failed
Posted by Heng Luo 3 years, 6 months ago 
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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66372): https://edk2.groups.io/g/devel/message/66372
Mute This Topic: https://groups.io/mt/77519924/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] XhciDxe: Clean up UsbDevContext if USB slot initialization is failed
Posted by Wu, Hao A 3 years, 6 months ago 
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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66438): https://edk2.groups.io/g/devel/message/66438
Mute This Topic: https://groups.io/mt/77519924/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] XhciDxe: Clean up UsbDevContext if USB slot initialization is failed
Posted by Heng Luo 3 years, 6 months ago 
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



-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.