* [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing on error path @ 2022-09-20 13:14 Sean Rhodes 2022-09-20 13:14 ` [PATCH 2/2] MdeModulePkg/UsbBusDxe: Reset the device on error Sean Rhodes 2022-09-21 5:30 ` [edk2-devel] [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing on error path Wu, Hao A 0 siblings, 2 replies; 10+ messages in thread From: Sean Rhodes @ 2022-09-20 13:14 UTC (permalink / raw) To: devel; +Cc: Sean Rhodes, Hao A Wu, Ray Ni Zero out the PortState in case GetPortStatus didn't set it, to avoid continuing with EFI_DEVICE_ERROR. Cc: Hao A Wu <hao.a.wu@intel.com> Cc: Ray Ni <ray.ni@intel.com> Signed-off-by: Sean Rhodes <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; + // // Host learns of the new device by polling the hub for port changes. // -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] MdeModulePkg/UsbBusDxe: Reset the device on error 2022-09-20 13:14 [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing on error path Sean Rhodes @ 2022-09-20 13:14 ` Sean Rhodes 2022-09-21 5:31 ` [edk2-devel] " Wu, Hao A 2022-09-21 5:30 ` [edk2-devel] [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing on error path Wu, Hao A 1 sibling, 1 reply; 10+ messages in thread From: Sean Rhodes @ 2022-09-20 13:14 UTC (permalink / raw) To: devel; +Cc: Sean Rhodes, Hao A Wu, Ray Ni Try a port reset if GetPortStatus returns and error. Cc: Hao A Wu <hao.a.wu@intel.com> Cc: Ray Ni <ray.ni@intel.com> Signed-off-by: Sean Rhodes <sean@starlabs.systems> --- MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c index 7fc567898a..13112be2a5 100644 --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c @@ -992,7 +992,9 @@ UsbEnumeratePort ( // Now, new device connected, enumerate and configure the device // DEBUG ((DEBUG_INFO, "UsbEnumeratePort: new device connected at port %d\n", Port)); - if (USB_BIT_IS_SET (PortState.PortChangeStatus, USB_PORT_STAT_C_RESET)) { + if (USB_BIT_IS_SET (PortState.PortChangeStatus, USB_PORT_STAT_C_RESET) && + (Status != EFI_DEVICE_ERROR)) + { Status = UsbEnumerateNewDev (HubIf, Port, FALSE); } else { Status = UsbEnumerateNewDev (HubIf, Port, TRUE); -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/UsbBusDxe: Reset the device on error 2022-09-20 13:14 ` [PATCH 2/2] MdeModulePkg/UsbBusDxe: Reset the device on error Sean Rhodes @ 2022-09-21 5:31 ` Wu, Hao A 0 siblings, 0 replies; 10+ messages in thread From: Wu, Hao A @ 2022-09-21 5:31 UTC (permalink / raw) To: devel@edk2.groups.io, Rhodes, Sean; +Cc: Ni, Ray > -----Original Message----- > From: devel@edk2.groups.io <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 <sean@starlabs.systems>; Wu, Hao A > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com> > Subject: [edk2-devel] [PATCH 2/2] MdeModulePkg/UsbBusDxe: Reset the > device on error > > Try a port reset if GetPortStatus returns and error. > > Cc: Hao A Wu <hao.a.wu@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Signed-off-by: Sean Rhodes <sean@starlabs.systems> > --- > MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c > b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c > index 7fc567898a..13112be2a5 100644 > --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c > +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c > @@ -992,7 +992,9 @@ UsbEnumeratePort ( > // Now, new device connected, enumerate and configure the device > > // > > DEBUG ((DEBUG_INFO, "UsbEnumeratePort: new device connected at > port %d\n", Port)); > > - if (USB_BIT_IS_SET (PortState.PortChangeStatus, > USB_PORT_STAT_C_RESET)) { > > + if (USB_BIT_IS_SET (PortState.PortChangeStatus, > USB_PORT_STAT_C_RESET) && > > + (Status != EFI_DEVICE_ERROR)) Sorry, I do not understand the purpose of the patch. Looking into the current implementation of function UsbEnumeratePort(), if the execution flow reaches here, the local variable 'Status' must have a value of EFI_SUCCESS. My take is that this check: 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; } will make the function return if the value of 'Status' does not equals to EFI_SUCCESS. Did I miss something for the above understanding? Best Regards, Hao Wu > > + { > > Status = UsbEnumerateNewDev (HubIf, Port, FALSE); > > } else { > > Status = UsbEnumerateNewDev (HubIf, Port, TRUE); > > -- > 2.34.1 > > > > -=-=-=-=-=-= > Groups.io Links: You receive all messages sent to this group. > View/Reply Online (#93990): https://edk2.groups.io/g/devel/message/93990 > Mute This Topic: https://groups.io/mt/93802899/1768737 > Group Owner: devel+owner@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub [hao.a.wu@intel.com] > -=-=-=-=-=-= > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing on error path 2022-09-20 13:14 [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing on error path Sean Rhodes 2022-09-20 13:14 ` [PATCH 2/2] MdeModulePkg/UsbBusDxe: Reset the device on error Sean Rhodes @ 2022-09-21 5:30 ` Wu, Hao A 2022-09-21 8:00 ` Sean Rhodes 1 sibling, 1 reply; 10+ messages in thread From: Wu, Hao A @ 2022-09-21 5:30 UTC (permalink / raw) To: devel@edk2.groups.io, Rhodes, Sean; +Cc: Ni, Ray 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 <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 <sean@starlabs.systems>; Wu, Hao A > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@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.wu@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Signed-off-by: Sean Rhodes <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+owner@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub [hao.a.wu@intel.com] > -=-=-=-=-=-= > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing on error path 2022-09-21 5:30 ` [edk2-devel] [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing on error path Wu, Hao A @ 2022-09-21 8:00 ` Sean Rhodes 2022-09-21 8:24 ` Wu, Hao A 0 siblings, 1 reply; 10+ messages in thread From: Sean Rhodes @ 2022-09-21 8:00 UTC (permalink / raw) To: Wu, Hao A; +Cc: devel@edk2.groups.io, Ni, Ray [-- Attachment #1.1: Type: text/plain, Size: 2930 bytes --] 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.wu@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 <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 <sean@starlabs.systems>; Wu, Hao A > > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@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.wu@intel.com> > > Cc: Ray Ni <ray.ni@intel.com> > > Signed-off-by: Sean Rhodes <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+owner@edk2.groups.io > > Unsubscribe: https://edk2.groups.io/g/devel/unsub [hao.a.wu@intel.com] > > -=-=-=-=-=-= > > > > [-- Attachment #1.2: Type: text/html, Size: 5298 bytes --] [-- Attachment #2: edk2-usb.txt --] [-- Type: text/plain, Size: 27540 bytes --] map: No mapping found. Press ESC in 1 seconds to skip startup.nsh or any other key to continue. Shell> XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 5 state - 801, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 5) UsbEnumeratePort: new device connected at port 5 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 5 is reset UsbEnumerateNewDev: No device present at port 5 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 803, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: new device connected at port 13 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success Enable Slot Successfully, The Slot ID = 0x4 Address 4 assigned successfully UsbEnumerateNewDev: hub port 13 is reset UsbEnumerateNewDev: device is of 3 speed UsbEnumerateNewDev: device uses translator (0, 0) UsbEnumerateNewDev: device is now ADDRESSED at 4 UsbEnumerateNewDev: max packet size for EP 0 is 512 Evaluate context UsbBuildDescTable: device has 1 configures UsbGetOneConfig: total length is 44 UsbParseConfigDesc: config 1 has 1 interfaces UsbParseInterfaceDesc: interface 0(setting 0) has 2 endpoints Endpoint[81]: Created BULK ring [EB25280~EB26280) Endpoint[2]: Created BULK ring [EB26280~EB27280) Configure Endpoint UsbEnumerateNewDev: device 4 is now in CONFIGED state UsbSelectConfig: config 1 selected for device 4 UsbSelectSetting: setting 0 selected for interface 0 InstallProtocolInterface: 09576E91-6D3F-11D2-8E39-00A0C969723B E883018 InstallProtocolInterface: 2B2F68D6-0CD2-44CF-8E8B-BBA20B1B5B75 E884D40 UsbConnectDriver: TPL before connect is 8, E883F18 UsbMassInitMedia: UsbBootGetParams (Media changed) InstallProtocolInterface: 964E5B21-6459-11D2-8E39-00A0C969723B E8839B8 InstallProtocolInterface: D432A67F-14DC-484B-B3BB-3F0291849327 E883A30 InstallProtocolInterface: CE345171-BA0B-11D2-8E4F-00A0C969723B E883720 BlockSize : 512 LastBlock : EC3FFF InstallProtocolInterface: 09576E91-6D3F-11D2-8E39-00A0C969723B E87AE18 InstallProtocolInterface: 964E5B21-6459-11D2-8E39-00A0C969723B E879030 InstallProtocolInterface: 8CF2F62C-BC9B-4821-808D-EC9EC421A1A0 E8790E8 InstallProtocolInterface: 09576E91-6D3F-11D2-8E39-00A0C969723B E87AF18 InstallProtocolInterface: 964E5B21-6459-11D2-8E39-00A0C969723B E879D30 InstallProtocolInterface: 8CF2F62C-BC9B-4821-808D-EC9EC421A1A0 E879DE8 PartitionDxe: El Torito standard found on handle 0xE883F18. InstallProtocolInterface: CE345171-BA0B-11D2-8E4F-00A0C969723B E8792A0 BlockSize : 512 LastBlock : F InstallProtocolInterface: CE345171-BA0B-11D2-8E4F-00A0C969723B E87A020 BlockSize : 512 LastBlock : 7FFF InstallProtocolInterface: 964E5B22-6459-11D2-8E39-00A0C969723B E868030 Installed Fat filesystem on E87AB18 UsbConnectDriver: TPL after connect is 8 PROGRESS CODE: V02020006 I0 XhcClearRootHubPortFeature: status Success Disable device slot 4! UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device at port 13 removed from root hub EB08C98 UsbDisconnectDriver: old TPL is 8, E883F18 Success to stop non-multi-lun root handle UsbDisconnectDriver: TPL after disconnect is 8, 0 UsbRemoveDevice: device 4 removed UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 803, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: new device connected at port 13 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success Enable Slot Successfully, The Slot ID = 0x5 Address 5 assigned successfully UsbEnumerateNewDev: hub port 13 is reset UsbEnumerateNewDev: device is of 3 speed UsbEnumerateNewDev: device uses translator (0, 0) UsbEnumerateNewDev: device is now ADDRESSED at 4 UsbEnumerateNewDev: max packet size for EP 0 is 512 Evaluate context UsbBuildDescTable: device has 1 configures UsbGetOneConfig: total length is 44 UsbParseConfigDesc: config 1 has 1 interfaces UsbParseInterfaceDesc: interface 0(setting 0) has 2 endpoints Endpoint[81]: Created BULK ring [EB25280~EB26280) Endpoint[2]: Created BULK ring [EB26280~EB27280) Configure Endpoint UsbEnumerateNewDev: device 4 is now in CONFIGED state UsbSelectConfig: config 1 selected for device 4 UsbSelectSetting: setting 0 selected for interface 0 InstallProtocolInterface: 09576E91-6D3F-11D2-8E39-00A0C969723B E883E98 InstallProtocolInterface: 2B2F68D6-0CD2-44CF-8E8B-BBA20B1B5B75 E884D40 UsbConnectDriver: TPL before connect is 8, E883E18 UsbMassInitMedia: UsbBootGetParams (Media changed) InstallProtocolInterface: 964E5B21-6459-11D2-8E39-00A0C969723B E8839B8 InstallProtocolInterface: D432A67F-14DC-484B-B3BB-3F0291849327 E883A30 InstallProtocolInterface: CE345171-BA0B-11D2-8E4F-00A0C969723B E883720 BlockSize : 512 LastBlock : EC3FFF InstallProtocolInterface: 09576E91-6D3F-11D2-8E39-00A0C969723B E879F18 InstallProtocolInterface: 964E5B21-6459-11D2-8E39-00A0C969723B E879030 InstallProtocolInterface: 8CF2F62C-BC9B-4821-808D-EC9EC421A1A0 E8790E8 InstallProtocolInterface: 09576E91-6D3F-11D2-8E39-00A0C969723B E883418 InstallProtocolInterface: 964E5B21-6459-11D2-8E39-00A0C969723B E87AAB0 InstallProtocolInterface: 8CF2F62C-BC9B-4821-808D-EC9EC421A1A0 E87AB68 PartitionDxe: El Torito standard found on handle 0xE883E18. InstallProtocolInterface: CE345171-BA0B-11D2-8E4F-00A0C969723B E8792A0 BlockSize : 512 LastBlock : F InstallProtocolInterface: CE345171-BA0B-11D2-8E4F-00A0C969723B E87AEA0 BlockSize : 512 LastBlock : 7FFF InstallProtocolInterface: 964E5B22-6459-11D2-8E39-00A0C969723B E868030 Installed Fat filesystem on E884C18 UsbConnectDriver: TPL after connect is 8 PROGRESS CODE: V02020006 I0 XhcClearRootHubPortFeature: status Success Disable device slot 5! UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device at port 13 removed from root hub EB08C98 UsbDisconnectDriver: old TPL is 8, E883E18 Success to stop non-multi-lun root handle UsbDisconnectDriver: TPL after disconnect is 8, 0 UsbRemoveDevice: device 4 removed UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 5 state - 801, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 5) UsbEnumeratePort: new device connected at port 5 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 5 is reset UsbEnumerateNewDev: No device present at port 5 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 5 state - 801, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 5) UsbEnumeratePort: new device connected at port 5 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 5 is reset UsbEnumerateNewDev: No device present at port 5 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 5 state - 801, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 5) UsbEnumeratePort: new device connected at port 5 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 5 is reset UsbEnumerateNewDev: No device present at port 5 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 803, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: new device connected at port 13 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 13 is reset XhcClearRootHubPortFeature: status Success UsbEnumerateNewDev: device is of 3 speed UsbEnumerateNewDev: device uses translator (0, 0) XhcControlTransfer: error - Device Error, transfer - 100 UsbEnumerateNewDev: failed to set device address - Device Error XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 803, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device at port 13 removed from root hub EB08C98 UsbRemoveDevice: device 4 removed UsbEnumeratePort: new device connected at port 13 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 13 is reset UsbEnumerateNewDev: No device present at port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 5 state - 801, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 5) UsbEnumeratePort: new device connected at port 5 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 5 is reset UsbEnumerateNewDev: No device present at port 5 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 803, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: new device connected at port 13 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 13 is reset UsbEnumerateNewDev: No device present at port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 5 state - 801, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 5) UsbEnumeratePort: new device connected at port 5 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 5 is reset UsbEnumerateNewDev: No device present at port 5 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 803, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: new device connected at port 13 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success Enable Slot Successfully, The Slot ID = 0x6 Address 6 assigned successfully UsbEnumerateNewDev: hub port 13 is reset UsbEnumerateNewDev: device is of 3 speed UsbEnumerateNewDev: device uses translator (0, 0) UsbEnumerateNewDev: device is now ADDRESSED at 4 UsbEnumerateNewDev: max packet size for EP 0 is 512 Evaluate context UsbBuildDescTable: device has 1 configures UsbGetOneConfig: total length is 44 UsbParseConfigDesc: config 1 has 1 interfaces UsbParseInterfaceDesc: interface 0(setting 0) has 2 endpoints Endpoint[81]: Created BULK ring [EB25280~EB26280) Endpoint[2]: Created BULK ring [EB26280~EB27280) Configure Endpoint UsbEnumerateNewDev: device 4 is now in CONFIGED state UsbSelectConfig: config 1 selected for device 4 UsbSelectSetting: setting 0 selected for interface 0 InstallProtocolInterface: 09576E91-6D3F-11D2-8E39-00A0C969723B E883318 InstallProtocolInterface: 2B2F68D6-0CD2-44CF-8E8B-BBA20B1B5B75 E8847C0 UsbConnectDriver: TPL before connect is 8, E883D18 XhcCheckUrbResult: TRANSACTION_ERROR! Completecode = 4 XhcControlTransfer: error - Device Error, transfer - 40 XhcCheckUrbResult: TRANSACTION_ERROR! Completecode = 4 XhcBulkTransfer: error - Device Error, transfer - 40 UsbBotExecCommand: UsbBotSendCommand (Device Error) UsbBootExecCmd: Device Error to Exec 0x12 Cmd (Result = 1) Stop Slot = 6,Dci = 4 XhcStopEndpoint: Slot = 0x6, Dci = 0x4 Transfer Default Error Occur! Completecode = 0x13! XhcStopEndpoint: Stop Endpoint Failed, Status = Device Error XhcDequeueTrbFromEndpoint: Stop Endpoint Failed, Status = Device Error XhcTransfer[Type=2]: XhcDequeueTrbFromEndpoint failed! XhcBulkTransfer: error - Time out, transfer - 0 UsbBotExecCommand: UsbBotSendCommand (Time out) UsbBootRequestSense: (Time out) CmdResult=0x1 Stop Slot = 6,Dci = 4 XhcStopEndpoint: Slot = 0x6, Dci = 0x4 Transfer Default Error Occur! Completecode = 0x13! XhcStopEndpoint: Stop Endpoint Failed, Status = Device Error XhcDequeueTrbFromEndpoint: Stop Endpoint Failed, Status = Device Error XhcTransfer[Type=2]: XhcDequeueTrbFromEndpoint failed! XhcBulkTransfer: error - Time out, transfer - 0 UsbBotExecCommand: UsbBotSendCommand (Time out) UsbBootExecCmd: Time out to Exec 0x12 Cmd Stop Slot = 6,Dci = 4 XhcStopEndpoint: Slot = 0x6, Dci = 0x4 Transfer Default Error Occur! Completecode = 0x13! XhcStopEndpoint: Stop Endpoint Failed, Status = Device Error XhcDequeueTrbFromEndpoint: Stop Endpoint Failed, Status = Device Error XhcTransfer[Type=2]: XhcDequeueTrbFromEndpoint failed! XhcBulkTransfer: error - Time out, transfer - 0 UsbBotExecCommand: UsbBotSendCommand (Time out) UsbBootExecCmd: Time out to Exec 0x12 Cmd Stop Slot = 6,Dci = 4 XhcStopEndpoint: Slot = 0x6, Dci = 0x4 Transfer Default Error Occur! Completecode = 0x13! XhcStopEndpoint: Stop Endpoint Failed, Status = Device Error XhcDequeueTrbFromEndpoint: Stop Endpoint Failed, Status = Device Error XhcTransfer[Type=2]: XhcDequeueTrbFromEndpoint failed! XhcBulkTransfer: error - Time out, transfer - 0 UsbBotExecCommand: UsbBotSendCommand (Time out) UsbBootExecCmd: Time out to Exec 0x12 Cmd Stop Slot = 6,Dci = 4 XhcStopEndpoint: Slot = 0x6, Dci = 0x4 Transfer Default Error Occur! Completecode = 0x13! XhcStopEndpoint: Stop Endpoint Failed, Status = Device Error XhcDequeueTrbFromEndpoint: Stop Endpoint Failed, Status = Device Error XhcTransfer[Type=2]: XhcDequeueTrbFromEndpoint failed! XhcBulkTransfer: error - Time out, transfer - 0 UsbBotExecCommand: UsbBotSendCommand (Time out) UsbBootExecCmd: Time out to Exec 0x12 Cmd Stop Slot = 6,Dci = 4 XhcStopEndpoint: Slot = 0x6, Dci = 0x4 Transfer Default Error Occur! Completecode = 0x13! XhcStopEndpoint: Stop Endpoint Failed, Status = Device Error XhcDequeueTrbFromEndpoint: Stop Endpoint Failed, Status = Device Error XhcTransfer[Type=2]: XhcDequeueTrbFromEndpoint failed! XhcBulkTransfer: error - Time out, transfer - 0 UsbBotExecCommand: UsbBotSendCommand (Time out) UsbBootExecCmd: Time out to Exec 0x12 Cmd UsbBootGetParams: UsbBootInquiry (Time out) UsbMassInitMedia: UsbBootGetParams (Time out) UsbMassInitNonLun: UsbMassInitMedia (Time out) USBMassDriverBindingStart: UsbMassInitNonLun (Time out) UsbConnectDriver: TPL after connect is 8 UsbSelectConfig: failed to connect driver Not Found, ignored PROGRESS CODE: V02020006 I0 XhcClearRootHubPortFeature: status Success Disable device slot 6! XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 5 state - 801, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 5) UsbEnumeratePort: new device connected at port 5 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 5 is reset UsbEnumerateNewDev: No device present at port 5 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device at port 13 removed from root hub EB08C98 UsbRemoveDevice: device 4 removed UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 803, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: new device connected at port 13 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 13 is reset UsbEnumerateNewDev: No device present at port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 803, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: new device connected at port 13 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 13 is reset UsbEnumerateNewDev: No device present at port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 5 state - 801, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 5) UsbEnumeratePort: new device connected at port 5 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 5 is reset UsbEnumerateNewDev: No device present at port 5 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 803, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: new device connected at port 13 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 13 is reset UsbEnumerateNewDev: No device present at port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 5 state - 801, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 5) UsbEnumeratePort: new device connected at port 5 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 5 is reset UsbEnumerateNewDev: No device present at port 5 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 5 state - 801, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 5) UsbEnumeratePort: new device connected at port 5 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 5 is reset UsbEnumerateNewDev: No device present at port 5 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 5 state - 801, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 5) UsbEnumeratePort: new device connected at port 5 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 5 is reset UsbEnumerateNewDev: No device present at port 5 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 5 state - 801, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 5) UsbEnumeratePort: new device connected at port 5 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 5 is reset UsbEnumerateNewDev: No device present at port 5 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 5 state - 801, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 5) UsbEnumeratePort: new device connected at port 5 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 5 is reset UsbEnumerateNewDev: No device present at port 5 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 00, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: device disconnected event on port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 5 state - 801, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 5) UsbEnumeratePort: new device connected at port 5 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 5 is reset UsbEnumerateNewDev: No device present at port 5 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 803, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: new device connected at port 13 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 13 is reset UsbEnumerateNewDev: No device present at port 13 XhcClearRootHubPortFeature: status Success UsbEnumeratePort: port 13 state - 803, change - 01 on EB08C98 UsbEnumeratePort: Device Connect/Disconnect Normally (port 13) UsbEnumeratePort: new device connected at port 13 XhcUsbPortReset! XhcSetRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcClearRootHubPortFeature: status Success XhcSetRootHubPortFeature: status Success UsbEnumerateNewDev: hub port 13 is reset UsbEnumerateNewDev: No device present at port 13 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing on error path 2022-09-21 8:00 ` Sean Rhodes @ 2022-09-21 8:24 ` Wu, Hao A 2022-09-21 8:36 ` Sean Rhodes 0 siblings, 1 reply; 10+ messages in thread From: Wu, Hao A @ 2022-09-21 8:24 UTC (permalink / raw) To: Rhodes, Sean; +Cc: devel@edk2.groups.io, Ni, Ray [-- Attachment #1: Type: text/plain, Size: 3749 bytes --] 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> Sent: Wednesday, September 21, 2022 4:01 PM To: Wu, Hao A <hao.a.wu@intel.com> Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@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.wu@intel.com<mailto:hao.a.wu@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.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@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.wu@intel.com<mailto:hao.a.wu@intel.com>> > Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@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+owner@edk2.groups.io<mailto:devel%2Bowner@edk2.groups.io> > Unsubscribe: https://edk2.groups.io/g/devel/unsub [hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>] > -=-=-=-=-=-= > [-- Attachment #2: Type: text/html, Size: 8769 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing on error path 2022-09-21 8:24 ` Wu, Hao A @ 2022-09-21 8:36 ` Sean Rhodes 2022-09-22 7:30 ` Wu, Hao A 0 siblings, 1 reply; 10+ messages in thread From: Sean Rhodes @ 2022-09-21 8:36 UTC (permalink / raw) To: Wu, Hao A; +Cc: devel@edk2.groups.io, Ni, Ray [-- Attachment #1: Type: text/plain, Size: 4259 bytes --] 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.wu@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> > *Sent:* Wednesday, September 21, 2022 4:01 PM > *To:* Wu, Hao A <hao.a.wu@intel.com> > *Cc:* devel@edk2.groups.io; Ni, Ray <ray.ni@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.wu@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 <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 <sean@starlabs.systems>; Wu, Hao A > > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@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.wu@intel.com> > > Cc: Ray Ni <ray.ni@intel.com> > > Signed-off-by: Sean Rhodes <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+owner@edk2.groups.io > > Unsubscribe: https://edk2.groups.io/g/devel/unsub [hao.a.wu@intel.com] > > -=-=-=-=-=-= > > > > [-- Attachment #2: Type: text/html, Size: 9617 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing on error path 2022-09-21 8:36 ` Sean Rhodes @ 2022-09-22 7:30 ` Wu, Hao A 2022-09-23 8:02 ` Sean Rhodes 0 siblings, 1 reply; 10+ messages in thread From: Wu, Hao A @ 2022-09-22 7:30 UTC (permalink / raw) To: devel@edk2.groups.io, Rhodes, Sean; +Cc: Ni, Ray [-- Attachment #1: Type: text/plain, Size: 6490 bytes --] 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 <devel@edk2.groups.io> On Behalf Of Sean Rhodes Sent: Wednesday, September 21, 2022 4:37 PM To: Wu, Hao A <hao.a.wu@intel.com> Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@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.wu@intel.com<mailto:hao.a.wu@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.wu@intel.com<mailto:hao.a.wu@intel.com>> Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@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.wu@intel.com<mailto:hao.a.wu@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.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@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.wu@intel.com<mailto:hao.a.wu@intel.com>> > Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@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+owner@edk2.groups.io<mailto:devel%2Bowner@edk2.groups.io> > Unsubscribe: https://edk2.groups.io/g/devel/unsub [hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>] > -=-=-=-=-=-= > [-- Attachment #2: Type: text/html, Size: 18015 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing on error path 2022-09-22 7:30 ` Wu, Hao A @ 2022-09-23 8:02 ` Sean Rhodes 2022-09-23 8:12 ` Wu, Hao A 0 siblings, 1 reply; 10+ messages in thread From: Sean Rhodes @ 2022-09-23 8:02 UTC (permalink / raw) To: Wu, Hao A; +Cc: devel@edk2.groups.io, Ni, Ray [-- Attachment #1: Type: text/plain, Size: 6967 bytes --] 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.wu@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 <devel@edk2.groups.io> * On Behalf Of *Sean > Rhodes > *Sent:* Wednesday, September 21, 2022 4:37 PM > *To:* Wu, Hao A <hao.a.wu@intel.com> > *Cc:* devel@edk2.groups.io; Ni, Ray <ray.ni@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.wu@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> > *Sent:* Wednesday, September 21, 2022 4:01 PM > *To:* Wu, Hao A <hao.a.wu@intel.com> > *Cc:* devel@edk2.groups.io; Ni, Ray <ray.ni@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.wu@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 <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 <sean@starlabs.systems>; Wu, Hao A > > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@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.wu@intel.com> > > Cc: Ray Ni <ray.ni@intel.com> > > Signed-off-by: Sean Rhodes <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+owner@edk2.groups.io > > Unsubscribe: https://edk2.groups.io/g/devel/unsub [hao.a.wu@intel.com] > > -=-=-=-=-=-= > > > > > [-- Attachment #2: Type: text/html, Size: 16652 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing on error path 2022-09-23 8:02 ` Sean Rhodes @ 2022-09-23 8:12 ` Wu, Hao A 0 siblings, 0 replies; 10+ messages in thread From: Wu, Hao A @ 2022-09-23 8:12 UTC (permalink / raw) To: Rhodes, Sean; +Cc: devel@edk2.groups.io, Ni, Ray [-- Attachment #1: Type: text/plain, Size: 7967 bytes --] 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.wu@intel.com> Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@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.wu@intel.com<mailto:hao.a.wu@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.wu@intel.com<mailto:hao.a.wu@intel.com>> Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@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.wu@intel.com<mailto:hao.a.wu@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.wu@intel.com<mailto:hao.a.wu@intel.com>> Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@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.wu@intel.com<mailto:hao.a.wu@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.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@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.wu@intel.com<mailto:hao.a.wu@intel.com>> > Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@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+owner@edk2.groups.io<mailto:devel%2Bowner@edk2.groups.io> > Unsubscribe: https://edk2.groups.io/g/devel/unsub [hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>] > -=-=-=-=-=-= > [-- Attachment #2: Type: text/html, Size: 25522 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-09-23 8:12 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-09-20 13:14 [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing on error path Sean Rhodes 2022-09-20 13:14 ` [PATCH 2/2] MdeModulePkg/UsbBusDxe: Reset the device on error Sean Rhodes 2022-09-21 5:31 ` [edk2-devel] " Wu, Hao A 2022-09-21 5:30 ` [edk2-devel] [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing on error path Wu, Hao A 2022-09-21 8:00 ` Sean Rhodes 2022-09-21 8:24 ` Wu, Hao A 2022-09-21 8:36 ` Sean Rhodes 2022-09-22 7:30 ` Wu, Hao A 2022-09-23 8:02 ` Sean Rhodes 2022-09-23 8:12 ` Wu, Hao A
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox