From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by mx.groups.io with SMTP id smtpd.web11.3653.1600133263296517679 for ; Mon, 14 Sep 2020 18:27:43 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@intel.onmicrosoft.com header.s=selector2-intel-onmicrosoft-com header.b=iNuZNCTU; spf=pass (domain: intel.com, ip: 134.134.136.20, mailfrom: hao.a.wu@intel.com) IronPort-SDR: eI4r6iK/McoUfD2QnGe/mEOy6GWFTT7j7SHz1XMvtPEOdba+wQOlLP0kHHNlVNu2EtspihdVYz y6sHFtG4b2wA== X-IronPort-AV: E=McAfee;i="6000,8403,9744"; a="146873731" X-IronPort-AV: E=Sophos;i="5.76,427,1592895600"; d="scan'208";a="146873731" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Sep 2020 18:27:42 -0700 IronPort-SDR: v0N+h4JGnOPGn9VN8CBDEwGvZglyxs9JYaQ2At6+UdnF/al/oep5yD6GITCHfFgLJEO+ayC5Ld x5IHLD68YY7w== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.76,427,1592895600"; d="scan'208";a="451097145" Received: from fmsmsx603.amr.corp.intel.com ([10.18.126.83]) by orsmga004.jf.intel.com with ESMTP; 14 Sep 2020 18:27:42 -0700 Received: from fmsmsx612.amr.corp.intel.com (10.18.126.92) by fmsmsx603.amr.corp.intel.com (10.18.126.83) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Mon, 14 Sep 2020 18:27:41 -0700 Received: from fmsedg601.ED.cps.intel.com (10.1.192.135) by fmsmsx612.amr.corp.intel.com (10.18.126.92) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5 via Frontend Transport; Mon, 14 Sep 2020 18:27:41 -0700 Received: from NAM02-BL2-obe.outbound.protection.outlook.com (104.47.38.51) by edgegateway.intel.com (192.55.55.70) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.1713.5; Mon, 14 Sep 2020 18:27:41 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=i7HZgFRhFNkzAufbEiGVWgIlb/804DYlvDaPVEP0Q+ee6i2hyJRZ0u5wosigxT9bwgl/IRl9LEV/7yhFA2xrDD53f+f7wwiyvpID4zLWne8jBiN5CLd5yOYLXt9YAnf+ISYz7B3PrCGDlAPpHtjVncHaI8aywbPIH0Tm0Xt/5dBcBBGNOEufa0Tmxd0HOqIGNujzm58HadYIUQASzwaIojtLx4xtTb93r7rPn/4NKmLjBapuERLebPHwnpdHk3icB4MTsou+rHtZNRi5aPABTaOk9TepCEGHi2OortDRcNzb5CeZXzJ1ahM6lOi/cbswU04IOthFfD3yELcusy7toQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=qr37IbF5AjnF0MbsqmEusJ1YWYGAkFnoG1okPtdvmlo=; b=kd+Tz7bfnCTU7qZ7ODjM8263ZFES9qVtN18Lo2Rk1mVlVVlBOATaCUXzHQwUgVDLVEHUu/aC/7kK1yb2pCz9E0AXua4pCn8J1AZ4IpjR10suwC9XGz753GoVQ60BHa9JV/GkrPG9Nxte3FyIB20CqhC9NjaKF+g9uH+QCNuuvyaKaP+EhPID3reqj5EkmL5zXw5h6PgHGDmIn6UQKqRC8hnksgOMo4zMsx4y62hyAtpU5dNJjVP9+1A8vA/1uo1NHYmntOyoMopyGDSSvuXW+eQfUcvXZ5YAv0PWbHZAshHMe2R8ckpAr+Z3TOgCcudYj0stBeu43OoTRWSJ5wFa+g== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=qr37IbF5AjnF0MbsqmEusJ1YWYGAkFnoG1okPtdvmlo=; b=iNuZNCTUi1DWoItzEPCOizEJV6t1eivvlmiTzrG5psHkkrqLHg2d7dBlM00lkyU6yO5hPOsGfJh1IOoNNIJj41UpqXuCJ1xRNtIHp0Qz2/+63/Ns8UUX8ncoeLI2KiHjoATYN4lPrBuBmW6Vo+QMMG7VWq6tkE+WKJdVTBGU3Vw= Received: from BYAPR11MB3670.namprd11.prod.outlook.com (2603:10b6:a03:f8::11) by BY5PR11MB3909.namprd11.prod.outlook.com (2603:10b6:a03:191::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3370.17; Tue, 15 Sep 2020 01:27:39 +0000 Received: from BYAPR11MB3670.namprd11.prod.outlook.com ([fe80::2129:60f2:c049:c024]) by BYAPR11MB3670.namprd11.prod.outlook.com ([fe80::2129:60f2:c049:c024%7]) with mapi id 15.20.3370.019; Tue, 15 Sep 2020 01:27:39 +0000 From: "Wu, Hao A" To: "devel@edk2.groups.io" , "patrick.henz@hpe.com" , "Ni, Ray" CC: "Wang, Jian J" Subject: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts Thread-Topic: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts Thread-Index: AQHWgUs82cadCyRNmkKDU4HvBxhYqqlWJO1AgAwdBQCABroOYA== Date: Tue, 15 Sep 2020 01:27:38 +0000 Message-ID: References: In-Reply-To: Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.5.1.3 authentication-results: edk2.groups.io; dkim=none (message not signed) header.d=none;edk2.groups.io; dmarc=none action=none header.from=intel.com; x-originating-ip: [192.198.147.218] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 4c91381e-dd44-401f-5975-08d8591688d1 x-ms-traffictypediagnostic: BY5PR11MB3909: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:9508; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: N1++eWnTn/LeqSclZwOmT2HXFWNkp0vGlbshJEejspXWiA5Soz8c22QEvMtR56JnCr7XSC7RU2ONzfrVjzLbr7WeH/OGgFzbLRpbqHowXPhjSMarFduOVh8M7JiJUmsSAf/zfgGc/PDE3RriM4Ly8No6QT/GZaN4WMQZWr49IXlKMDeOsVueu1eaKf1WJJhKq8wYPOFQojAlen6Cs8xoovZ1ISum8HWhCHqy1yvAQKaD0A+j1Wu5Zyb7b0mtDq6rujxWJX3fRvcXd9YTrRn6KAnSb+unoPzBupPbx0TfvCmt+74t2keijOGvVh9WYoSoFOIFKLR534aR0wqhcZT2zB0cf//zx7pyf0drZUQb3PodeKfugCt3PTT3A58dj9UiPU3DFDsPPwB3RnFzsAp4RQ== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BYAPR11MB3670.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(136003)(396003)(346002)(39860400002)(376002)(366004)(966005)(71200400001)(2906002)(8676002)(19627235002)(7696005)(83380400001)(33656002)(53546011)(6506007)(52536014)(6636002)(316002)(110136005)(5660300002)(4326008)(26005)(76116006)(66476007)(8936002)(9686003)(66946007)(478600001)(55016002)(66446008)(64756008)(66556008)(186003)(86362001)(107886003);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: hUWEaf6R5+avNj7szynwexhHoJFCsaOoBtjrp9BG/Yt0WMwZ6GLpPudlCiqlW4snMa8r8RDULODHJRKvg+533UIqtfOfmtF+SRD6y7x5lkeh1kfV8jhtRVgkSIVDgeWActI+yAwlWPhwZNeeuBNyV1WblS3r/MAkmZXioJVPxvpt1Jb/uYwwgqBMlbx3tG9lCTolIMZvsvF9y9OD0sF0uZEGgMZtVj20M4Mdu3WqIw5QJQY1xturCeMBf4OyyHBf1EIhWBIVYncmjcnC6EgrNFs6lJcU3A3rtEF/ByYtIz2jPPmuL5+JCu/6u/62QVYnJdmJAtzzHS03Us2fNpCkhCEwgbg8UOmNK3fZf8NoAALsG29E89JiSfPFqPf9H9CCcNUvDPQbf+js6KU8gCIrEQRzgGz3VnjirtRKd7aMM0XeT9XWhRNAKbDCNqm9gZJYT2cqnu18/MgjfPod0Hky0N255WbIsiSpIxMIv7gi9VRwHAGkjBAGGjlh2RrDPQ8bFiTLa86acO41rs80icLRzBBJkC7V+43fiZ/G/Jo59cWqowT7Ua9OIeSSIn/v3JJWTe4EkfYKVrBAAK2pk3N6qfUJ2tb+Jw89gd9Z9irUmxn2LkZn0/H7PdbY9Wkmua3WwMzlfIMqpfOOlxGtqX7Cog== MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3670.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 4c91381e-dd44-401f-5975-08d8591688d1 X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Sep 2020 01:27:38.7252 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: r3LmSRMDTcGe//8LPoL2BNguUoFuI022fb6B6Ll+cx1ZNdNT+2n/jCa34FYKxd9y2E+FDFJWss5WEkwRP3u/7w== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR11MB3909 Return-Path: hao.a.wu@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: devel@edk2.groups.io On Behalf Of Henz, > Patrick > Sent: Friday, September 11, 2020 2:43 AM > To: devel@edk2.groups.io; Wu, Hao A ; Ni, Ray > > Cc: Wang, Jian J > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix > Broken Timeouts >=20 > Hi Hao and Ray, >=20 > In regards to the behavior when Timeout =3D=3D 0, I did try to account f= or that in > my initial patch with the following logic: >=20 > =09(0 =3D=3D > Timeout)?(EFI_TIMER_PERIOD_MICROSECONDS(0xFFFFFFFF)):(EFI_TIMER_ > PERIOD_MILLISECONDS(Timeout)) >=20 > This results in a timeout that happens sooner than what the current code > would produce, but it falls in line with what the original code seemed t= o > intend to do. Ray suggested that I enhance the code by not creating the > timer event when Timeout is 0, which I assumed meant that XhcExecTransfe= r > () would just return. I personally think it would be a good idea to keep= this > behavior in the code, but would like Ray's input on the matter. Apprecia= te I prefer to keep the origin behavior. Ray, what is your comment on this? Best Regards, Hao Wu > the help! >=20 > Thanks, > Patrick Henz >=20 > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Wu, Hao A > Sent: Wednesday, September 2, 2020 9:25 PM > To: devel@edk2.groups.io; Henz, Patrick ; Ni, Ray > > Cc: Wang, Jian J > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix > Broken Timeouts >=20 > Hello Patrick, a couple of inline comments below. > Hello Ray, need your input on one thing below as well. >=20 >=20 > > -----Original Message----- > > From: devel@edk2.groups.io On Behalf Of > > patrick.henz@hpe.com > > Sent: Thursday, September 3, 2020 1:05 AM > > To: devel@edk2.groups.io > > Cc: Patrick Henz ; Wang, Jian J > > ; Wu, Hao A ; Ni, Ray > > > > Subject: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix Broken > > Timeouts > > > > From: Patrick Henz > > > > REF:INVALID URI REMOVED > > ocore.org_show-5Fbug.cgi-3Fid- > 3D2948&d=3DDwIFAg&c=3DC5b8zRQO1miGmBeVZ2LFWg > > > &r=3Dwx4n0HbqxSAP19Eklmv6gq7ivDQlqQ_ITOkZIBUNNKg&m=3DOKlWpRL8ZyDf > hUEh6S4OU > > aMasig0MPoajX7Vz2sDSvY&s=3DLTDbPsspkRCcWFBfThqhR_FaljF2kQLagB_t- > kbAm80&e > > =3D > > > > Timeouts in the XhciDxe driver are taking longer than expected due to > > the timeout loops not accounting for code execution time. As en > > example, 5 second timeouts have been observed to take around 36 > > seconds to complete. > > Use SetTimer and Create/CheckEvent from Boot Services to determine > > when timeout occurred. > > > > Cc: Jian J Wang > > Cc: Hao A Wu > > Cc: Ray Ni > > Signed-off-by: Patrick Henz > > --- > > MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c | 35 ++++++++++++++++--- > > MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 43 > > +++++++++++++++++++----- > > 2 files changed, 65 insertions(+), 13 deletions(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c > > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c > > index 42b773ab31be..33ac13504669 100644 > > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c > > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c > > @@ -442,17 +442,44 @@ XhcWaitOpRegBit ( > > IN UINT32 Timeout > > ) > > { > > - UINT32 Index; > > - UINT64 Loop; > > + EFI_STATUS Status; > > + EFI_EVENT TimeoutEvent; > > > > - Loop =3D Timeout * XHC_1_MILLISECOND; > > + TimeoutEvent =3D NULL; > > > > - for (Index =3D 0; Index < Loop; Index++) { > > + if (Timeout =3D=3D 0) { > > + goto TIMEOUT; > > + } > > + > > + Status =3D gBS->CreateEvent ( > > + EVT_TIMER, > > + TPL_CALLBACK, > > + NULL, > > + NULL, > > + &TimeoutEvent > > + ); > > + > > + if (!EFI_ERROR (Status)) { > > + Status =3D gBS->SetTimer (TimeoutEvent, > > + TimerRelative, > > + EFI_TIMER_PERIOD_MILLISECONDS(Timeout)); > > + } > > + > > + if (EFI_ERROR(Status)) { > > + goto TIMEOUT; > > + } >=20 >=20 > Could you help to refine the return status for the case when CreateEvent= or > SetTimer calls fail? > I think it will return EFI_TIMEOUT at this moment, which might confuse t= he > caller. > You may need to modify the function description comment section for the > new return value also. >=20 > A similar case applies to XhcExecTransfer() as well. >=20 >=20 > > + > > + do { > > if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) =3D=3D WaitToSet) { > > return EFI_SUCCESS; > > } > > > > gBS->Stall (XHC_1_MICROSECOND); > > + } while (EFI_ERROR(gBS->CheckEvent (TimeoutEvent))); > > + > > +TIMEOUT: > > + if (TimeoutEvent !=3D NULL) { > > + gBS->CloseEvent (TimeoutEvent); > > } > > > > return EFI_TIMEOUT; > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c > > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c > > index ab8957c546ee..d6290b5fe33b 100644 > > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c > > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c > > @@ -1273,11 +1273,19 @@ XhcExecTransfer ( > > ) > > { > > EFI_STATUS Status; > > - UINTN Index; > > - UINT64 Loop; > > UINT8 SlotId; > > UINT8 Dci; > > BOOLEAN Finished; > > + EFI_EVENT TimeoutEvent; > > + EFI_STATUS TimerStatus; > > + > > + Status =3D EFI_SUCCESS; > > + Finished =3D FALSE; > > + TimeoutEvent =3D NULL; > > + > > + if (Timeout =3D=3D 0) { > > + goto DONE; > > + } > > > > if (CmdTransfer) { > > SlotId =3D 0; > > @@ -1291,29 +1299,46 @@ XhcExecTransfer ( > > ASSERT (Dci < 32); > > } > > > > - Status =3D EFI_SUCCESS; > > - Loop =3D Timeout * XHC_1_MILLISECOND; > > - if (Timeout =3D=3D 0) { > > - Loop =3D 0xFFFFFFFF; >=20 >=20 > Ray and Patrick, the previous behavior when 'Timeout' is 0 for this func= tion is > that it will do a 'psudo-indefinite' loop by setting the 'Loop' variable= to the > value of MAX_UINT32. >=20 > But after the patch, the behavior got changed and the function will dire= ctly > return EFI_TIMEOUT when 'Timeout' is 0. > This behavior change might impact the callers when they expecting a 'psu= do- > indefinite' timeout. > I think it would be better to keep the origin behavior, what is your tho= ught? >=20 > Best Regards, > Hao Wu >=20 >=20 > > + TimerStatus =3D gBS->CreateEvent ( > > + EVT_TIMER, > > + TPL_CALLBACK, > > + NULL, > > + NULL, > > + &TimeoutEvent > > + ); > > + > > + if (!EFI_ERROR (TimerStatus)) { > > + TimerStatus =3D gBS->SetTimer (TimeoutEvent, > > + TimerRelative, > > + > > + EFI_TIMER_PERIOD_MILLISECONDS(Timeout)); > > + } > > + > > + if (EFI_ERROR (TimerStatus)) { > > + goto DONE; > > } > > > > XhcRingDoorBell (Xhc, SlotId, Dci); > > > > - for (Index =3D 0; Index < Loop; Index++) { > > + do { > > Finished =3D XhcCheckUrbResult (Xhc, Urb); > > if (Finished) { > > break; > > } > > gBS->Stall (XHC_1_MICROSECOND); > > - } > > + } while (EFI_ERROR(gBS->CheckEvent (TimeoutEvent))); > > > > - if (Index =3D=3D Loop) { > > +DONE: > > + if (!Finished) { > > Urb->Result =3D EFI_USB_ERR_TIMEOUT; > > Status =3D EFI_TIMEOUT; > > } else if (Urb->Result !=3D EFI_USB_NOERROR) { > > Status =3D EFI_DEVICE_ERROR; > > } > > > > + if (TimeoutEvent !=3D NULL) { > > + gBS->CloseEvent (TimeoutEvent); > > + } > > + > > return Status; > > } > > > > -- > > 2.28.0 > > > > > > >=20 >=20 >=20 >=20 >=20 >=20