Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/UsbBusDxe: Avoid continuing on error path

["Sean Rhodes" <sean@starlabs.systems>]

[-- 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 --]