public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, patrick.henz@hpe.com
Cc: Jian J Wang <jian.j.wang@intel.com>,
	Hao A Wu <hao.a.wu@intel.com>, Ray Ni <ray.ni@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts
Date: Wed, 2 Sep 2020 08:45:39 +0200	[thread overview]
Message-ID: <17e2b77f-d10c-9953-b588-f03bfa496d74@redhat.com> (raw)
In-Reply-To: <ada27224f32b4ac24ca9df31668ab4500fd8fa21.1598978760.git.patrick.henz@hpe.com>

Hi Patrick,

On 09/01/20 20:55, patrick.henz@hpe.com wrote:
> From: henz <patrick.henz@hpe.com>

some meta-comments:

(1) Please consider setting the "user.name" item in your git config to
your full name, "Patrick Henz". Because right now it seems to be just
"henz", and that doesn't look very nice in the git commit history.

(2) When you update this patch for v2, point (1) will not fix this very
patch however. So please update this particular patch with

  git commit --amend --author='Patrick Henz <patrick.henz@hpe.com>'

(3) Please run

  python3 BaseTools/Scripts/SetupGit.py

in your edk2 clone -- it automates a bunch of the settings listed at
<https://github.com/tianocore/tianocore.github.io/wiki/Laszlo%27s-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05>,
and more. For example, it sets 8bit content-transfer-encoding for
mailing out your edk2 patches, which is helpful because quoted-printable
doesn't play nice with the hard CRLF line endings that edk2 uses.

Thank you!
Laszlo


> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2948
> 
> Timeouts in the XhciDxe driver are taking longer than
> expected due to the timeout loops not accounting for
> code execution time. As en example, 5 second timeouts
> have been observed to take around 36 seconds to complete.
> Use SetTimer and Create/CheckEvent from Boot Services to
> determine when timeout occurred.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c   | 28 ++++++++++++++++---
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 34 +++++++++++++++++-------
>  2 files changed, 49 insertions(+), 13 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> index 42b773ab31..5f7507f0a5 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> @@ -442,17 +442,37 @@ XhcWaitOpRegBit (
>    IN UINT32               Timeout
> 
>    )
> 
>  {
> 
> -  UINT32                  Index;
> 
> -  UINT64                  Loop;
> 
> +  EFI_STATUS Status;
> 
> +  EFI_EVENT  TimeoutEvent;
> 
>  
> 
> -  Loop   = Timeout * XHC_1_MILLISECOND;
> 
> +  if (Timeout == 0) {
> 
> +    return EFI_TIMEOUT;
> 
> +  }
> 
> +
> 
> +  Status = gBS->CreateEvent (
> 
> +                  EVT_TIMER,
> 
> +                  TPL_CALLBACK,
> 
> +                  NULL,
> 
> +                  NULL,
> 
> +                  &TimeoutEvent
> 
> +                  );
> 
>  
> 
> -  for (Index = 0; Index < Loop; Index++) {
> 
> +  if (!EFI_ERROR (Status)) {
> 
> +    Status = gBS->SetTimer (TimeoutEvent,
> 
> +                            TimerRelative,
> 
> +                            EFI_TIMER_PERIOD_MILLISECONDS(Timeout));
> 
> +  }
> 
> +
> 
> +  do {
> 
>      if (XHC_REG_BIT_IS_SET (Xhc, Offset, Bit) == WaitToSet) {
> 
>        return EFI_SUCCESS;
> 
>      }
> 
>  
> 
>      gBS->Stall (XHC_1_MICROSECOND);
> 
> +  } while (!EFI_ERROR(Status) && EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
> 
> +
> 
> +  if (TimeoutEvent != NULL) {
> 
> +    gBS->CloseEvent (TimeoutEvent);
> 
>    }
> 
>  
> 
>    return EFI_TIMEOUT;
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> index ab8957c546..cf271f0493 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> @@ -1273,11 +1273,11 @@ XhcExecTransfer (
>    )
> 
>  {
> 
>    EFI_STATUS              Status;
> 
> -  UINTN                   Index;
> 
> -  UINT64                  Loop;
> 
>    UINT8                   SlotId;
> 
>    UINT8                   Dci;
> 
>    BOOLEAN                 Finished;
> 
> +  EFI_EVENT               TimeoutEvent;
> 
> +  EFI_STATUS              TimerStatus;
> 
>  
> 
>    if (CmdTransfer) {
> 
>      SlotId = 0;
> 
> @@ -1292,28 +1292,44 @@ XhcExecTransfer (
>    }
> 
>  
> 
>    Status = EFI_SUCCESS;
> 
> -  Loop   = Timeout * XHC_1_MILLISECOND;
> 
> -  if (Timeout == 0) {
> 
> -    Loop = 0xFFFFFFFF;
> 
> -  }
> 
> +
> 
> +  TimerStatus = gBS->CreateEvent (
> 
> +                       EVT_TIMER,
> 
> +                       TPL_CALLBACK,
> 
> +                       NULL,
> 
> +                       NULL,
> 
> +                       &TimeoutEvent
> 
> +                       );
> 
>  
> 
>    XhcRingDoorBell (Xhc, SlotId, Dci);
> 
>  
> 
> -  for (Index = 0; Index < Loop; Index++) {
> 
> +  if (!EFI_ERROR (TimerStatus)) {
> 
> +    TimerStatus = gBS->SetTimer (TimeoutEvent,
> 
> +                                 TimerRelative,
> 
> +                                 (0 == Timeout)?
> 
> +                                 (EFI_TIMER_PERIOD_MICROSECONDS(0xFFFFFFFF)):
> 
> +                                 (EFI_TIMER_PERIOD_MILLISECONDS(Timeout)));
> 
> +  }
> 
> +
> 
> +  do {
> 
>      Finished = XhcCheckUrbResult (Xhc, Urb);
> 
>      if (Finished) {
> 
>        break;
> 
>      }
> 
>      gBS->Stall (XHC_1_MICROSECOND);
> 
> -  }
> 
> +  } while (!EFI_ERROR(TimerStatus) && EFI_ERROR(gBS->CheckEvent (TimeoutEvent)));
> 
>  
> 
> -  if (Index == Loop) {
> 
> +  if (!Finished) {
> 
>      Urb->Result = EFI_USB_ERR_TIMEOUT;
> 
>      Status      = EFI_TIMEOUT;
> 
>    } else if (Urb->Result != EFI_USB_NOERROR) {
> 
>      Status      = EFI_DEVICE_ERROR;
> 
>    }
> 
>  
> 
> +  if (TimeoutEvent != NULL) {
> 
> +    gBS->CloseEvent (TimeoutEvent);
> 
> +  }
> 
> +
> 
>    return Status;
> 
>  }
> 
>  
> 


  parent reply	other threads:[~2020-09-02  6:45 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-01 18:55 [PATCH 0/1] Fix XhciDxe Timeouts patrick.henz
2020-09-01 18:55 ` [PATCH 1/1] MdeModulePkg/XhciDxe: Fix Broken Timeouts patrick.henz
2020-09-02  3:21   ` Ni, Ray
2020-09-02  6:45   ` Laszlo Ersek [this message]
2020-09-02  5:08 ` [PATCH 0/1] Fix XhciDxe Timeouts Wu, Hao A
2020-09-02  6:49   ` [edk2-devel] " Laszlo Ersek
2020-09-02  6:58     ` Wu, Hao A
2020-09-02 23:03   ` patrick.henz
2020-09-03  3:54     ` [edk2-devel] " Wu, Hao A
2020-09-03  8:53       ` Ni, Ray

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=17e2b77f-d10c-9953-b588-f03bfa496d74@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox