From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0b-002e3701.pphosted.com (mx0b-002e3701.pphosted.com [148.163.143.35]) by mx.groups.io with SMTP id smtpd.web12.29053.1599763410855255536 for ; Thu, 10 Sep 2020 11:43:31 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@hpe.com header.s=pps0720 header.b=BKOBcsez; spf=pass (domain: hpe.com, ip: 148.163.143.35, mailfrom: prvs=0522c5130e=patrick.henz@hpe.com) Received: from pps.filterd (m0150244.ppops.net [127.0.0.1]) by mx0b-002e3701.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 08AIh2iH032551 for ; Thu, 10 Sep 2020 18:43:30 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=hpe.com; h=from : to : cc : subject : date : message-id : references : in-reply-to : content-type : content-transfer-encoding : mime-version; s=pps0720; bh=cX94G+ZyyUcwFUpx8ZhjlmSmptiS3HOmKG1lDVt4JQE=; b=BKOBcsezdyLvTVr7KTdwXHGmpT9nvQJPuf3Hi+dfKLCS8wMR5HPPacYSdg3cMrF1Q4LP a3JGY5tBIyEXnB6dOYMc6KqGkdtdXXpiZuaT+dRupW/ektkI+RHmU0mbQCaMFBv+6jnV xdHM2x9ljc4Lhwdsd0WEfOdTXqR7IcUWASIDNeqzOv+6whD66TLzPYiQ5z98ZK9NBwvU cghr/1GequNUR27Dc5v+lKbbwxABRYO4sFmPsaz583fdzhmKw4KeyKH53wrEynURj7mO PKCCZmXBEaRgwmhVWPYSuoxfVZi1efSHRFV6x/VaV4LA9e6x83e3OUqGJStUOZ9B/hTr YQ== Received: from g2t2353.austin.hpe.com (g2t2353.austin.hpe.com [15.233.44.26]) by mx0b-002e3701.pphosted.com with ESMTP id 33eu1qf6ub-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Thu, 10 Sep 2020 18:43:29 +0000 Received: from G9W8454.americas.hpqcorp.net (exchangepmrr1.us.hpecorp.net [16.216.161.4]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by g2t2353.austin.hpe.com (Postfix) with ESMTPS id 2BA4C96 for ; Thu, 10 Sep 2020 18:43:29 +0000 (UTC) Received: from G9W8456.americas.hpqcorp.net (2002:10d8:a15f::10d8:a15f) by G9W8454.americas.hpqcorp.net (2002:10d8:a104::10d8:a104) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Thu, 10 Sep 2020 18:43:28 +0000 Received: from NAM12-BN8-obe.outbound.protection.outlook.com (15.241.52.12) by G9W8456.americas.hpqcorp.net (16.216.161.95) with Microsoft SMTP Server (TLS) id 15.0.1497.2 via Frontend Transport; Thu, 10 Sep 2020 18:43:28 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Wobevmff05SuV5+YwS0UuDACYXL3m6N3mSNpiuZzqZmJUnC4UeVslcbctcwIt0X+8h0SN24z3ABJQNj/BQPcas+rfh7Ive5GJGgIuxHQrTRL9Vhhe5yCVTt/6xqWON2xr8bLbl05shTLEleK2SNFXaiN85WZzy89fClxgKxSO1b54JLZMuBpGWgTE+Zt2PErcHX9SVVH81AcXsGln/WIaCpI5ZCrCG3YJqGmAa7iHIlEWasVKRt8+j3ywy5WXf3J45LFp4Gy0KFch+E4NJghdLj01FG+GR+hFzzyV8hjGML6nf58m+Lw+ECNYM3uDzGu15OiLBDL8KaThjTIR9pCMQ== 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=kbV9MhtbTy30Wu19gA9RoiiL5J1iu2H/9U1ie2WNKzc=; b=Win9RLzHKjBV6PjdxI+QRo04vOg0XcWIxmCaOmBtxhEwNOH2YVPp94kj7WAbQbcmN+XCuuTgRk6VZ4ckr/eEbpq6N3bgPgjfsa2VCHTDojo8ezHV4sl4hZl7Zgp4UwEEKLYjNBLZKBGjBXaBikR/Mze8UIH3Q+mzeXqsaV3Tw2/jL4gCY0sh6nf6Ia6o0V9sm/eZxzmijDFZYzonx0GCY7ZwWAZEJrW31mioClU6J/YhMLdGuWE+KLmngfZl/AUfZps3pON3CfPUlpPlj1K0afWYjMk0iLQ5GF8bLQK/aklztTsj7v1iRsIN0N+T4agv5XJaIzO5rd4DmUVyr27V1A== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=hpe.com; dmarc=pass action=none header.from=hpe.com; dkim=pass header.d=hpe.com; arc=none Received: from TU4PR8401MB0478.NAMPRD84.PROD.OUTLOOK.COM (2a01:111:e400:7709::19) by TU4PR8401MB0399.NAMPRD84.PROD.OUTLOOK.COM (2a01:111:e400:770c::17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3370.16; Thu, 10 Sep 2020 18:43:27 +0000 Received: from TU4PR8401MB0478.NAMPRD84.PROD.OUTLOOK.COM ([fe80::4968:3a88:e4a6:5cea]) by TU4PR8401MB0478.NAMPRD84.PROD.OUTLOOK.COM ([fe80::4968:3a88:e4a6:5cea%8]) with mapi id 15.20.3370.016; Thu, 10 Sep 2020 18:43:27 +0000 From: "Henz, Patrick" To: "devel@edk2.groups.io" , "hao.a.wu@intel.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: AQHWgUsxWPIW6PxHA0eKlL+SoBhY46lWMB4AgAv0mxA= Date: Thu, 10 Sep 2020 18:43:26 +0000 Message-ID: References: In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: edk2.groups.io; dkim=none (message not signed) header.d=none;edk2.groups.io; dmarc=none action=none header.from=hpe.com; x-originating-ip: [76.17.188.158] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: 45b233a4-cfba-4c55-e60b-08d855b967b9 x-ms-traffictypediagnostic: TU4PR8401MB0399: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:3173; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: YWcONVP0aIsomocM/yzdRUtlht/8D1Jw55ki7SkX8B9Gs1IUlAYPajUJ143FgbXdDMtgf1qmqP4GyXz1fR1pSKCR9WKsUi+hGDK2gALYEkJTDEKTCDzYpX7KaayrwLlpN9n4O7uWUEArCZQ1R4kT46HfT/2uWLmhU+Va6UY8Qy2y82rwFSzzakGqx9ykVVtauoR40gPUY7Td+cYtsMpM4x+22N1fqs5Mp4lrQULsiXr0s/+G85BqFCxoWkeTvZcIcl2k/y6gl8VC1qfPxNIbgOnlRUhV6kFV6+AAqSMS2DuuuVaAtQrfG8FEnoGsfbZHI9iRQYaIK+UqivlYAYW9LPyXdTDHuHtlVB2Can6KmtcnbMrqT2KR5tGORyamvvXPMcgqC7KKvrKfAodIPcdBqQ== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:TU4PR8401MB0478.NAMPRD84.PROD.OUTLOOK.COM;PTR:;CAT:NONE;SFS:(396003)(366004)(39860400002)(136003)(376002)(346002)(8936002)(66556008)(66946007)(186003)(478600001)(33656002)(110136005)(8676002)(83380400001)(316002)(53546011)(7696005)(6506007)(2906002)(76116006)(66446008)(5660300002)(64756008)(66476007)(26005)(966005)(55016002)(86362001)(9686003)(52536014)(4326008)(71200400001);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: WXHcojQ2KMhfU6p3RHCjTIZ4+/71VXX3AmMKL7RUgnqj7zX1sXqtMAqihirf0SySX5ICY00sJllT2Ry6Iw6KgGKRWyFc8qFf1SCIDdhiW+Qv9PdD6xB/C9N20piT8trOfdbk3c0p81+mOf3ADuf/BDIJL7zi5p5El0YbtHb+ecp53rqNLMNomE4okE1AZ9WqTfdHAig4h0F1gvsagT0U9G17rGMoP2gbGVgPtQ/VxRsceDWovhQo5m059ExpfE6387Q78V2rpVgyxNi4y68cHooKQBPWvdQeY+YvGVN9cWRReXrRQ51/DONuKQqcyJecoEkpUAGPfn2asQPzfAvVZNV1N/WrXc778lwyjJBPnshD5aHkzupWG4ycH+rHGURe6C+XBk1mlWSGchOMk9GyYymbRj1qkAsMc4WoxBDdscklEggNFolOO1bBTIjRqew0VCFBfrFdUbyN8aMHQ3A5FAoYSbpusT8eevpm3/8HnczRES1yg1GUt0zDtpjQ0nGhUeCz5Gdt0aSMgrdAbrdFylxGFZUSBT/NyCJ2ExlSyl8e5VRlP2IgiCcS9cnFgZmrKV6u/sLpIgSCFOyJsMn84kOX+u7Z2RQ+z0/qjZ6d+lvIG3qq32PMHuDD9Jt2vZwoxTl2+8mjCnecYaisHXGI3w== x-ms-exchange-transport-forked: True X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: TU4PR8401MB0478.NAMPRD84.PROD.OUTLOOK.COM X-MS-Exchange-CrossTenant-Network-Message-Id: 45b233a4-cfba-4c55-e60b-08d855b967b9 X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Sep 2020 18:43:26.9092 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 105b2061-b669-4b31-92ac-24d304d195dc X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: TDt1jpulXFdc37O9TBhj3lOtuM5paFjc6mghndFfcRuKIrltdo27No6B1U8ZTSS18aKPa0CgXom2FKYdXWCKgQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: TU4PR8401MB0399 X-OriginatorOrg: hpe.com X-Proofpoint-UnRewURL: 3 URL's were un-rewritten MIME-Version: 1.0 X-HPE-SCL: -1 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.235,18.0.687 definitions=2020-09-10_08:2020-09-10,2020-09-10 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxscore=0 clxscore=1015 impostorscore=0 lowpriorityscore=0 adultscore=0 mlxlogscore=999 malwarescore=0 suspectscore=0 priorityscore=1501 spamscore=0 phishscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2006250000 definitions=main-2009100171 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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: (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 code w= ould produce, but it falls in line with what the original code seemed to in= tend to do. Ray suggested that I enhance the code by not creating the timer= event when Timeout is 0, which I assumed meant that XhcExecTransfer () wou= ld just return. I personally think it would be a good idea to keep this beh= avior in the code, but would like Ray's input on the matter. Appreciate 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, Ray 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=20 > patrick.henz@hpe.com > Sent: Thursday, September 3, 2020 1:05 AM > To: devel@edk2.groups.io > Cc: Patrick Henz ; Wang, Jian J=20 > ; Wu, Hao A ; Ni, Ray=20 > > Subject: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/XhciDxe: Fix Broken=20 > Timeouts >=20 > From: Patrick Henz >=20 > REF:INVALID URI REMOVED > ocore.org_show-5Fbug.cgi-3Fid-3D2948&d=3DDwIFAg&c=3DC5b8zRQO1miGmBeVZ2LF= Wg > &r=3Dwx4n0HbqxSAP19Eklmv6gq7ivDQlqQ_ITOkZIBUNNKg&m=3DOKlWpRL8ZyDfhUEh6S4= OU > aMasig0MPoajX7Vz2sDSvY&s=3DLTDbPsspkRCcWFBfThqhR_FaljF2kQLagB_t-kbAm80&e > =3D >=20 > Timeouts in the XhciDxe driver are taking longer than expected due to=20 > the timeout loops not accounting for code execution time. As en=20 > example, 5 second timeouts have been observed to take around 36=20 > seconds to complete. > Use SetTimer and Create/CheckEvent from Boot Services to determine=20 > when timeout occurred. >=20 > 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(-) >=20 > 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; >=20 > - Loop =3D Timeout * XHC_1_MILLISECOND; > + TimeoutEvent =3D NULL; >=20 > - 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 CreateEvent o= r 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 the ne= w 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; > } >=20 > gBS->Stall (XHC_1_MICROSECOND); > + } while (EFI_ERROR(gBS->CheckEvent (TimeoutEvent))); > + > +TIMEOUT: > + if (TimeoutEvent !=3D NULL) { > + gBS->CloseEvent (TimeoutEvent); > } >=20 > 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; > + } >=20 > if (CmdTransfer) { > SlotId =3D 0; > @@ -1291,29 +1299,46 @@ XhcExecTransfer ( > ASSERT (Dci < 32); > } >=20 > - 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 functi= on is that it will do a 'psudo-indefinite' loop by setting the 'Loop' varia= ble to the value of MAX_UINT32. But after the patch, the behavior got changed and the function will direct= ly return EFI_TIMEOUT when 'Timeout' is 0. This behavior change might impact the callers when they expecting a 'psudo= -indefinite' timeout. I think it would be better to keep the origin behavior, what is your thoug= ht? 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; > } >=20 > XhcRingDoorBell (Xhc, SlotId, Dci); >=20 > - 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))); >=20 > - 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; > } >=20 > + if (TimeoutEvent !=3D NULL) { > + gBS->CloseEvent (TimeoutEvent); > + } > + > return Status; > } >=20 > -- > 2.28.0 >=20 >=20 >=20