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


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

Reply via email to