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