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? 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 >
XhciSched.c.patch
Description: XhciSched.c.patch
------------------------------------------------------------------------------ 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
