Re: [PATCH 1/3] MdeModulePkg/XhciDxe: Refine IsTransferRingTrb and IsAsyncIntTrb

["Wu, Hao A" <hao.a.wu@intel.com>]

Comments are made below.

Best Regards,
Hao Wu


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu
> Ni
> Sent: Wednesday, June 28, 2017 6:40 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A; Tian, Feng; Zeng, Star
> Subject: [edk2] [PATCH 1/3] MdeModulePkg/XhciDxe: Refine IsTransferRingTrb
> and IsAsyncIntTrb
> 
> Current implementation of IsTransferRingTrb only checks whether
> the TRB is in the RING of the URB.
> The patch enhanced the logic to check that whether the TRB belongs
> to the transaction of URB.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 93 ++++++++++++++++-----------
> -----
>  1 file changed, 47 insertions(+), 46 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> index 457344051b..93803c352e 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> @@ -977,45 +977,45 @@ XhcFreeSched (
>  }
> 
>  /**
> -  Check if the Trb is a transaction of the URBs in XHCI's asynchronous transfer
> list.
> +  Check if the Trb is a transaction of the URB.
> 
> -  @param Xhc    The XHCI Instance.
> -  @param Trb    The TRB to be checked.
> -  @param Urb    The pointer to the matched Urb.
> +  @param Trb    The TRB to be checked
> +  @param Urb    The URB to be checked.
> 
> -  @retval TRUE  The Trb is matched with a transaction of the URBs in the async
> list.
> -  @retval FALSE The Trb is not matched with any URBs in the async list.
> +  @retval TRUE  It is a transaction of the URB.
> +  @retval FALSE It is not any transaction of the URB.
> 
>  **/
>  BOOLEAN
> -IsAsyncIntTrb (
> +IsTransferRingTrb (
>    IN  USB_XHCI_INSTANCE   *Xhc,
>    IN  TRB_TEMPLATE        *Trb,
> -  OUT URB                 **Urb
> +  IN  URB                 *Urb
>    )
>  {
> -  LIST_ENTRY              *Entry;
> -  LIST_ENTRY              *Next;
> -  TRB_TEMPLATE            *CheckedTrb;
> -  URB                     *CheckedUrb;
> -  UINTN                   Index;
> +  LINK_TRB      *LinkTrb;
> +  TRB_TEMPLATE  *CheckedTrb;
> +  UINTN         Index;
> +  EFI_PHYSICAL_ADDRESS PhyAddr;
> 
> -  EFI_LIST_FOR_EACH_SAFE (Entry, Next, &Xhc->AsyncIntTransfers) {
> -    CheckedUrb = EFI_LIST_CONTAINER (Entry, URB, UrbList);
> -    CheckedTrb = CheckedUrb->TrbStart;
> -    for (Index = 0; Index < CheckedUrb->TrbNum; Index++) {
> -      if (Trb == CheckedTrb) {
> -        *Urb = CheckedUrb;
> -        return TRUE;
> -      }
> -      CheckedTrb++;
> -      //
> -      // If the checked TRB is the link TRB at the end of the transfer ring,
> -      // recircle it to the head of the ring.
> -      //
> -      if (CheckedTrb->Type == TRB_TYPE_LINK) {
> -        CheckedTrb = (TRB_TEMPLATE*) CheckedUrb->Ring->RingSeg0;
> -      }
> +  CheckedTrb = Urb->TrbStart;
> +  if (CheckedTrb->Type == TRB_TYPE_NORMAL) {
> +    ASSERT (Urb->TrbNum == 1);
> +  }

Is the above if-ASSERT check needed here? I think it's possible for the
'Urb->TrbNum' to be greater than 1 when 'Urb->DataLen' is larger than 0x10000
for Bulk and Interrupt transter (whose trb type will be TRB_TYPE_NORMAL).

> +  for (Index = 0; Index < Urb->TrbNum; Index++) {
> +    if (Trb == CheckedTrb) {
> +      return TRUE;
> +    }
> +    CheckedTrb++;
> +    //
> +    // If the checked TRB is the link TRB at the end of the transfer ring,
> +    // recircle it to the head of the ring.
> +    //
> +    if (CheckedTrb->Type == TRB_TYPE_LINK) {
> +      LinkTrb = (LINK_TRB *) CheckedTrb;
> +      PhyAddr = (EFI_PHYSICAL_ADDRESS)(LinkTrb->PtrLo | LShiftU64 ((UINT64)
> LinkTrb->PtrHi, 32));
> +      CheckedTrb = (TRB_TEMPLATE *)(UINTN) UsbHcGetHostAddrForPciAddr
> (Xhc->MemPool, (VOID *)(UINTN) PhyAddr, sizeof (TRB_TEMPLATE));
> +      ASSERT (CheckedTrb == Urb->Ring->RingSeg0);
>      }
>    }
> 
> @@ -1023,38 +1023,39 @@ IsAsyncIntTrb (
>  }
> 
>  /**
> -  Check if the Trb is a transaction of the URB.
> +  Check if the Trb is a transaction of the URBs in XHCI's asynchronous transfer
> list.
> 
> -  @param Trb    The TRB to be checked
> -  @param Urb    The transfer ring to be checked.
> +  @param Xhc    The XHCI Instance.
> +  @param Trb    The TRB to be checked.
> +  @param Urb    The pointer to the matched Urb.
> 
> -  @retval TRUE  It is a transaction of the URB.
> -  @retval FALSE It is not any transaction of the URB.
> +  @retval TRUE  The Trb is matched with a transaction of the URBs in the
> async list.
> +  @retval FALSE The Trb is not matched with any URBs in the async list.
> 
>  **/
>  BOOLEAN
> -IsTransferRingTrb (
> +IsAsyncIntTrb (
> +  IN  USB_XHCI_INSTANCE   *Xhc,
>    IN  TRB_TEMPLATE        *Trb,
> -  IN  URB                 *Urb
> +  OUT URB                 **Urb
>    )
>  {
> -  TRB_TEMPLATE  *CheckedTrb;
> -  UINTN         Index;
> -
> -  CheckedTrb = Urb->Ring->RingSeg0;
> -
> -  ASSERT (Urb->Ring->TrbNumber == CMD_RING_TRB_NUMBER || Urb->Ring-
> >TrbNumber == TR_RING_TRB_NUMBER);
> +  LIST_ENTRY              *Entry;
> +  LIST_ENTRY              *Next;
> +  URB                     *CheckedUrb;
> 
> -  for (Index = 0; Index < Urb->Ring->TrbNumber; Index++) {
> -    if (Trb == CheckedTrb) {
> +  EFI_LIST_FOR_EACH_SAFE (Entry, Next, &Xhc->AsyncIntTransfers) {
> +    CheckedUrb = EFI_LIST_CONTAINER (Entry, URB, UrbList);
> +    if (IsTransferRingTrb (Xhc, Trb, CheckedUrb)) {
> +      *Urb = CheckedUrb;
>        return TRUE;
>      }
> -    CheckedTrb++;
>    }
> 
>    return FALSE;
>  }
> 
> +
>  /**
>    Check the URB's execution result and update the URB's
>    result accordingly.
> @@ -1131,7 +1132,7 @@ XhcCheckUrbResult (
>      // This way is used to avoid that those completed async transfer events don't
> get
>      // handled in time and are flushed by newer coming events.
>      //
> -    if (IsTransferRingTrb (TRBPtr, Urb)) {
> +    if (IsTransferRingTrb (Xhc, TRBPtr, Urb)) {
>        CheckedUrb = Urb;
>      } else if (IsAsyncIntTrb (Xhc, TRBPtr, &AsyncUrb)) {
>        CheckedUrb = AsyncUrb;
> --
> 2.12.2.windows.2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel