[edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: On reset rebuild descriptor table

[Guomin Jiang]

Hi Jeremy,

Let move the discussion into https://edk2.groups.io/g/devel/message/58125, and I will add history in that message.

> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Jeremy
> Linton
> Sent: Tuesday, April 28, 2020 8:44 AM
> To: Jiang, Guomin <guomin.jiang at intel.com>; devel at edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu at intel.com>; Ni, Ray <ray.ni at intel.com>; Kinney,
> Michael D <michael.d.kinney at intel.com>; Tian, Feng <feng.tian at intel.com>;
> Wang, Jian J <jian.j.wang at intel.com>; ard.biesheuvel at arm.com
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: On reset
> rebuild descriptor table
> 
> Hi!
> 
> On 4/27/20 7:15 PM, Jiang, Guomin wrote:
> > Hi Jeremy,
> >
> > You can refer https://edk2.groups.io/g/devel/message/58125 for discussion
> about this solution.
> 
> Oh fun, odd how a bug can exist in a code base for years and then this
> happens.. I will move the discussion there.
> 
> Thanks,
> 
> >
> > Two issue I found:
> > 1. Memory leakage may occur if doing so and I am investigating it.
> 
> It seems our solutions differ a bit?

Memory leakage will happened when invoke UsbBuildDescTable(), because the old allocated buffer haven't been freed but allocated new
> 
> > 2. It test pass with OVMF but fail in real platform, and I am figuring out the
> flow.
> 
> Hmm, I've been seeing this on a RPI with an attached USB3 hub and 5 bay USB
> JBOD. (there is another problem but the reset crash is keeps the machine
> from booting).
> 
> 
> 
> 
> >
> > Best Regards
> > Guomin
> >
> >> -----Original Message-----
> >> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
> Jeremy
> >> Linton
> >> Sent: Tuesday, April 28, 2020 6:16 AM
> >> To: devel at edk2.groups.io
> >> Cc: Wu, Hao A <hao.a.wu at intel.com>; Ni, Ray <ray.ni at intel.com>;
> >> Kinney, Michael D <michael.d.kinney at intel.com>; Tian, Feng
> >> <feng.tian at intel.com>; Wang, Jian J <jian.j.wang at intel.com>;
> >> ard.biesheuvel at arm.com; Jeremy Linton <jeremy.linton at arm.com>
> >> Subject: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: On reset
> >> rebuild descriptor table
> >>
> >> During port reset, the device descriptors should be checked before
> >> attempting to set an endpoint configuration.
> >>
> >> In particular this fixes a crash due to ASSERT(TrsRing != NULL) in
> >> XhcSyncTrsRing(). That crash happens during error recovery on devices
> >> attached to XHCI hosts. This is because the port disable clears and
> >> deallocats all the EP data structures. When the port is reconfigured
> >> without first requesting the EP descriptors,
> >> XhcSetConfigCmd[64]() is not being called because the
> >> NumConfigurations remains 0.
> >>
> >> We could attempt to rebuild the EP descriptions directly from the
> >> XHCI driver. OTOH, its probably good practice to assure the device
> >> description is what we expect from within the core USB subsystem
> >> during reset.
> >>
> >> Signed-off-by: Jeremy Linton <jeremy.linton at arm.com>
> >> ---
> >>   MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c | 8 ++++++++
> >>   1 file changed, 8 insertions(+)
> >>
> >> diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> >> b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> >> index 4b4915c019..17bb691bf8 100644
> >> --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> >> +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> >> @@ -874,6 +874,14 @@ UsbIoPortReset (
> >>     // is in CONFIGURED state.
> >>
> >>     //
> >>
> >>     if (Dev->ActiveConfig != NULL) {
> >>
> >> +    Status = UsbBuildDescTable (Dev);
> >>
> >> +
> >>
> >> +    if (EFI_ERROR (Status)) {
> >>
> >> +      DEBUG ((EFI_D_ERROR, "UsbIoPortReset: failed to build
> >> + descriptor table
> >> for %d - %r\n",
> >>
> >> +                 Dev->Address, Status));
> >>
> >> +      goto ON_EXIT;
> >>
> >> +    }
> >>
> >> +
> >>
> >>       Status = UsbSetConfig (Dev,
> >> Dev->ActiveConfig->Desc.ConfigurationValue);
> >>
> >>
> >>
> >>       if (EFI_ERROR (Status)) {
> >>
> >> --
> >> 2.24.1
> >>
> >>
> >> -=-=-=-=-=-=
> >> Groups.io Links: You receive all messages sent to this group.
> >>
> >> View/Reply Online (#58193):
> >> https://edk2.groups.io/g/devel/message/58193
> >> Mute This Topic: https://groups.io/mt/73315690/4399222
> >> Group Owner: devel+owner at edk2.groups.io
> >> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> >> [guomin.jiang at intel.com] -=-=-=-=-=-=
> >
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#58200): https://edk2.groups.io/g/devel/message/58200
Mute This Topic: https://groups.io/mt/73315690/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-