Hi, Baranee Which case do you think the EvtTrb would be NULL?
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 ------------------------------------------------------------------------------ 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
