From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Jiang, Guomin" <guomin.jiang@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Wang, Jian J" <jian.j.wang@intel.com>, "Ni, Ray" <ray.ni@intel.com>
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: Rebuild the description table after Reset Device
Date: Sun, 26 Apr 2020 08:03:37 +0000 [thread overview]
Message-ID: <B80AF82E9BFB8E4FBD8C89DA810C6A093C9F81EE@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <DM6PR11MB29557DCFAF0C7F831DA4C60A9DAE0@DM6PR11MB2955.namprd11.prod.outlook.com>
> -----Original Message-----
> From: Jiang, Guomin
> Sent: Sunday, April 26, 2020 2:44 PM
> To: Wu, Hao A; devel@edk2.groups.io
> Cc: Wang, Jian J; Ni, Ray
> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: Rebuild the
> description table after Reset Device
>
> Hi Hao,
>
> Add comments inline.
>
> > -----Original Message-----
> > From: Wu, Hao A <hao.a.wu@intel.com>
> > Sent: Sunday, April 26, 2020 1:12 PM
> > To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Jiang,
> Guomin
> > <guomin.jiang@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>
> > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: Rebuild the
> > description table after Reset Device
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf
> Of
> > > Wu, Hao A
> > > Sent: Sunday, April 26, 2020 1:10 PM
> > > To: Jiang, Guomin; devel@edk2.groups.io
> > > Cc: Wang, Jian J; Ni, Ray
> > > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: Rebuild
> > the
> > > description table after Reset Device
> > >
> > > > -----Original Message-----
> > > > From: Jiang, Guomin
> > > > Sent: Saturday, April 25, 2020 9:36 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: Wang, Jian J; Wu, Hao A; Ni, Ray
> > > > Subject: [PATCH] MdeModulePkg/UsbBusDxe: Rebuild the description
> > > > table after Reset Device
> > > >
> > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2694
> > > >
> > > > When the USB fail and then Reset Device, it should rebuild description.
> > > >
> > > > Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
> > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > > Cc: Ray Ni <ray.ni@intel.com>
> > > > ---
> > > > MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c | 5 +++++
> > > > 1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > > > b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > > > index 4b4915c019..9f2d2cc87f 100644
> > > > --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > > > +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > > > @@ -869,6 +869,11 @@ UsbIoPortReset (
> > > >
> > > >
> > > > DEBUG (( EFI_D_INFO, "UsbIoPortReset: device is now ADDRESSED at
> > > > %d\n", Dev->Address));
> > > >
> > > >
> > > >
> > > > + //
> > > >
> > > > + // The description will be invalid after reset, should rebuild it as well.
> > > >
> > > > + //
> > > >
> > > > + UsbBuildDescTable (Dev);
> > >
> > >
> > > Hello Guomin,
> > >
> > > Thanks for the proposed patch.
> > >
> > > Could you help to explain in more detail for the above fix with regard
> > > to the transfer ring not being set properly in the XHCI driver? Thanks.
>
> When I debug, I dump the below data structure and found that before
> UsbMassReset, the corresponding slot transfer ring is normal and invalid
> after UsbMassReset.
> USB_DEV_CONTEXT = 0x148
> +0x0 Enabled
> +0x1 SlotId
> +0x4 RouteString
> +0x8 ParentRouteString
> +0xC XhciDevAddr
> +0xD BusDevAddr
> +0x10 InputContext
> +0x18 OutputContext
> +0x20 EndpointTransferRing[31]
> +0x118 DevDesc
> +0x130 ConfDesc
> +0x138 ActiveConfiguration
> +0x140 ActiveAlternateSetting
>
> Before UsbMassReset
> 000002D0: -01 01 00 00 00 00 10 10
> 000002E0: 00 00 00 00 01 01 00 00-80 02 FA 06 00 00 00 00
> 000002F0: C0 16 FA 06 00 00 00 00-98 27 FC 06 00 00 00 00
> 00000300: 00 00 00 00 00 00 00 00-98 26 FC 06 00 00 00 00
> 00000310: 98 BE F9 06 00 00 00 00-00 00 00 00 00 00 00 00
> 00000320: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 00000330: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 00000340: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 00000350: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 00000360: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 00000370: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 00000380: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 00000390: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 000003A0: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 000003B0: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 000003C0: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 000003D0: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 000003E0: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 000003F0: 12 01 00 03 00 00 00 09-F4 46 01 00 00 00 01 02
> 00000400: 03 01 00 00 00 00 00 00-98 CD F9 06 00 00 00 00
> 00000410: 01 00 00 00 00 00 00 00-18 24 FC 06 00 00 00 00
>
> After UsbMassReset
> 000002D0: -01 01 00 00 00 00 10 10
> 000002E0: 00 00 00 00 01 01 00 00-80 02 FA 06 00 00 00 00
> 000002F0: C0 16 FA 06 00 00 00 00-00 00 00 00 00 00 00 00
> 00000300: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 00000310: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 00000320: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 00000330: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 00000340: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 00000350: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 00000360: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 00000370: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 00000380: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 00000390: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 000003A0: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 000003B0: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 000003C0: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 000003D0: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 000003E0: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00
> 000003F0: 12 01 00 03 00 00 00 09-F4 46 01 00 00 00 01 02
> 00000400: 03 01 00 00 00 00 00 00-98 CD F9 06 00 00 00 00
> 00000410: 01 00 00 00 00 00 00 00-18 24 FC 06 00 00 00 00
>
> After Reset Finished, it will try to get data again by sending command to Usb
> Mass Device and it will use USB_DEV_CONTEXT. EndpointTransferRing[4].
> But it haven't been initialized after reset, and ASSERT will trigger when access
> the USB_DEV_CONTEXT. EndpointTransferRing and will show
> ASSERT
> c:\users\guominji\dcg10nm\edk2\MdeModulePkg\Bus\Pci\XhciDxe\XhciSche
> d.c(1909): TrsRing != ((void *) 0)
>
> The EndpointTranferRing should be initialized by UsbBuildDescTable() but
> seem that it is omitted in Reset branch, so I add it.
Sorry Guomin, I still do not fully understand the link between calling the
UsbBuildDescTable() in UsbBusDxe and the re-initialization of the
'EndpointTranferRing' in the XhciDxe. Could you help to provide more detail?
Another thing I found (if current proposed fix is a proper solution) is that in
UsbBuildDescTable() , several memory allocations will be made to store the
device descriptor/config descriptor. Could you help to double check if their
old values are properly freed in order to avoid memory leak?
Best Regards,
Hao Wu
>
> > >
> > > Also, judging from the function description comments in
> > UsbBuildDescTable():
> > > |> /**
> > > |> Build the whole array of descriptors. This function must
> > > |> be called after UsbGetMaxPacketSize0 returns the max packet
> > > |> size correctly for endpoint 0.
> > > |> ...
> > > |> **/
> > > |> EFI_STATUS
> > > |> UsbBuildDescTable (
> > > |> IN USB_DEVICE *UsbDev
> > > |> )
> > >
> > > Does function UsbGetMaxPacketSize0() need to be called before
> > > UsbBuildDescTable() in the proposed fix?
>
> I ignored it and will double check it.
>
> >
> >
> > One more thing, could you help to add the information for what tests have
> > been done for the proposed patch as well?
> >
> > Thanks in advance.
> >
>
> Have test OVMF and test pass, detail as below
> I use qemu-w64-2020020, you should install it first and add it to PATH
> environment, the procedure as below
> 1. Import the test patch from
> https://bugzilla.tianocore.org/attachment.cgi?id=508
> 2. type ```build -p OvmfPkg\OvmfPkgX64.dsc -b DEBUG -a X64``` to generate
> OVMF.fd
> 3. type ```qemu-img create file.img 1G``` to create test image.
> 4. type ```qemu-system-x86_64 -bios OVMF.fd -drive
> if=none,id=stick,file=file.img -device nec-usb-xhci,id=xhci, -device usb-
> storage,bus=xhci.0,drive=stick -global isa-debugcon.iobase=0x402 -
> debugcon file:xhci.log```
> 5. you will see hang currently and will see ASSERT when check xhci.log.
> 6. import proposal patch, the symptom disappear.
>
> >
> > >
> > > Best Regards,
> > > Hao Wu
> > >
> > >
> > > >
> > > > +
> > > >
> > > > //
> > > >
> > > > // Reset the current active configure, after this device
> > > >
> > > > // is in CONFIGURED state.
> > > >
> > > > --
> > > > 2.25.1.windows.1
> > >
> > >
> > >
> >
>
next prev parent reply other threads:[~2020-04-26 8:03 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-25 1:36 [PATCH] MdeModulePkg/UsbBusDxe: Rebuild the description table after Reset Device Guomin Jiang
2020-04-26 5:09 ` Wu, Hao A
2020-04-26 5:12 ` [edk2-devel] " Wu, Hao A
2020-04-26 6:43 ` Guomin Jiang
2020-04-26 8:03 ` Wu, Hao A [this message]
2020-04-26 13:04 ` Guomin Jiang
2020-04-27 1:36 ` Wu, Hao A
2020-04-28 1:03 ` Jeremy Linton
2020-04-28 1:14 ` Guomin Jiang
2020-04-28 2:31 ` Jeremy Linton
2020-04-28 3:22 ` Guomin Jiang
[not found] ` <1609DDC63BB4AEBF.19313@groups.io>
2020-04-30 9:47 ` Guomin Jiang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=B80AF82E9BFB8E4FBD8C89DA810C6A093C9F81EE@SHSMSX104.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox