From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by mx.groups.io with SMTP id smtpd.web10.5066.1600832463049792988 for ; Tue, 22 Sep 2020 20:41:03 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@intel.onmicrosoft.com header.s=selector2-intel-onmicrosoft-com header.b=ladeq/mX; spf=pass (domain: intel.com, ip: 192.55.52.151, mailfrom: ray.ni@intel.com) IronPort-SDR: MnpFzYFj9USbaWXSWvoU9dWbayCFEAYqBlxn4FEwlFBOvao+4yMVRRTciEMkWTW5VzX6EW7+1M qeEvU/uZAa0A== X-IronPort-AV: E=McAfee;i="6000,8403,9752"; a="140783973" X-IronPort-AV: E=Sophos;i="5.77,293,1596524400"; d="scan'208";a="140783973" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Sep 2020 20:41:01 -0700 IronPort-SDR: SDMibcGIwDjEoev1F9yg9h2xwRgmARCDNImq82qkGQzEkYYyZT+FsiJCm0+zh8u0RnY3XFNbyy QOXIlk4CkALQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.77,293,1596524400"; d="scan'208";a="412865439" Received: from fmsmsx601.amr.corp.intel.com ([10.18.126.81]) by fmsmga001.fm.intel.com with ESMTP; 22 Sep 2020 20:41:01 -0700 Received: from fmsmsx603.amr.corp.intel.com (10.18.126.83) by fmsmsx601.amr.corp.intel.com (10.18.126.81) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Tue, 22 Sep 2020 20:41:01 -0700 Received: from fmsedg601.ED.cps.intel.com (10.1.192.135) 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 via Frontend Transport; Tue, 22 Sep 2020 20:41:01 -0700 Received: from NAM11-CO1-obe.outbound.protection.outlook.com (104.47.56.173) 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; Tue, 22 Sep 2020 20:41:00 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=CcVIvfR4ea7pOHbGqdTwEqQfsZB8jRstB9aAydN98zvM2zd4UwZZ5eXw54LkQjXYfJbXAgoDYpQH3za96Q02FmXhJtSe7MuTLzObGcKf6T8obZT65jTFztmJ1LzY2xoOYYq7AUBn5CuUW/ANtR9N6HCaV+4B3lnOJWDs6Z8eRFQ5aUCSMXlBiGTEoPsimjVM5TafVGNZQSJoTFBRY98Z+/bQGksYta3ZyKsNA6UDiOW4q/yyaUzznWwYhMUzqFQhyqzoFOviDEL4nAFzs/wTaJ/bha5cyX/Z3nknfyQYrpKmDAG2Wx1k/Bl/hrvGguMb38IQgOS1zaFI/2ZrmIhy0w== 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=WjX0jnZ64gyfzio3ohE6WmoH3GAst3lKJiOyKCwJUEg=; b=THrNo7uEWsa9X60wmPXcwJuxsfIU2msiH/eT6IMyzvzlaSfjobjOUua3PQ99Yd55eGO2HzHiTPQpL5wIym1ilNapoffjIR81HlW4xiTGjWn6d0cv2iC5iWyntok9FJ+xRjKX6WqEE3ExD2RfO+CgcWmlcIQ1N+XyWfR7JiSpklIlNB73ia1IH2HzAKZBHZFeoaFT/b77ukyIf39SoARXAFS7lfhCs7wbWk274ovI4Ws5ADghcBE3KazqbkaJipfySSkK5monZISQrtHyPaSHGHrsu1jcR2E640Y05nUixAc1QW52S+NBcoDSmeo2K1aUU4/T7ApXus1KH74+4MvvWg== 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=WjX0jnZ64gyfzio3ohE6WmoH3GAst3lKJiOyKCwJUEg=; b=ladeq/mXc3rKZ327gDZYFyk3Jx0TkEhkvV9SJ9DCLOnR8xOgR0m1adO8TjjjtpoBmKreHtHFvAYdEfmiJ37VuxSSasi0N7D4vrvJMqOMTQxu7DKR2mgxUsni1cPCtnw5s9UyQtVbNQmPpOcAk5QyqtQbijChu8MwovxtzYc4SG8= Received: from BY5PR11MB4007.namprd11.prod.outlook.com (2603:10b6:a03:189::28) by BYAPR11MB3606.namprd11.prod.outlook.com (2603:10b6:a03:b5::25) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3391.19; Wed, 23 Sep 2020 03:40:59 +0000 Received: from BY5PR11MB4007.namprd11.prod.outlook.com ([fe80::1533:4053:1c45:3596]) by BY5PR11MB4007.namprd11.prod.outlook.com ([fe80::1533:4053:1c45:3596%6]) with mapi id 15.20.3370.033; Wed, 23 Sep 2020 03:40:59 +0000 From: "Ni, Ray" To: "Wu, Hao A" , "devel@edk2.groups.io" , "patrick.henz@hpe.com" 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: AQHWgUtL3Luis5ZjxE+rzlL6BtvAv6lWMB4AgAwR1ACABrpCAIAMt12w Date: Wed, 23 Sep 2020 03:40:59 +0000 Message-ID: References: In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: intel.com; dkim=none (message not signed) header.d=none;intel.com; dmarc=none action=none header.from=intel.com; x-originating-ip: [192.198.147.194] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: c5b2a29f-fc6f-4a93-a6c9-08d85f727cca x-ms-traffictypediagnostic: BYAPR11MB3606: 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: eGLd/kzIZZKvlpTNWKiqGokl8MmFGiLHnbBheMCNss0X3fyIug+v/bSLG0JFUXbl7HHUUStk7XJ54ycJnTmKteTkzL9dj6ATLrWxlB+3fAY2r6VR53dkk0H/SNUPnHxymnNmWl7q1uD4moht5wraoOy+SHKUptxW8lJ/LE5JH1zgH2NZNOZioigqyKkUF+cNsWVHt0hMJt//UFa7IEyvmEYBHtt9xb7cTj1OjxLheoKHX5EwwVM/X/gcDZGJyhU4DbFp8IGex74Q/ajvnhxw0qmFtp6fJEarkiENeDea0z7m/PZlnf7rtZ3ENrbtF93S6IuL02Uacp27WOitSuLQPscNfCHGDLy4ORBohqOMrveartJlpXJ2wTpATX8gXWTG+DXIhs99C9GNOjFimad/ug== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BY5PR11MB4007.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(376002)(396003)(39860400002)(346002)(136003)(366004)(8676002)(2906002)(52536014)(4326008)(966005)(66946007)(478600001)(86362001)(66476007)(83380400001)(33656002)(66556008)(64756008)(66446008)(76116006)(7696005)(53546011)(19627235002)(316002)(26005)(186003)(5660300002)(9686003)(8936002)(71200400001)(107886003)(55016002)(110136005)(6506007);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: WSfTJItpT+657usV1rrgfkyuCbnIRn+ZxPJ6cBNmhOpyzQvwvJDsXHXQgzsbVz5Dgc+7cDT83SmkQisHKthUO94b9WNzh+hHGoFJCfoiFRTTXtyr0LQDvpKNu7ZuRGHhnV4DMu9JZZHcjjSsS+Vqj2+ITvArc7BUDf97YyukD6mCQby7cey+1oZcYhUkaXmQ9J4Atj2RW044JKqt1V7dgImLlh3FYUm5qkxb0SeZO08lNqSYzcD0+rKhBaG1eBYcikzJrRHOsFBqbxyv5lOUEr02AzT/WCC+F1bXRFllIHy7Ygn6NQe3gndPwpLy3xuFdbAoiLhNhv7IXQDeqSKI1tKH8sD6FohxHGFlNOIlZ07BFMW40Qots9lryLgdu5aOCxBAWEf5nmHNZtIBEuEuerOWWHl3N2uOLJQh62fYFnJ+fGHP+P3+RVOQ/5AVdiChn8o17s8LBlHyH44Of3F2LXTsetvBx26tFgKpsKSD1FN1qQJX2HwCSyR6Wcd5OusCHh7MlyEi995WpoFJsKDXD48LP8wIZmq/8t/ufRQqsjF2FZRXUanHdJyNDpUX4uwgUsgXuedkmebY5jNN6F7WxWz0y1HsH9pGn3RTw2F8EZ7Vf62WJa9o/TKFr9iVfR7XU6TNb5msdrOUHlriqlOnNw== MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: BY5PR11MB4007.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: c5b2a29f-fc6f-4a93-a6c9-08d85f727cca X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Sep 2020 03:40:59.5792 (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: 8nhryIc74F8zVSeb7xikG5w0RYOKgvUkuq8xp82P0aUUwf+I8YxPV9G56JPq/RegMRE7HP90iDb91AtJcQnnEA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB3606 Return-Path: ray.ni@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hao, Yes the behavior should not be changed. Can we keep the original behavior while still not creating the timer when = infinite-loop is needed? Thanks, Ray > -----Original Message----- > From: Wu, Hao A > Sent: Tuesday, September 15, 2020 9:28 AM > 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 Broke= n > Timeouts >=20 > > -----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 > > > > Hi Hao and Ray, > > > > In regards to the behavior when Timeout =3D=3D 0, I did try to account= for that in > > my initial patch with the following logic: > > > > =09(0 =3D=3D > > Timeout)?(EFI_TIMER_PERIOD_MICROSECONDS(0xFFFFFFFF)):(EFI_TIMER_ > > PERIOD_MILLISECONDS(Timeout)) > > > > This results in a timeout that happens sooner than what the current co= de > > would produce, but it falls in line with what the original code seemed= to > > intend to do. Ray suggested that I enhance the code by not creating th= e > > timer event when Timeout is 0, which I assumed meant that XhcExecTrans= fer > > () would just return. I personally think it would be a good idea to ke= ep this > > behavior in the code, but would like Ray's input on the matter. Apprec= iate >=20 >=20 > I prefer to keep the origin behavior. > Ray, what is your comment on this? >=20 > Best Regards, > Hao Wu >=20 >=20 > > the help! > > > > Thanks, > > Patrick Henz > > > > -----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, Ra= y > > > > Cc: Wang, Jian J > > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix > > Broken Timeouts > > > > Hello Patrick, a couple of inline comments below. > > Hello Ray, need your input on one thing below as well. > > > > > > > -----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 Broke= n > > > 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 t= o > > > 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; > > > + } > > > > > > Could you help to refine the return status for the case when CreateEve= nt or > > SetTimer calls fail? > > I think it will return EFI_TIMEOUT at this moment, which might confuse= the > > caller. > > You may need to modify the function description comment section for th= e > > new return value also. > > > > A similar case applies to XhcExecTransfer() as well. > > > > > > > + > > > + 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; > > > > > > Ray and Patrick, the previous behavior when 'Timeout' is 0 for this fu= nction is > > that it will do a 'psudo-indefinite' loop by setting the 'Loop' variab= le to the > > value of MAX_UINT32. > > > > But after the patch, the behavior got changed and the function will di= rectly > > return EFI_TIMEOUT when 'Timeout' is 0. > > This behavior change might impact the callers when they expecting a 'p= sudo- > > indefinite' timeout. > > I think it would be better to keep the origin behavior, what is your t= hought? > > > > Best Regards, > > Hao Wu > > > > > > > + 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