Baranee, I found XhciPei also have same issue and here are the fixes on these two modules.
If you are ok with my changes, could you sign Reviewed-by? Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Feng Tian <feng.t...@intel.com> Thanks Feng -----Original Message----- From: Anbazhagan, Baraneedharan [mailto:anbazha...@hp.com] Sent: Tuesday, July 7, 2015 10:09 AM To: Tian, Feng Cc: 'Eric Wittmayer'; 'edk2-devel@lists.sourceforge.net' Subject: RE: [edk2] XHCI question Do you have any other feedback on these code changes? If not, could you please commit those changes. Thanks. > > -----Original Message----- > > From: Tian, Feng [mailto:feng.t...@intel.com] > > Sent: Friday, June 26, 2015 1:28 AM > > To: Anbazhagan, Baraneedharan; Eric Wittmayer; > > edk2-devel@lists.sourceforge.net > > Cc: Tian, Feng > > Subject: RE: [edk2] XHCI question > > > > Thanks, Baranee > > > > I double checked code flow. > > > > When there is transaction error, the Urb->EvtTrb would not be > > updated and is NULL (see line 1165 of last revision in EDKII trunk > > XhciSched.c). Then XhcCmdTransfer() may return EFI_SUCCESS with > > EvtTrb == NULL. So I think it may bring exception at > > XhcCmdTransfer() caller with original code. > > > > But from your proposed fix, you just check if EvtTrb is NULL in > > XhcCheckUrbResult(). > > As the EvtTrb is calculated from XhcCheckNewEvent(), I didn't see > > any possibility of being NULL for EvtTrb. > > > > And as Eric's patch is incomplete (only update XhciBulkTransfer(), > > in fact other transfers have problem as well) I made a little > > adjustment on it to keep consistent with EhciDxe/UhciDxe. > > > > could you help review and test on your machine? > These changes are consistent with other controller modules and looks good to > me. > Didn't get a chance to verify with failing drive. Thanks. > > -Baranee > > > > > > Thanks > > Feng > > > > -----Original Message----- > > From: Anbazhagan, Baraneedharan [mailto:anbazha...@hp.com] > > Sent: Friday, June 26, 2015 01:51 > > To: Tian, Feng; Eric Wittmayer; edk2-devel@lists.sourceforge.net > > Subject: RE: [edk2] XHCI question > > > > > -----Original Message----- > > > From: Tian, Feng [mailto:feng.t...@intel.com] > > > Sent: Wednesday, June 24, 2015 11:02 PM > > > To: Anbazhagan, Baraneedharan; Eric Wittmayer; > > > edk2-devel@lists.sourceforge.net > > > Cc: Tian, Feng > > > Subject: RE: [edk2] XHCI question > > > > > > Hi, Baranee > > > > > > Which case do you think the EvtTrb would be NULL? > > In case of transaction error failure which doesn't return correct > > Status leads to issue next command without resetting the port and in > > that scenario we have seen that EvTrb was NULL. Perhaps if > > transaction error status is reported properly, it might not be > > needed. But in general it's better > to check for NULL before accessing the ptr. > > > > > > > > Thanks > > > Feng > > > > > > -----Original Message----- > > > From: Anbazhagan, Baraneedharan [mailto:anbazha...@hp.com] > > > Sent: Thursday, June 25, 2015 10:43 > > > To: Eric Wittmayer; Tian, Feng; edk2-devel@lists.sourceforge.net > > > Subject: RE: [edk2] XHCI question > > > > > > > > > > -----Original Message----- > > > > From: Eric Wittmayer [mailto:e...@frescologic.com] > > > > Sent: Wednesday, June 24, 2015 8:15 PM > > > > To: 'Tian, Feng'; edk2-devel@lists.sourceforge.net; Anbazhagan, > > > > Baraneedharan > > > > Subject: RE: [edk2] XHCI question > > > > > > > > Hi Feng, > > > > I see what Baranee is saying. > > > > The UEFI spec says in describing UsbBulkTransfer " If an error > > > > other than timeout occurs during the USB transfer, then > > > > EFI_DEVICE_ERROR is returned and the detailed status code will > > > > be returned in the Status > > parameter." > > > > > > > > I suggest that the following change would make XhcBulkTransfer > > > > the same as EhcBulkTransfer and would result in consistent > > > > behavior of UsbBulkTransfer on both EHCI and xHCI hardware. > > > > > > > Eric's code change will address the issue and also safe to check > > > for EvTrb != NULL before processing it. > > > > > > Index: XhciSched.c > > > > > > =================================================================== > > > --- XhciSched.c (revision 17702) > > > +++ XhciSched.c (working copy) > > > @@ -1074,7 +1074,11 @@ > > > // > > > goto EXIT; > > > } > > > - > > > + if(EvtTrb == NULL) > > > + { > > > + Status = EFI_DEVICE_ERROR; > > > + goto EXIT; > > > + } > > > // > > > // Only handle COMMAND_COMPLETETION_EVENT and TRANSFER_EVENT. > > > // > > > > > > > diff --git a/trunk/edk2/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c > > > > b/trunk/edk2/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c > > > > --- a/trunk/edk2/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c (revision 17702) > > > > +++ b/trunk/edk2/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c (working copy) > > > > @@ -1243,10 +1243,12 @@ > > > > > > > > if (*TransferResult == EFI_USB_NOERROR) { > > > > Status = EFI_SUCCESS; > > > > - } else if (*TransferResult == EFI_USB_ERR_STALL) { > > > > - RecoveryStatus = XhcRecoverHaltedEndpoint(Xhc, Urb); > > > > - if (EFI_ERROR (RecoveryStatus)) { > > > > - DEBUG ((EFI_D_ERROR, "XhcBulkTransfer: XhcRecoverHaltedEndpoint > > > > failed\n")); > > > > + } else { > > > > + if (*TransferResult == EFI_USB_ERR_STALL) { > > > > + RecoveryStatus = XhcRecoverHaltedEndpoint(Xhc, Urb); > > > > + if (EFI_ERROR (RecoveryStatus)) { > > > > + DEBUG ((EFI_D_ERROR, "XhcBulkTransfer: > > > > + XhcRecoverHaltedEndpoint > > > > failed\n")); > > > > + } > > > > } > > > > Status = EFI_DEVICE_ERROR; > > > > } > > > > > > > > Regards, > > > > Eric > > > > > > > > > -----Original Message----- > > > > > From: Tian, Feng [mailto:feng.t...@intel.com] > > > > > Sent: Wednesday, June 24, 2015 5:57 PM > > > > > To: edk2-devel@lists.sourceforge.net; Eric Wittmayer; > > > > > anbazha...@hp.com > > > > > Cc: Tian, Feng > > > > > Subject: RE: [edk2] XHCI question > > > > > > > > > > Hi, Baranee > > > > > > > > > > 1. XhcCheckUrbResult() reports URB transfer state in > > > > > Urb->Result rather > > > > than > > > > > Urb->Finished. > > > > > 2. XhcCheckUrbResult() is an internal function rather than an > > > > > exposed interface for upper layer. > > > > > 3. XhcBulkTransfer() returns status according to Urb->Result > > > > > rather than others. > > > > > > > > > > Please let me know what's the real issue you met. > > > > > > > > > > Thanks > > > > > Feng > > > > > > > > > > -----Original Message----- > > > > > From: Anbazhagan, Baraneedharan [mailto:anbazha...@hp.com] > > > > > Sent: Thursday, June 25, 2015 06:18 > > > > > To: Eric Wittmayer; edk2-devel@lists.sourceforge.net > > > > > Subject: Re: [edk2] XHCI question > > > > > > > > > > > > > > Whether XhcCheckUrbResult returns correct error status > > > > > > > > > in case of > > > > > > > failure? > > > > > > > > > XhcCheckUrbResult function header indicates it should > > > > > > > > > report URB transfer state - Urb->Finished (seems to > > > > > > > > > align with > > > > > > > > > EhciDxe) but the implementation is trying to return > > > > > > > > > EFI_STATUS and it doesn't get updated based on EvtTrb- > > > > > > > > > >Completecode. With a failing drive, XhcExecTransfer > > > > > > > > > >returns EFI_SUCCESS > > > > > > > > in > > > > > > > > > case of transaction error. > > > > > > > > > > > > > > > > > Looking at the source code for XhcCheckUrbResult, if you > > > > > > > > pass in a URB Pointer, the > > > > > > > > Urb->Result is updated with the completion code from the event. > > > > > > > > Look for the switch on EvtTrb->Completecode. In the > > > > > > > > case where Urb is passed in, CheckedUrb == Urb. > > > > > > > > EFI_STATUS that is returned indicates if the function > > > > > > > > actually checked any new events. > > > > > > > > > > > > > > > > Regards, > > > > > > > > Eric > > > > > > > > > > > > > > > If return value of XhcCheckUrbResult is a status of > > > > > > > whether it checked any new events or not, then > > > > > > > XhcBulkTransfer needs to be updated to return correct > > > > > > > failure status to the caller instead of checking for > > > > > > EFI_USB_ERR_STALL > > > > > > > and EFI_USB_NOERROR alone . > > > > > > > > > > > > > > > > > > > > > > > > > Error status is returned to the caller in TransferResult ( > > > > > > the last parameter to XhcBulkTransfer ). > > > > > > The check you are talking about is used to recover the > > > > > > endpoint to a running state if it stalled. > > > > > > Currently if there is a transaction error, the caller will > > > > > > find EFI_USB_ERR_TIMEOUT in TransferResult when > > > > > > XhcBulkTransfer > > > > > completes. > > > > > > What do you think should be done differently? > > > > > > > > > > > > Regards, > > > > > > Eric > > > > > > > > > > > > > > > > > > > > > > > It needs to be consistent between USB controller modules - Ehci/Xhci. > > > > > XHCI BulkTransfer returns success if event is checked and > > > > > caller is expected to > > > > look > > > > > for TransferResult. EHCI module passes error back to caller in Status. > > > > UsbBot > > > > > device driver checks for status on bulk transfer call and it > > > > > looks for TransferResult if status is not success. > > > > > > > > > > -Baranee > > > > > > > > > > > > > > > > > > > ---------------------------------------------------------------- > > > > -- > > > > -- > > > > -- > > > > ------ > > > > -- > > > > > Monitor 25 network devices or servers for free with OpManager! > > > > > OpManager is web-based network management software that > > > > > monitors network devices and physical & virtual servers, > > > > > alerts via email & sms for fault. Monitor 25 devices for free with no > > > > > restriction. > > > > > Download now > > > > > http://ad.doubleclick.net/ddm/clk/292181274;119417398;o > > > > > _______________________________________________ > > > > > edk2-devel mailing list > > > > > edk2-devel@lists.sourceforge.net > > > > > https://lists.sourceforge.net/lists/listinfo/edk2-devel > > > > >
0002-MdeModulePkg-XhciPei-Error-handling-enhancement-for-.patch
Description: 0002-MdeModulePkg-XhciPei-Error-handling-enhancement-for-.patch
0001-MdeModulePkg-XhciDxe-Error-handling-enhancement-for-.patch
Description: 0001-MdeModulePkg-XhciDxe-Error-handling-enhancement-for-.patch
------------------------------------------------------------------------------ Don't Limit Your Business. Reach for the Cloud. GigeNET's Cloud Solutions provide you with the tools and support that you need to offload your IT needs and focus on growing your business. Configured For All Businesses. Start Your Cloud Today. https://www.gigenetcloud.com/
_______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
