Hello,
As I mentioned previously, if the code execution flow is:
UsbEnumeratePort
Status = HubApi->GetPortStatus (HubIf, Port, &PortState);
UsbRootHubGetPortStatus
UsbHcGetRootHubPortStatus
XhcGetRootHubPortStatus
Then the content in PortState will be initialized to 0 within
XhcGetRootHubPortStatus().
I think you need to double confirm what is the exact code execution flow in
your case and justify why the first patch is needed to resolve the issue you
met.
Could you help on this? Based on the current information I got during the
discussion, I have no idea on why the proposed change can resolve the issue.
Best Regards,
Hao Wu
From: Sean Rhodes <sean@starlabs.systems>
Sent: Friday, September 23, 2022 4:02 PM
To: Wu, Hao A <hao.a...@intel.com>
Cc: devel@edk2.groups.io; Ni, Ray <ray...@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing
on error path
Hi Hao
I retested, and it seems you are correct; the second patch has no effect.
Still, the first is required to resolve the issue -
https://github.com/tianocore/edk2/pull/3353
Maybe it is because enumeration happens with the port status not zero'd?
Thanks
Sean
On Thu, 22 Sept 2022 at 08:30, Wu, Hao A
<hao.a...@intel.com<mailto:hao.a...@intel.com>> wrote:
Hello,
1. For “"Avoid continuing on error path" - The reset can happen before
PortState is zero'd, so running this at the start of the function ensures it
will”:
My take is that the execution flow is like:
UsbEnumeratePort
Status = HubApi->GetPortStatus (HubIf, Port, &PortState);
UsbRootHubGetPortStatus
UsbHcGetRootHubPortStatus
XhcGetRootHubPortStatus
If this is the case, within XhcGetRootHubPortStatus
(MdeModulePkg\Bus\Pci\XhciDxe\Xhci.c), it does have logic to:
Offset = (UINT32)(XHC_PORTSC_OFFSET + (0x10 *
PortNumber));
PortStatus->PortStatus = 0;
PortStatus->PortChangeStatus = 0;
I am not sure why the below proposed change in UsbEnumeratePort can resolve the
issue:
// Zero out PortState in case GetPortStatus does not set it and we
// continue on the EFI_DEVICE_ERROR path
PortState.PortStatus = 0;
PortState.PortChangeStatus = 0;
Or the test you are performing includes connecting USB devices on a USB Hub?
2. For “" Reset the device on error" - this will only attempt enumeration if
there isn't an error.”:
As I mentioned in reply of that patch, by the code execution reaches the change
you made:
if (USB_BIT_IS_SET (PortState.PortChangeStatus, USB_PORT_STAT_C_RESET) &&
(Status != EFI_DEVICE_ERROR))
‘Status’ will always be EFI_SUCCESS, I do not understand what is the point of
adding “&& (Status != EFI_DEVICE_ERROR)”.
Could you help to double check if the change is really what you want to do?
Best Regards,
Hao Wu
From: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
<devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Sean Rhodes
Sent: Wednesday, September 21, 2022 4:37 PM
To: Wu, Hao A <hao.a...@intel.com<mailto:hao.a...@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray
<ray...@intel.com<mailto:ray...@intel.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing
on error path
Hi Hao
If I just boot a device to the EFI shell and then connect a USB, edk2 will
constantly try to access the device, fail and reset the port. It won't stop
doing this, and the USB drive can't be accessed.
I tested 13 USB drives; 8 saw this problem, and 5 worked correctly. It doesn't
matter which SOC, or if the port is USB 2.0 or 3.0.
" Reset the device on error" - this will only attempt enumeration if there
isn't an error.
"Avoid continuing on error path" - The reset can happen before PortState is
zero'd, so running this at the start of the function ensures it will
Hopefully, that makes sense!
Thanks
Sean
On Wed, 21 Sept 2022 at 09:24, Wu, Hao A
<hao.a...@intel.com<mailto:hao.a...@intel.com>> wrote:
Sorry, could you help to explain more on the endless loop? Like what causes the
loop and why the proposed change can resolve the issue.
I am not experienced enough in the USB domain to quickly figure out the whole
picture with only log being provided.
Best Regards,
Hao Wu
From: Sean Rhodes <sean@starlabs.systems<mailto:sean@starlabs.systems>>
Sent: Wednesday, September 21, 2022 4:01 PM
To: Wu, Hao A <hao.a...@intel.com<mailto:hao.a...@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray
<ray...@intel.com<mailto:ray...@intel.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing
on error path
Hi Hao
I've attached a debug log for the problem I'm trying to solve (these two
patches solve it).
The reset happens before PortState changes, so it's an endless loop.
Thanks
Sean
On Wed, 21 Sept 2022 at 06:31, Wu, Hao A
<hao.a...@intel.com<mailto:hao.a...@intel.com>> wrote:
Sorry, could you help to share more information on what issue is met for this
proposed patch?
It seems to me that the change made below is to handle the case where:
Status = HubApi->GetPortStatus (HubIf, Port, &PortState);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "UsbEnumeratePort: failed to get state of port %d\n",
Port));
return Status;
}
GetPortStatus() returns EFI_ SUCCESS, but does not update the output parameter
'PortState'.
Could you help to check what causes 'PortState' not being updated after a
success return from GetPortStatus()? Thanks in advance.
Also, one inline comment below:
> -----Original Message-----
> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Sean
> Rhodes
> Sent: Tuesday, September 20, 2022 9:14 PM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Rhodes, Sean <sean@starlabs.systems<mailto:sean@starlabs.systems>>; Wu,
> Hao A
> <hao.a...@intel.com<mailto:hao.a...@intel.com>>; Ni, Ray
> <ray...@intel.com<mailto:ray...@intel.com>>
> Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid
> continuing on error path
>
> Zero out the PortState in case GetPortStatus didn't set it, to avoid
> continuing with EFI_DEVICE_ERROR.
>
> Cc: Hao A Wu <hao.a...@intel.com<mailto:hao.a...@intel.com>>
> Cc: Ray Ni <ray...@intel.com<mailto:ray...@intel.com>>
> Signed-off-by: Sean Rhodes
> <sean@starlabs.systems<mailto:sean@starlabs.systems>>
> ---
> MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
> b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
> index aed34596f4..7fc567898a 100644
> --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
> +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
> @@ -900,6 +900,11 @@ UsbEnumeratePort (
> Child = NULL;
>
> HubApi = HubIf->HubApi;
>
>
>
> + // Zero out PortState in case GetPortStatus does not set it and we
>
> + // continue on the EFI_DEVICE_ERROR path
>
> + PortState.PortStatus = 0;
>
> + PortState.PortChangeStatus = 0;
Could you help to use:
ZeroMem (&PortState, sizeof (EFI_USB_PORT_STATUS));
here to align with other places in this driver?
Best Regards,
Hao Wu
>
> +
>
> //
>
> // Host learns of the new device by polling the hub for port changes.
>
> //
>
> --
> 2.34.1
>
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#93989): https://edk2.groups.io/g/devel/message/93989
> Mute This Topic: https://groups.io/mt/93802896/1768737
> Group Owner: devel+ow...@edk2.groups.io<mailto:devel%2bow...@edk2.groups.io>
> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [hao.a...@intel.com<mailto:hao.a...@intel.com>]
> -=-=-=-=-=-=
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#94206): https://edk2.groups.io/g/devel/message/94206
Mute This Topic: https://groups.io/mt/93802896/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
