From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mx.groups.io with SMTP id smtpd.web10.998.1587951393256413005 for ; Sun, 26 Apr 2020 18:36:33 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.136, mailfrom: hao.a.wu@intel.com) IronPort-SDR: eE4e3dbtBNr8V15MVLNx20A6hgLKmHW9/GWjQ11FioNxuvyZFoxQ5JdD5UFpsKqUJiyzbgy1Mf oG1FxfPHs8bA== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Apr 2020 18:36:32 -0700 IronPort-SDR: BKOn+3oy/e/Fe+EW4fODBbwVqpVaL8WSDa3g5BQYAyX5zLY0F+0laU4jVelwpeD7vg0di9IQ5e Khl1fZTm0lHg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,321,1583222400"; d="scan'208";a="366984861" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga001.fm.intel.com with ESMTP; 26 Apr 2020 18:36:32 -0700 Received: from fmsmsx101.amr.corp.intel.com (10.18.124.199) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 26 Apr 2020 18:36:32 -0700 Received: from shsmsx153.ccr.corp.intel.com (10.239.6.53) by fmsmsx101.amr.corp.intel.com (10.18.124.199) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 26 Apr 2020 18:36:32 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.225]) by SHSMSX153.ccr.corp.intel.com ([169.254.12.89]) with mapi id 14.03.0439.000; Mon, 27 Apr 2020 09:36:29 +0800 From: "Wu, Hao A" 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 Thread-Topic: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: Rebuild the description table after Reset Device Thread-Index: AQHWGqH+6oJcz8EY40K8DVzwoZo6F6iK2sQggAACMZCAAA8QEIAAHB0QgABHW8CAAOJngA== Date: Mon, 27 Apr 2020 01:36:28 +0000 Message-ID: References: <20200425013620.1159-1-guomin.jiang@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Jiang, Guomin > Sent: Sunday, April 26, 2020 9:05 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 >=20 > Add comment inline. >=20 > > -----Original Message----- > > From: Wu, Hao A > > Sent: Sunday, April 26, 2020 4:04 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: 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 > > > > Sent: Sunday, April 26, 2020 1:12 PM > > > > To: devel@edk2.groups.io; Wu, Hao A ; Jiang, > > > Guomin > > > > > > > > Cc: Wang, Jian J ; Ni, Ray > > > > 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=3D2694 > > > > > > > > > > > > When the USB fail and then Reset Device, it should rebuild > description. > > > > > > > > > > > > Signed-off-by: Guomin Jiang > > > > > > Cc: Jian J Wang > > > > > > Cc: Hao A Wu > > > > > > Cc: Ray Ni > > > > > > --- > > > > > > 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 rebu= ild 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 d= river? > > 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 =3D 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: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 > > > 00000400: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 > > > 00000410: 01 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 > > > > > > After Reset Finished, it will try to get data again by sending comma= nd > > > 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\XhciSch > > e > > > d.c(1909): TrsRing !=3D ((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 d= etail? >=20 > I am sorry that make confusion to you, I will explain it in detail now. > 1. The critical call flow as below > UsbMassReset() > --> UsbMass->Transport->Reset() =3D=3D UsbBotResetDevice() > --> UsbBot->UsbIo->UsbPortReset() =3D=3D UsbIoPortReset() > --> UsbIf->Device->ParentIf->HubApi->ResetPort() =3D=3D > UsbRootHubResetPort() > --> UsbHcGetRootHubPortStatus() > --> UsbBus->Usb2Hc-GetRootHubPortStatus() =3D=3D > XhcGetRootHubPortStatus() > --> XhcPollPortStatusChange() > --> XhcDisableSlotCmd() or XhcDisableS= lotCmd64() [Very > very critical routine 1] > --> XhcInitializeDeviceSlot() or XhcIn= itializeDeviceSlot64() > [Very very critical routine 2] > --> UsbSetAddress() > --> UsbSetConfig() > --> UsbBus->Usb2Hc->ControlTransfer() =3D=3D XhcContro= lTransfer() > --> XhcSetConfigCmd() or XhcSetConfigCmd64() [Ve= ry very > critical routine 3] >=20 > There are three very very critical routine, > 1. XhcDisableSlotCmd() or XhcDisableSlotCmd64() will disable the slot co= ntext > and free the allocated memory. > 2. XhcInitializeDeviceSlot() or XhcInitializeDeviceSlot() will initializ= e the basic > control and slot endpoint transfer ring. > 3. XhcSetConfigCmd() or XhcSetConfigCmd64() will configure the other > transfer ring. It depend on Xhc- > >UsbDevContext[Slot].DevDesc.NumConfigurations, unfortunately, the > DevDesc haven't benn initialized by UsbBuildDescTable and it is 0. So > XhcSetConfigCmd() or XhcSetConfigCmd64() haven't run. >=20 > After run below 3 critical routine, the device is initialize but > EndpointTransferRing of Input and Output haven't been initialized and st= ill > keep NULL. >=20 > When UsbMassReadBlocks, it will use uninitialized EndpointTransferRing a= nd > will ASSERT. Thanks a lot Guomin, Now I understand the reason behind for the proposed fix. I would suggest to add an abstract of the above analysis in the commit mes= sage, such information will be helpful for others who might work on USB/Xhci in = the future. >=20 > > Another thing I found (if current proposed fix is a proper solution) i= s 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? > > >=20 > I will investigate it and give you feedback. Thanks for this. Best Regards, Hao Wu >=20 > > 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 packe= t > > > > > |> 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 tes= ts > > > > 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=3D508 > > > 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=3Dnone,id=3Dstick,file=3Dfile.img -device nec-usb-xhci,id=3Dxhci,= -device > > > usb- storage,bus=3Dxhci.0,drive=3Dstick -global isa-debugcon.iobase= =3D0x402 > > > - 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 > > > > > > > > > > > > > > >=20 > > > > > > > > > >=20