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.web08.4055.1663749413893228408 for ; Wed, 21 Sep 2022 01:36:54 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@starlabs-systems.20210112.gappssmtp.com header.s=20210112 header.b=CtBXG5lf; spf=pass (domain: starlabs.systems, ip: 209.85.219.179, mailfrom: sean@starlabs.systems) Received: by mail-yb1-f179.google.com with SMTP id 198so7036995ybc.1 for ; Wed, 21 Sep 2022 01:36:53 -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=s/H7fUR5GURouxuZoWnF87XbNdAvDWpH/hpbCCbBqP4=; b=CtBXG5lfTJ+PVNi0hjsBlje8nROu13mbaUo50EoerXKWREhmDXsg7XORN5pMDgA6m7 9lkCKi1Aa7Kr2EQLnqN0aReVvLoR44Vlf9omL4fjP7gW0qYp8JXJf6qtKoU5kigOHXTt PkT0eoUu/uiHTGDHiI4T947McW0IaO5T+N3EiqnUKlbkkZXl7q+P5X+zloRoxnMu8Urm FlMeWd8EXARYL2sG4pBZFkOaoG5vpF9ou+bTz2CQLP2CtNZvII+qGRjv7hudG3y4DEPg +IweOdTEFwkglWb6XT2OVgKwj1pAC67cREiZpyexHhDb5vKeFceDdPJctBJQtBcvEqns xSqQ== 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=s/H7fUR5GURouxuZoWnF87XbNdAvDWpH/hpbCCbBqP4=; b=Gsc/RsnqKltSnghHbB8cYMzYUvdUQMF2GphSNUI3DjtrIOaryDxLClhlzC3gt6t2cM x3kbneblh0SBhZFamhOVZXIBWbiAPwajKNAcJyArkpkgMX7n0VayF5IkltLmDvKIkhEt ILJeotTYboEZRI/LK+zTTJMP425leY9N/W437r4xJacufR5ySxtwjkaKm3LP0x75LUpX A31bCvlF4dIn2CuaKcwb4Bt1azqlHQq2E/ycD/qcXe6zZVP3Yp0S4rNGFHqf9Xqk6nWS 0DcVQgtXwcIpsq2gMsE7NNSFvrmMzJCJHLaU4dYEzBPp5yTPJBCsycC47uLm/qb0zgm7 OBUg== X-Gm-Message-State: ACrzQf1g3sJ4lQ/ZYI0VV1mcTfFO4SKUTwRCBLD/y3YBd+WKwP6QEk0b 7FGo7KoANCxZoGpQF6KqCTye7iI43w1G+wAvlWCe X-Google-Smtp-Source: AMsMyM6e3lgOlCzYY7oiTsFzwpthe1+Bl4jvtLVoqogva18GWdhuKAzirUqfvc1bR0CftarURK4uLTIVUvYZSI+cjas= X-Received: by 2002:a25:288:0:b0:6af:5c0a:cf9e with SMTP id 130-20020a250288000000b006af5c0acf9emr22044828ybc.527.1663749412921; Wed, 21 Sep 2022 01:36:52 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: "Sean Rhodes" Date: Wed, 21 Sep 2022 09:36:41 +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="000000000000c0b9e305e92bd88d" --000000000000c0b9e305e92bd88d Content-Type: text/plain; charset="UTF-8" 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] > > -=-=-=-=-=-= > > > > --000000000000c0b9e305e92bd88d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Hao

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 port. It won't stop doing this, and the USB = drive can't be accessed.

I tested 13 USB drives; 8 saw t= his problem, and 5 worked correctly. It doesn't matter which SOC, or if= the port is USB 2.0 or 3.0.

"=C2=A0Reset the device on= error" - this will only attempt enumeration if there isn't an err= or.
"Avoid continuing on error path" - The reset can happen = before PortState is zero'd, so running this at the start of the functio= n ensures it will

Hopefully, that makes sense!


Sean


=
On Wed, 21 Sept 2022 at 09:24, Wu, Ha= o A <hao.a.wu@intel.com> wr= ote:

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&g= t;
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
>

--000000000000c0b9e305e92bd88d--