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 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 * On Behalf Of *Sean > Rhodes > *Sent:* Wednesday, September 21, 2022 4:37 PM > *To:* Wu, Hao A > *Cc:* devel@edk2.groups.io; Ni, Ray > *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 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 > *Sent:* Wednesday, September 21, 2022 4:01 PM > *To:* Wu, Hao A > *Cc:* devel@edk2.groups.io; Ni, Ray > *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 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 On Behalf Of Sean > > Rhodes > > Sent: Tuesday, September 20, 2022 9:14 PM > > To: devel@edk2.groups.io > > Cc: Rhodes, Sean ; Wu, Hao A > > ; Ni, Ray > > 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 > > Cc: Ray Ni > > Signed-off-by: Sean Rhodes > > --- > > 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+owner@edk2.groups.io > > Unsubscribe: https://edk2.groups.io/g/devel/unsub [hao.a.wu@intel.com] > > -=-=-=-=-=-= > > > > >