Re: [PATCH 1/4] MdeModulePkg XhciDxe: Extract new XhciInsertAsyncIntTransfer function

["Zeng, Star" <star.zeng@intel.com>]

On 2018/10/26 12:57, Ni, Ruiyu wrote:
> On 10/25/2018 6:58 PM, Star Zeng wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1274
>>
>> Extract new XhciInsertAsyncIntTransfer function from
>> XhcAsyncInterruptTransfer.
>>
>> It is code preparation for following patch,
>> no essential functional change.
>>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Hao Wu <hao.a.wu@intel.com>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Star Zeng <star.zeng@intel.com>
>> ---
>>   MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      | 18 +-------
>>   MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 74 
>> +++++++++++++++++++++++++++++---
>>   MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h | 28 ++++++++++++
>>   3 files changed, 98 insertions(+), 22 deletions(-)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c 
>> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>> index f1c60bef01c0..7f64f9c7c982 100644
>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>> @@ -1346,7 +1346,6 @@ XhcAsyncInterruptTransfer (
>>     EFI_STATUS              Status;
>>     UINT8                   SlotId;
>>     UINT8                   Index;
>> -  UINT8                   *Data;
>>     EFI_TPL                 OldTpl;
>>     //
>> @@ -1413,36 +1412,21 @@ XhcAsyncInterruptTransfer (
>>       goto ON_EXIT;
>>     }
>> -  Data = AllocateZeroPool (DataLength);
>> -
>> -  if (Data == NULL) {
>> -    DEBUG ((EFI_D_ERROR, "XhcAsyncInterruptTransfer: failed to 
>> allocate buffer\n"));
>> -    Status = EFI_OUT_OF_RESOURCES;
>> -    goto ON_EXIT;
>> -  }
>> -
>> -  Urb = XhcCreateUrb (
>> +  Urb = XhciInsertAsyncIntTransfer (
>>             Xhc,
>>             DeviceAddress,
>>             EndPointAddress,
>>             DeviceSpeed,
>>             MaximumPacketLength,
>> -          XHC_INT_TRANSFER_ASYNC,
>> -          NULL,
>> -          Data,
>>             DataLength,
>>             CallBackFunction,
>>             Context
>>             );
>> -
>>     if (Urb == NULL) {
>> -    DEBUG ((EFI_D_ERROR, "XhcAsyncInterruptTransfer: failed to create 
>> URB\n"));
>> -    FreePool (Data);
>>       Status = EFI_OUT_OF_RESOURCES;
>>       goto ON_EXIT;
>>     }
>> -  InsertHeadList (&Xhc->AsyncIntTransfers, &Urb->UrbList);
>>     //
>>     // Ring the doorbell
>>     //
>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c 
>> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
>> index 166c44bf5e66..2d7c08dc5bfa 100644
>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
>> @@ -264,11 +264,11 @@ XhcCreateTransferTrb (
>>     // No need to remap.
>>     //
>>     if ((Urb->Data != NULL) && (Urb->DataMap == NULL)) {
>> -    if (((UINT8) (Urb->Ep.Direction)) == EfiUsbDataIn) {
>> -      MapOp = EfiPciIoOperationBusMasterWrite;
>> -    } else {
>> -      MapOp = EfiPciIoOperationBusMasterRead;
>> -    }
>> +      if (((UINT8) (Urb->Ep.Direction)) == EfiUsbDataIn) {
>> +        MapOp = EfiPciIoOperationBusMasterWrite;
>> +      } else {
>> +        MapOp = EfiPciIoOperationBusMasterRead;
>> +      }
> 
> Unnecessary change, right?
> 
>>       Len = Urb->DataLen;
>>       Status  = Xhc->PciIo->Map (Xhc->PciIo, MapOp, Urb->Data, &Len, 
>> &PhyAddr, &Map);
>> @@ -1411,6 +1411,70 @@ XhciDelAllAsyncIntTransfers (
>>   }
>>   /**
>> +  Insert a single asynchronous interrupt transfer for
>> +  the device and endpoint.
>> +
>> +  @param Xhc            The XHCI Instance
>> +  @param BusAddr        The logical device address assigned by UsbBus 
>> driver
>> +  @param EpAddr         Endpoint addrress
>> +  @param DevSpeed       The device speed
>> +  @param MaxPacket      The max packet length of the endpoint
>> +  @param DataLen        The length of data buffer
>> +  @param Callback       The function to call when data is transferred
>> +  @param Context        The context to the callback
>> +
>> +  @return Created URB or NULL
>> +
>> +**/
>> +URB *
>> +XhciInsertAsyncIntTransfer (
>> +  IN USB_XHCI_INSTANCE                  *Xhc,
>> +  IN UINT8                              BusAddr,
>> +  IN UINT8                              EpAddr,
>> +  IN UINT8                              DevSpeed,
>> +  IN UINTN                              MaxPacket,
>> +  IN UINTN                              DataLen,
>> +  IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
>> +  IN VOID                               *Context
>> +  )
>> +{
>> +  VOID      *Data;
>> +  URB       *Urb;
>> +
>> +  Data = AllocateZeroPool (DataLen);
>> +  if (Data == NULL) {
>> +    DEBUG ((DEBUG_ERROR, "%a: failed to allocate buffer\n", 
>> __FUNCTION__));
>> +    return NULL;
>> +  }
>> +
>> +  Urb = XhcCreateUrb (
>> +          Xhc,
>> +          BusAddr,
>> +          EpAddr,
>> +          DevSpeed,
>> +          MaxPacket,
>> +          XHC_INT_TRANSFER_ASYNC,
>> +          NULL,
>> +          Data,
>> +          DataLen,
>> +          Callback,
>> +          Context
>> +          );
>> +  if (Urb == NULL) {
>> +    DEBUG ((DEBUG_ERROR, "%a: failed to create URB\n", __FUNCTION__));
> 
> FreePool (Data) is needed.

Oh yes, although the final code after this patch series does not need 
it, but this patch needs it. I need add it, same for patch 2. Thanks for 
catching it. :)

Thanks,
Star

> 
>> +    return NULL;
>> +  }
>> +
>> +  //
>> +  // New asynchronous transfer must inserted to the head.
>> +  // Check the comments in XhcMoniteAsyncRequests
>> +  //
>> +  InsertHeadList (&Xhc->AsyncIntTransfers, &Urb->UrbList);
>> +
>> +  return Urb;
>> +}
>> +
>> +/**
>>     Update the queue head for next round of asynchronous transfer
>>     @param  Xhc     The XHCI Instance.
>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h 
>> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
>> index 097408828a1f..cd1403f2842a 100644
>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
>> @@ -853,6 +853,34 @@ XhciDelAllAsyncIntTransfers (
>>     );
>>   /**
>> +  Insert a single asynchronous interrupt transfer for
>> +  the device and endpoint.
>> +
>> +  @param Xhc            The XHCI Instance
>> +  @param BusAddr        The logical device address assigned by UsbBus 
>> driver
>> +  @param EpAddr         Endpoint addrress
>> +  @param DevSpeed       The device speed
>> +  @param MaxPacket      The max packet length of the endpoint
>> +  @param DataLen        The length of data buffer
>> +  @param Callback       The function to call when data is transferred
>> +  @param Context        The context to the callback
>> +
>> +  @return Created URB or NULL
>> +
>> +**/
>> +URB *
>> +XhciInsertAsyncIntTransfer (
>> +  IN USB_XHCI_INSTANCE                  *Xhc,
>> +  IN UINT8                              DeviceAddress,
>> +  IN UINT8                              EndPointAddress,
>> +  IN UINT8                              DeviceSpeed,
>> +  IN UINTN                              MaximumPacketLength,
>> +  IN UINTN                              DataLength,
>> +  IN EFI_ASYNC_USB_TRANSFER_CALLBACK    CallBackFunction,
>> +  IN VOID                               *Context
>> +  );
>> +
>> +/**
>>     Set Bios Ownership
>>     @param  Xhc          The XHCI Instance.
>>
> 
>