Discussion:
[edk2] XHCI question
Anbazhagan, Baraneedharan
2015-06-23 23:16:05 UTC
Permalink
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.

-Baranee
Eric Wittmayer
2015-06-24 05:12:43 UTC
Permalink
Date: Tue, 23 Jun 2015 23:16:05 +0000
Subject: [edk2] XHCI question
as.hpqcorp.net>
Content-Type: text/plain; charset="us-ascii"
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-
Post by Anbazhagan, Baraneedharan
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
Anbazhagan, Baraneedharan
2015-06-24 21:10:35 UTC
Permalink
-----Original Message-----
Sent: Wednesday, June 24, 2015 12:13 AM
Subject: RE: [edk2] XHCI question
Date: Tue, 23 Jun 2015 23:16:05 +0000
Subject: [edk2] XHCI question
as.hpqcorp.net>
Content-Type: text/plain; charset="us-ascii"
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-
Post by Anbazhagan, Baraneedharan
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 .
Eric Wittmayer
2015-06-24 22:03:37 UTC
Permalink
-----Original Message-----
Sent: Wednesday, June 24, 2015 2:11 PM
Subject: RE: [edk2] XHCI question
-----Original Message-----
Sent: Wednesday, June 24, 2015 12:13 AM
Subject: RE: [edk2] XHCI question
Date: Tue, 23 Jun 2015 23:16:05 +0000
Subject: [edk2] XHCI question
as.hpqcorp.net>
Content-Type: text/plain; charset="us-ascii"
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-
Post by Anbazhagan, Baraneedharan
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
Anbazhagan, Baraneedharan
2015-06-24 22:17:37 UTC
Permalink
Post by Anbazhagan, Baraneedharan
Post by Anbazhagan, Baraneedharan
Post by Anbazhagan, Baraneedharan
Whether XhcCheckUrbResult returns correct error status in case of
failure?
Post by Anbazhagan, Baraneedharan
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-
Post by Anbazhagan, Baraneedharan
Completecode. With a failing drive, XhcExecTransfer returns EFI_SUCCESS
in
Post by Anbazhagan, Baraneedharan
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
Post by Anbazhagan, Baraneedharan
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
Tian, Feng
2015-06-25 00:57:28 UTC
Permalink
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:***@hp.com]
Sent: Thursday, June 25, 2015 06:18
To: Eric Wittmayer; edk2-***@lists.sourceforge.net
Subject: Re: [edk2] XHCI question
Post by Anbazhagan, Baraneedharan
Post by Anbazhagan, Baraneedharan
Post by Anbazhagan, Baraneedharan
Whether XhcCheckUrbResult returns correct error status in case of
failure?
Post by Anbazhagan, Baraneedharan
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-
Post by Anbazhagan, Baraneedharan
Completecode. With a failing drive, XhcExecTransfer returns EFI_SUCCESS
in
Post by Anbazhagan, Baraneedharan
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
Post by Anbazhagan, Baraneedharan
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-***@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
Eric Wittmayer
2015-06-25 01:14:47 UTC
Permalink
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.

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-----
Sent: Wednesday, June 24, 2015 5:57 PM
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-----
Sent: Thursday, June 25, 2015 06:18
Subject: Re: [edk2] XHCI question
Post by Anbazhagan, Baraneedharan
Post by Anbazhagan, Baraneedharan
Post by Eric Wittmayer
Post by Anbazhagan, Baraneedharan
Whether XhcCheckUrbResult returns correct error status in case of
failure?
Post by Eric Wittmayer
Post by Anbazhagan, Baraneedharan
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-
Post by Anbazhagan, Baraneedharan
Completecode. With a failing drive, XhcExecTransfer returns EFI_SUCCESS
in
Post by Anbazhagan, Baraneedharan
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
Post by Anbazhagan, Baraneedharan
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.
Post by Anbazhagan, Baraneedharan
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
https://lists.sourceforge.net/lists/listinfo/edk2-devel
Anbazhagan, Baraneedharan
2015-06-25 02:43:28 UTC
Permalink
-----Original Message-----
Sent: Wednesday, June 24, 2015 8:15 PM
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-----
Sent: Wednesday, June 24, 2015 5:57 PM
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-----
Sent: Thursday, June 25, 2015 06:18
Subject: Re: [edk2] XHCI question
Post by Anbazhagan, Baraneedharan
Post by Anbazhagan, Baraneedharan
Post by Eric Wittmayer
Post by Anbazhagan, Baraneedharan
Whether XhcCheckUrbResult returns correct error status in case of
failure?
Post by Eric Wittmayer
Post by Anbazhagan, Baraneedharan
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-
Post by Anbazhagan, Baraneedharan
Completecode. With a failing drive, XhcExecTransfer returns
EFI_SUCCESS
in
Post by Anbazhagan, Baraneedharan
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
Post by Anbazhagan, Baraneedharan
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.
Post by Anbazhagan, Baraneedharan
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
https://lists.sourceforge.net/lists/listinfo/edk2-devel
Tian, Feng
2015-06-25 04:02:14 UTC
Permalink
Hi, Baranee

Which case do you think the EvtTrb would be NULL?

Thanks
Feng

-----Original Message-----
From: Anbazhagan, Baraneedharan [mailto:***@hp.com]
Sent: Thursday, June 25, 2015 10:43
To: Eric Wittmayer; Tian, Feng; edk2-***@lists.sourceforge.net
Subject: RE: [edk2] XHCI question
-----Original Message-----
Sent: Wednesday, June 24, 2015 8:15 PM
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)) {
+ XhcRecoverHaltedEndpoint
failed\n"));
+ }
}
Status = EFI_DEVICE_ERROR;
}
Regards,
Eric
-----Original Message-----
Sent: Wednesday, June 24, 2015 5:57 PM
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-----
Sent: Thursday, June 25, 2015 06:18
Subject: Re: [edk2] XHCI question
Post by Anbazhagan, Baraneedharan
Post by Anbazhagan, Baraneedharan
Post by Eric Wittmayer
Post by Anbazhagan, Baraneedharan
Whether XhcCheckUrbResult returns correct error status in case of
failure?
Post by Eric Wittmayer
Post by Anbazhagan, Baraneedharan
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-
Post by Anbazhagan, Baraneedharan
Completecode. With a failing drive, XhcExecTransfer returns
EFI_SUCCESS
in
Post by Anbazhagan, Baraneedharan
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
Post by Anbazhagan, Baraneedharan
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.
Post by Anbazhagan, Baraneedharan
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
https://lists.sourceforge.net/lists/listinfo/edk2-devel
Anbazhagan, Baraneedharan
2015-06-25 17:51:28 UTC
Permalink
-----Original Message-----
Sent: Wednesday, June 24, 2015 11:02 PM
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-----
Sent: Thursday, June 25, 2015 10:43
Subject: RE: [edk2] XHCI question
-----Original Message-----
Sent: Wednesday, June 24, 2015 8:15 PM
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)) {
+ XhcRecoverHaltedEndpoint
failed\n"));
+ }
}
Status = EFI_DEVICE_ERROR;
}
Regards,
Eric
-----Original Message-----
Sent: Wednesday, June 24, 2015 5:57 PM
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-----
Sent: Thursday, June 25, 2015 06:18
Subject: Re: [edk2] XHCI question
Post by Anbazhagan, Baraneedharan
Post by Anbazhagan, Baraneedharan
Post by Eric Wittmayer
Post by Anbazhagan, Baraneedharan
Whether XhcCheckUrbResult returns correct error status in case of
failure?
Post by Eric Wittmayer
Post by Anbazhagan, Baraneedharan
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-
Post by Anbazhagan, Baraneedharan
Completecode. With a failing drive, XhcExecTransfer returns
EFI_SUCCESS
in
Post by Anbazhagan, Baraneedharan
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
Post by Anbazhagan, Baraneedharan
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.
Post by Anbazhagan, Baraneedharan
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
https://lists.sourceforge.net/lists/listinfo/edk2-devel
Tian, Feng
2015-06-26 06:28:05 UTC
Permalink
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:***@hp.com]
Sent: Friday, June 26, 2015 01:51
To: Tian, Feng; Eric Wittmayer; edk2-***@lists.sourceforge.net
Subject: RE: [edk2] XHCI question
-----Original Message-----
Sent: Wednesday, June 24, 2015 11:02 PM
To: Anbazhagan, Baraneedharan; Eric Wittmayer;
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-----
Sent: Thursday, June 25, 2015 10:43
Subject: RE: [edk2] XHCI question
-----Original Message-----
Sent: Wednesday, June 24, 2015 8:15 PM
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)) {
+ XhcRecoverHaltedEndpoint
failed\n"));
+ }
}
Status = EFI_DEVICE_ERROR;
}
Regards,
Eric
-----Original Message-----
Sent: Wednesday, June 24, 2015 5:57 PM
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-----
Sent: Thursday, June 25, 2015 06:18
Subject: Re: [edk2] XHCI question
Post by Anbazhagan, Baraneedharan
Post by Anbazhagan, Baraneedharan
Post by Eric Wittmayer
Post by Anbazhagan, Baraneedharan
Whether XhcCheckUrbResult returns correct error status in case of
failure?
Post by Eric Wittmayer
Post by Anbazhagan, Baraneedharan
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-
Post by Anbazhagan, Baraneedharan
Completecode. With a failing drive, XhcExecTransfer
returns EFI_SUCCESS
in
Post by Anbazhagan, Baraneedharan
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
Post by Anbazhagan, Baraneedharan
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.
Post by Anbazhagan, Baraneedharan
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
https://lists.sourceforge.net/lists/listinfo/edk2-devel
Anbazhagan, Baraneedharan
2015-06-26 23:20:42 UTC
Permalink
-----Original Message-----
Sent: Friday, June 26, 2015 1:28 AM
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-----
Sent: Friday, June 26, 2015 01:51
Subject: RE: [edk2] XHCI question
-----Original Message-----
Sent: Wednesday, June 24, 2015 11:02 PM
To: Anbazhagan, Baraneedharan; Eric Wittmayer;
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-----
Sent: Thursday, June 25, 2015 10:43
Subject: RE: [edk2] XHCI question
-----Original Message-----
Sent: Wednesday, June 24, 2015 8:15 PM
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)) {
+ XhcRecoverHaltedEndpoint
failed\n"));
+ }
}
Status = EFI_DEVICE_ERROR;
}
Regards,
Eric
-----Original Message-----
Sent: Wednesday, June 24, 2015 5:57 PM
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-----
Sent: Thursday, June 25, 2015 06:18
Subject: Re: [edk2] XHCI question
Post by Anbazhagan, Baraneedharan
Post by Anbazhagan, Baraneedharan
Post by Eric Wittmayer
Post by Anbazhagan, Baraneedharan
Whether XhcCheckUrbResult returns correct error status in
case of
failure?
Post by Eric Wittmayer
Post by Anbazhagan, Baraneedharan
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-
Post by Anbazhagan, Baraneedharan
Completecode. With a failing drive, XhcExecTransfer
returns EFI_SUCCESS
in
Post by Anbazhagan, Baraneedharan
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
Post by Anbazhagan, Baraneedharan
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.
Post by Anbazhagan, Baraneedharan
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
https://lists.sourceforge.net/lists/listinfo/edk2-devel
Anbazhagan, Baraneedharan
2015-07-07 02:08:53 UTC
Permalink
Do you have any other feedback on these code changes? If not, could you please commit those changes. Thanks.
Post by Anbazhagan, Baraneedharan
-----Original Message-----
Sent: Friday, June 26, 2015 1:28 AM
To: Anbazhagan, Baraneedharan; Eric Wittmayer;
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-----
Sent: Friday, June 26, 2015 01:51
Subject: RE: [edk2] XHCI question
-----Original Message-----
Sent: Wednesday, June 24, 2015 11:02 PM
To: Anbazhagan, Baraneedharan; Eric Wittmayer;
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-----
Sent: Thursday, June 25, 2015 10:43
Subject: RE: [edk2] XHCI question
-----Original Message-----
Sent: Wednesday, June 24, 2015 8:15 PM
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)) {
+ XhcRecoverHaltedEndpoint
failed\n"));
+ }
}
Status = EFI_DEVICE_ERROR;
}
Regards,
Eric
-----Original Message-----
Sent: Wednesday, June 24, 2015 5:57 PM
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-----
Sent: Thursday, June 25, 2015 06:18
Subject: Re: [edk2] XHCI question
Post by Anbazhagan, Baraneedharan
Post by Anbazhagan, Baraneedharan
Post by Eric Wittmayer
Post by Anbazhagan, Baraneedharan
Whether XhcCheckUrbResult returns correct error status
in case of
failure?
Post by Eric Wittmayer
Post by Anbazhagan, Baraneedharan
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-
Post by Anbazhagan, Baraneedharan
Completecode. With a failing drive, XhcExecTransfer
returns EFI_SUCCESS
in
Post by Anbazhagan, Baraneedharan
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
Post by Anbazhagan, Baraneedharan
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.
Post by Anbazhagan, Baraneedharan
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
https://lists.sourceforge.net/lists/listinfo/edk2-devel
Tian, Feng
2015-07-07 02:43:52 UTC
Permalink
Baranee,

I found XhciPei also have same issue and here are the fixes on these two modules.

If you are ok with my changes, could you sign Reviewed-by?

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Feng Tian <***@intel.com>

Thanks
Feng

-----Original Message-----
From: Anbazhagan, Baraneedharan [mailto:***@hp.com]
Sent: Tuesday, July 7, 2015 10:09 AM
To: Tian, Feng
Cc: 'Eric Wittmayer'; 'edk2-***@lists.sourceforge.net'
Subject: RE: [edk2] XHCI question

Do you have any other feedback on these code changes? If not, could you please commit those changes. Thanks.
Post by Anbazhagan, Baraneedharan
-----Original Message-----
Sent: Friday, June 26, 2015 1:28 AM
To: Anbazhagan, Baraneedharan; Eric Wittmayer;
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-----
Sent: Friday, June 26, 2015 01:51
Subject: RE: [edk2] XHCI question
-----Original Message-----
Sent: Wednesday, June 24, 2015 11:02 PM
To: Anbazhagan, Baraneedharan; Eric Wittmayer;
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-----
Sent: Thursday, June 25, 2015 10:43
Subject: RE: [edk2] XHCI question
-----Original Message-----
Sent: Wednesday, June 24, 2015 8:15 PM
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)) {
+ XhcRecoverHaltedEndpoint
failed\n"));
+ }
}
Status = EFI_DEVICE_ERROR;
}
Regards,
Eric
-----Original Message-----
Sent: Wednesday, June 24, 2015 5:57 PM
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-----
Sent: Thursday, June 25, 2015 06:18
Subject: Re: [edk2] XHCI question
Post by Anbazhagan, Baraneedharan
Post by Anbazhagan, Baraneedharan
Post by Eric Wittmayer
Post by Anbazhagan, Baraneedharan
Whether XhcCheckUrbResult returns correct error status
in case of
failure?
Post by Eric Wittmayer
Post by Anbazhagan, Baraneedharan
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-
Post by Anbazhagan, Baraneedharan
Completecode. With a failing drive, XhcExecTransfer
returns EFI_SUCCESS
in
Post by Anbazhagan, Baraneedharan
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
Post by Anbazhagan, Baraneedharan
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.
Post by Anbazhagan, Baraneedharan
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
https://lists.sourceforge.net/lists/listinfo/edk2-devel
Anbazhagan, Baraneedharan
2015-07-07 13:52:01 UTC
Permalink
Thanks for fixing it in PEI module as well.
-----Original Message-----
Sent: Monday, July 06, 2015 9:44 PM
To: Anbazhagan, Baraneedharan
Subject: RE: [edk2] XHCI question
Baranee,
I found XhciPei also have same issue and here are the fixes on these two modules.
If you are ok with my changes, could you sign Reviewed-by?
Contributed-under: TianoCore Contribution Agreement 1.0
Thanks
Feng
-----Original Message-----
Sent: Tuesday, July 7, 2015 10:09 AM
To: Tian, Feng
Subject: RE: [edk2] XHCI question
Do you have any other feedback on these code changes? If not, could you please
commit those changes. Thanks.
Post by Anbazhagan, Baraneedharan
-----Original Message-----
Sent: Friday, June 26, 2015 1:28 AM
To: Anbazhagan, Baraneedharan; Eric Wittmayer;
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().
Post by Anbazhagan, Baraneedharan
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-----
Sent: Friday, June 26, 2015 01:51
Subject: RE: [edk2] XHCI question
-----Original Message-----
Sent: Wednesday, June 24, 2015 11:02 PM
To: Anbazhagan, Baraneedharan; Eric Wittmayer;
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-----
Sent: Thursday, June 25, 2015 10:43
Subject: RE: [edk2] XHCI question
-----Original Message-----
Sent: Wednesday, June 24, 2015 8:15 PM
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
===================================================================
Post by Anbazhagan, Baraneedharan
--- 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)) {
+ XhcRecoverHaltedEndpoint
failed\n"));
+ }
}
Status = EFI_DEVICE_ERROR;
}
Regards,
Eric
-----Original Message-----
Sent: Wednesday, June 24, 2015 5:57 PM
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-----
Sent: Thursday, June 25, 2015 06:18
Subject: Re: [edk2] XHCI question
Post by Anbazhagan, Baraneedharan
Post by Anbazhagan, Baraneedharan
Post by Eric Wittmayer
Post by Anbazhagan, Baraneedharan
Whether XhcCheckUrbResult returns correct error status
in case of
failure?
Post by Eric Wittmayer
Post by Anbazhagan, Baraneedharan
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-
Post by Anbazhagan, Baraneedharan
Completecode. With a failing drive, XhcExecTransfer
returns EFI_SUCCESS
in
Post by Anbazhagan, Baraneedharan
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
Post by Anbazhagan, Baraneedharan
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.
Post by Anbazhagan, Baraneedharan
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.
Post by Anbazhagan, Baraneedharan
Download now
http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel
Loading...