From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by mx.groups.io with SMTP id smtpd.web10.7604.1587888234242227139 for ; Sun, 26 Apr 2020 01:03:54 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.65, mailfrom: hao.a.wu@intel.com) IronPort-SDR: TxtQsAAp9nl/wVmH/YTefOMhUhf6qchZ6VDIo305wl9SU3NdwGCl5n32hAdKqG+J5gxh2IC3fW n1lt2upEQYbA== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Apr 2020 01:03:52 -0700 IronPort-SDR: IxD1PfT1z5HMSnUykp7lG6MkfZYVJoLWIiTL0MU95TGsygGrqE48jEDxuJMGrw8IkcgfnKVSP/ 4BfVh7DXeJFA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,319,1583222400"; d="scan'208";a="293178373" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga008.jf.intel.com with ESMTP; 26 Apr 2020 01:03:52 -0700 Received: from fmsmsx602.amr.corp.intel.com (10.18.126.82) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 26 Apr 2020 01:03:42 -0700 Received: from fmsmsx602.amr.corp.intel.com (10.18.126.82) by fmsmsx602.amr.corp.intel.com (10.18.126.82) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Sun, 26 Apr 2020 01:03:42 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx602.amr.corp.intel.com (10.18.126.82) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Sun, 26 Apr 2020 01:03:41 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.225]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.146]) with mapi id 14.03.0439.000; Sun, 26 Apr 2020 16:03:37 +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+6oJcz8EY40K8DVzwoZo6F6iK2sQggAACMZCAAA8QEIAAHB0Q Date: Sun, 26 Apr 2020 08:03:37 +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 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 >=20 > Hi Hao, >=20 > Add comments inline. >=20 > > -----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 descrip= tion. > > > > > > > > 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 a= t > > > > %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 rega= rd > > > to the transfer ring not being set properly in the XHCI driver? Than= ks. >=20 > 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 >=20 > 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 >=20 > After UsbMassReset > 000002D0: -01 01 00 00 00 00 1= 0 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 >=20 > After Reset Finished, it will try to get data again by sending command t= o Usb > Mass Device and it will use USB_DEV_CONTEXT. EndpointTransferRing[4]. > But it haven't been initialized after reset, and ASSERT will trigger whe= n access > the USB_DEV_CONTEXT. EndpointTransferRing and will show > ASSERT > c:\users\guominji\dcg10nm\edk2\MdeModulePkg\Bus\Pci\XhciDxe\XhciSche > d.c(1909): TrsRing !=3D ((void *) 0) >=20 > 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 detai= l? Another thing I found (if current proposed fix is a proper solution) is th= at in UsbBuildDescTable() , several memory allocations will be made to store the device descriptor/config descriptor. Could you help to double check if the= ir old values are properly freed in order to avoid memory leak? Best Regards, Hao Wu >=20 > > > > > > 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? >=20 > I ignored it and will double check it. >=20 > > > > > > One more thing, could you help to add the information for what tests h= ave > > been done for the proposed patch as well? > > > > Thanks in advance. > > >=20 > 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 generat= e > 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, -de= vice 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. >=20 > > > > > > > > Best Regards, > > > Hao Wu > > > > > > > > > > > > > > + > > > > > > > > // > > > > > > > > // Reset the current active configure, after this device > > > > > > > > // is in CONFIGURED state. > > > > > > > > -- > > > > 2.25.1.windows.1 > > > > > > > > >=20 > > >=20