> -----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 > ------------------------------------------------------------------------------ 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
