public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Cai, Xianglei" <xianglei.cai@intel.com>,
	"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Ni, Ray" <ray.ni@intel.com>,
	"Wang, Jian J" <jian.j.wang@intel.com>,
	"Gao, Liming" <gaoliming@byosoft.com.cn>,
	"Shih, More" <more.shih@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg\XhciDxe: Command Abort when times out
Date: Wed, 20 Sep 2023 08:19:39 +0000	[thread overview]
Message-ID: <DM6PR11MB4025DAD64DEC7CE168AD205DCAF9A@DM6PR11MB4025.namprd11.prod.outlook.com> (raw)
In-Reply-To: <91ce12999b52f08034b6cc58ad642dd526cf6c4b.1695020158.git.xianglei.cai@intel.com>

General level comments:
1. Some CI tests failed for the proposed patch: https://github.com/tianocore/edk2/pull/4844. Could you help to resolve them?
Please help to follow the step 11 in page: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process and check the CI test results for the upcoming patch?

2. Could you help to update the title of this commit to "MdeModulePkg/XhciDxe: Abort the Address Device cmd when time out" to better reflect the purpose of the change?

3. Could you help to add a brief description as the commit log message?

Serveral inline comments below:


> -----Original Message-----
> From: Cai, Xianglei <xianglei.cai@intel.com>
> Sent: Monday, September 18, 2023 2:57 PM
> To: devel@edk2.groups.io
> Cc: Cai, Xianglei <xianglei.cai@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao,
> Liming <gaoliming@byosoft.com.cn>; Shih, More <more.shih@intel.com>
> Subject: [PATCH 1/1] MdeModulePkg\XhciDxe: Command Abort when times
> out
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=4552
> 
> CC: Hao A Wu     <hao.a.wu@intel.com>
> CC: Ray Ni       <ray.ni@intel.com>
> CC: Jian J Wang  <jian.j.wang@intel.com>
> CC: Liming Gao   <gaoliming@byosoft.com.cn>
> CC: More Shih    <more.shih@intel.com>
> Signed-off-by: Xianglei Cai <xianglei.cai@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 35
> ++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> index 53421e64a850..5024d5e3acb9 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> @@ -2121,6 +2121,29 @@ RingIntTransferDoorBell (
>    return EFI_SUCCESS;
>  }
> 
> +/**
> +  Set Command abort
> +
> +  @param  Xhc           The XHCI Instance.
> +  @param  SlotId        The slot id to be disabled.
> +
> +  @retval EFI_SUCCESS   Successfully disable the device slot.


This function does not return value. Please help to update the above function description.

> +
> +**/
> +VOID
> +XhcCmdRingCmdAbort (
> +  IN USB_XHCI_INSTANCE  *Xhc,
> +  IN UINT8              SlotId
> +  )
> +{
> +  //
> +  // Some 3rd party XHCI external cards don't support single 64-bytes
> +width register access,
> +  // So divide it to two 32-bytes width register access.
> +  //


The above comments does not apply here. Please help to remove it.


> +  DEBUG ((DEBUG_INFO, "Command Ring Control set Command Abort,
> SlotId:
> +%d!\n", SlotId));
> +  XhcSetOpRegBit (Xhc, XHC_CRCR_OFFSET, XHC_CRCR_CA); }


Sorry for a question, do we need to wait the CRR bit of the CRCR register to become 0 to ensure the command ring is stopped?


> +
>  /**
>    Assign and initialize the device slot for a new device.
> 
> @@ -2331,6 +2354,12 @@ XhcInitializeDeviceSlot (
>      Xhc->UsbDevContext[SlotId].XhciDevAddr = DeviceAddress;
>    } else {
>      DEBUG ((DEBUG_ERROR, "    Slot %d address not assigned successfully.
> Status = %r\n", SlotId, Status));
> +    //
> +    // Software need abort the command if a command times out in xHCI spec
> 4.6.
> +    //


Suggest to update "xHCI spec 4.6" to "xHCI spec Revision 1.2, section 4.6.1.2"

Best Regards,
Hao Wu


> +    if (Status == EFI_TIMEOUT) {
> +      XhcCmdRingCmdAbort (Xhc, SlotId);
> +    }
>      XhcDisableSlotCmd (Xhc, SlotId);
>    }
> 
> @@ -2547,6 +2576,12 @@ XhcInitializeDeviceSlot64 (
>      Xhc->UsbDevContext[SlotId].XhciDevAddr = DeviceAddress;
>    } else {
>      DEBUG ((DEBUG_ERROR, "    Slot %d address not assigned successfully.
> Status = %r\n", SlotId, Status));
> +    //
> +    // Software need abort the command if a command times out in xHCI spec
> 4.6.
> +    //
> +    if (Status == EFI_TIMEOUT) {
> +      XhcCmdRingCmdAbort (Xhc, SlotId);
> +    }
>      XhcDisableSlotCmd64 (Xhc, SlotId);
>    }
> 
> --
> 2.42.0.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108900): https://edk2.groups.io/g/devel/message/108900
Mute This Topic: https://groups.io/mt/101435621/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



  reply	other threads:[~2023-09-20  8:19 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <cover.1695020158.git.xianglei.cai@intel.com>
2023-09-18  6:56 ` [edk2-devel] [PATCH 1/1] MdeModulePkg\XhciDxe: Command Abort when times out Xianglei Cai
2023-09-20  8:19   ` Wu, Hao A [this message]
     [not found] ` <1786073BDD30B479.9824@groups.io>
2023-09-20  3:07   ` Xianglei Cai

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=DM6PR11MB4025DAD64DEC7CE168AD205DCAF9A@DM6PR11MB4025.namprd11.prod.outlook.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