From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-yb1-f179.google.com (mail-yb1-f179.google.com [209.85.219.179]) by mx.groups.io with SMTP id smtpd.web09.5175.1663920135804664196 for ; Fri, 23 Sep 2022 01:02:16 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@starlabs-systems.20210112.gappssmtp.com header.s=20210112 header.b=x6m1zTlS; spf=pass (domain: starlabs.systems, ip: 209.85.219.179, mailfrom: sean@starlabs.systems) Received: by mail-yb1-f179.google.com with SMTP id y82so16108467yby.6 for ; Fri, 23 Sep 2022 01:02:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=starlabs-systems.20210112.gappssmtp.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=ZiZAy8Q/zNc0uA50QVDYdciAN7iHGWPTKEfDdiywi5o=; b=x6m1zTlSLp9X+zN7noDXYJmMfYn8+XVe/+qM2k+rZ7G32ZU74O416B/ElPcq/AVzP9 IXWtYbXVZrDftpuJmNdmY9YXGsTp+uLZqGlrvJRaX18BXfdVLBFbZRH4rJnI2IbykKu9 yz8ix2pPnXd/nM1hvXBlBh5HRjTGcI+nWBZ4jjhS4rxbnXacpXxrL8jMDSxy+qHuNbn+ L0nj3vxHP0YIg0P2B2M3rqJqkXzyud+A6WkZ7y8gkb7MBgrUdcgfB+j/RaGcmkilconw ijWkZN5F8cYXP3ZoXIMlZXKIL23u1+qBcPcMdc+vVSsnXqSGpnD+eiLbQBl3i7byYAY9 ja3Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=ZiZAy8Q/zNc0uA50QVDYdciAN7iHGWPTKEfDdiywi5o=; b=LP6BMnUQ2wHE4gqkLY5FTgbsczmq/h3dEesxjlppSCLcXYcOHpZ1OgXxSorcqVPOO2 sTrMIbSK71KYiXw0YczfCgPNljOv0YjVBSbD3JyuyrwKlImQ5FSDZMZsC5/B07IKZ+AE Wc4c48FcBCVjmOjhwPAlpngr7EuqRVCSkO0fPVLA+7WIXwVACgw1DIhysztd10Uw1vXk xumhuKWDUdjGi6K1ptpdyGTGEV3h7v73xD7oj4cXG2+yGwoLlwLPlWHBJpRz7L4xSaDG q9p0Jt1LH4M7kSW84lrArXIhY54ySjHVVAfyrwcF4o+2pFGz/xmp7lGOG41pH3QVkifz oWZw== X-Gm-Message-State: ACrzQf0kizb2QpVIr89I/RvVORE6lwxyZCh0DKSM+ZBFWy2x2hrLFm+6 uOuzLA/ZoMVwSsMse/LTS9Mc9lmcA5n4Q2QQv0Xz X-Google-Smtp-Source: AMsMyM4f0LmZpsS2BeH5aXCZ/VXL4/ifhUtZxTVNug6a8DpK2bOVMluTSBJYDnmGVX4V0E4KA1hTgBE40s23tY7tnH8= X-Received: by 2002:a25:bc8b:0:b0:6b0:43:8cac with SMTP id e11-20020a25bc8b000000b006b000438cacmr7755870ybk.576.1663920134746; Fri, 23 Sep 2022 01:02:14 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: "Sean Rhodes" Date: Fri, 23 Sep 2022 09:02:03 +0100 Message-ID: Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing on error path To: "Wu, Hao A" Cc: "devel@edk2.groups.io" , "Ni, Ray" Content-Type: multipart/alternative; boundary="0000000000009109a305e9539857" --0000000000009109a305e9539857 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 =E2=80=9C"Avoid continuing on error path" - The reset can happen b= efore > PortState is zero'd, so running this at the start of the function ensures > it will=E2=80=9D: > > My take is that the execution flow is like: > > UsbEnumeratePort > > Status =3D 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 =3D (UINT32)(XHC_PORTSC_OFFSET + (0x10 * > PortNumber)); > > PortStatus->PortStatus =3D 0; > > PortStatus->PortChangeStatus =3D 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 =3D 0; > > PortState.PortChangeStatus =3D 0; > > > > Or the test you are performing includes connecting USB devices on a USB > Hub? > > > > 2. For =E2=80=9C" Reset the device on error" - this will only attempt enu= meration > if there isn't an error.=E2=80=9D: > > 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 !=3D EFI_DEVICE_ERROR)) > > =E2=80=98Status=E2=80=99 will always be EFI_SUCCESS, I do not understand = what is the point > of adding =E2=80=9C&& (Status !=3D EFI_DEVICE_ERROR)=E2=80=9D. > > Could you help to double check if the change is really what you want to d= o? > > > > 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 wil= l > constantly try to access the device, fail and reset the port. It won't st= op > 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 ther= e > 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 =3D 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 =3D NULL; > > > > HubApi =3D HubIf->HubApi; > > > > > > > > + // Zero out PortState in case GetPortStatus does not set it and we > > > > + // continue on the EFI_DEVICE_ERROR path > > > > + PortState.PortStatus =3D 0; > > > > + PortState.PortChangeStatus =3D 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 > > > > > > > > -=3D-=3D-=3D-=3D-=3D-=3D > > Groups.io Links: You receive all messages sent to this group. > > View/Reply Online (#93989): https://edk2.groups.io/g/devel/message/9398= 9 > > 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] > > -=3D-=3D-=3D-=3D-=3D-=3D > > > >=20 > --0000000000009109a305e9539857 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Hao

I retested, and it seems you are c= orrect; the second patch has no effect. Still, the first is required to res= olve the issue -=C2=A0https://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,

=C2=A0

1. For =E2=80=9C"Avoid continuing on error path= " - The reset can happen before PortState is zero'd, so running th= is at the start of the function ensures it will=E2=80=9D:

My take is that the execution flow is like:

= UsbEnumeratePort

= =C2=A0=C2=A0=C2=A0 Status =3D HubApi->GetPortStatus (HubIf, Port, &P= ortState);

= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 UsbRootHubGetPortStatus

= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 UsbHcGet= RootHubPortStatus

= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 XhcGetRootHubPortStatus

If this is the case, within XhcGetRootHubPortStatus = (MdeModulePkg\Bus\Pci\XhciDxe\Xhci.c), it does have logic to:=

= =C2=A0 Offset=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0= =3D (UINT32)(XHC_PORTSC_OFFSET + (0x10 * PortNumber));=

= =C2=A0 PortStatus->PortStatus=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =3D 0;=

= =C2=A0 PortStatus->PortChangeStatus =3D 0;

I am not sure why the below proposed change in UsbEn= umeratePort can resolve the issue:

= =C2=A0 // Zero out PortState in case GetPortStatus does not set it and we

= =C2=A0 // continue on the EFI_DEVICE_ERROR path

= =C2=A0 PortState.PortStatus=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =3D 0;

= =C2=A0 PortState.PortChangeStatus =3D 0;

=C2=A0

Or the test you are performing includes connecting U= SB devices on a USB Hub?

=C2=A0

2. For =E2=80=9C"=C2=A0Reset the device on erro= r" - this will only attempt enumeration if there isn't an error.= =E2=80=9D:

As I mentioned in reply of that patch, by the code e= xecution reaches the change you made:
=C2=A0=C2=A0=C2=A0 if (= USB_BIT_IS_SET (PortState.PortChangeStatus, USB_PORT_STAT_C_RESET) &&am= p;

= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (Status !=3D EFI_DEVICE_ERROR))<= u>

=E2=80=98Status=E2=80=99 will always be EFI_SUCCESS,= I do not understand what is the point of adding =E2=80=9C&& (Statu= s !=3D EFI_DEVICE_ERROR)=E2=80=9D.

Could you help to double check if the change is real= ly what you want to do?

=C2=A0

Best Regards,

Hao Wu

=C2=A0

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

=C2=A0

Hi Hao

=C2=A0

If I just boot a device to the EFI shell and then connect a USB= , edk2 will constantly=C2=A0try to access the device, fail and reset the po= rt. It won't stop doing this, and the USB drive can't be accessed.

=C2=A0

I tested 13 USB drives; 8 saw this problem, and 5 worked correc= tly. It doesn't matter which SOC, or if the port is USB 2.0 or 3.0.<= /u>

=C2=A0

"=C2=A0Reset the device on error" - this will only at= tempt enumeration if there isn't an error.

"Avoid continuing on error path" - The reset can happ= en before PortState is zero'd, so running this at the start of the func= tion ensures it will

=C2=A0

Hopefully, that makes sense!

=C2=A0

Thanks


Sean

=C2=A0

=C2=A0

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 th= e issue.

I am not experienced enough in the USB domain to qui= ckly figure out the whole picture with only log being provided.

=C2=A0

Best Regards,

Hao Wu

=C2=A0

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

=C2=A0

Hi Hao

=C2=A0

I've attached a debug log for the problem I'm trying to= solve (these two patches solve it).

=C2=A0

The reset happens before PortState changes, so it's an endl= ess loop.

=C2=A0

Thanks

=C2=A0

Sean

=C2=A0

On Wed, 21 Sept 2022 at 06:31, Wu, Hao A <hao.a.wu@intel.com>= wrote:

Sorry, could you help t= o 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:

=C2=A0 Status =3D HubApi->GetPortStatus (HubIf, Port, &PortState);
=C2=A0 if (EFI_ERROR (Status)) {
=C2=A0 =C2=A0 DEBUG ((DEBUG_ERROR, "UsbEnumeratePort: failed to get st= ate of port %d\n", Port));
=C2=A0 =C2=A0 return Status;
=C2=A0 }

GetPortStatus() returns EFI_ SUCCESS, but does not update the output parame= ter 'PortState'.
Could you help to check what causes 'PortState' not being updated a= fter 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@ed= k2.groups.io
> Cc: Rhodes, Sean <sean@starlabs.systems>; Wu, Hao A
> <hao.a.wu@i= ntel.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 avo= id
> continuing with EFI_DEVICE_ERROR.
>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <r= ay.ni@intel.com>
> Signed-off-by: Sean Rhodes <sean@starlabs.systems>
> ---
>=C2=A0 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c | 5 +++++
>=C2=A0 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 (
>=C2=A0 =C2=A0 Child=C2=A0 =3D NULL;
>
>=C2=A0 =C2=A0 HubApi =3D HubIf->HubApi;
>
>
>
> +=C2=A0 // Zero out PortState in case GetPortStatus does not set it an= d we
>
> +=C2=A0 // continue on the EFI_DEVICE_ERROR path
>
> +=C2=A0 PortState.PortStatus=C2=A0 =C2=A0 =C2=A0 =C2=A0=3D 0;
>
> +=C2=A0 PortState.PortChangeStatus =3D 0;


Could you help to use:

=C2=A0 ZeroMem (&PortState, sizeof (EFI_USB_PORT_STATUS));

here to align with other places in this driver?


Best Regards,
Hao Wu


>
> +
>
>=C2=A0 =C2=A0 //
>
>=C2=A0 =C2=A0 // Host learns of the new device by polling the hub for p= ort changes.
>
>=C2=A0 =C2=A0 //
>
> --
> 2.34.1
>
>
>
> -=3D-=3D-=3D-=3D-=3D-=3D
> 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]
> -=3D-=3D-=3D-=3D-=3D-=3D
>

--0000000000009109a305e9539857--