Re: [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the description table after Reset Device

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

> -----Original Message-----
> From: Jiang, Guomin <guomin.jiang@intel.com>
> Sent: Saturday, May 9, 2020 4:27 PM
> To: devel@edk2.groups.io
> Cc: jeremy.linton@arm.com; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the description
> table after Reset Device
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2694
> 
> When the USB fail and then Reset Device, it should rebuild description.


Hello Guomin,

Could you help to add a brief summary of the relationship between the proposed
fix and the issue reported in the above BZ tracker? It would be helpful for
clarifying the purpose of the commit. Thanks.

One more inline comments below:


> 
> Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
> 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>
> ---
>  MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c  |   7 ++
>  MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c | 152
> +++++++++++++++++++++++  MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.h
> |  14 +++
>  3 files changed, 173 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> index 4b4915c019ad..7bb9373a6adf 100644
> --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> @@ -874,6 +874,13 @@ UsbIoPortReset (
>    // is in CONFIGURED state.
>    //
>    if (Dev->ActiveConfig != NULL) {
> +    Status = UsbRebuildDescTable (Dev);
> +
> +    if (EFI_ERROR (Status)) {
> +      DEBUG (( DEBUG_ERROR, "UsbIoPortReset: failed to rebuild desc table -
>  %r\n", Status));
> +      goto ON_EXIT;
> +    }
> +
>      Status = UsbSetConfig (Dev, Dev->ActiveConfig->Desc.ConfigurationValue);
> 
>      if (EFI_ERROR (Status)) {
> diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
> b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
> index b08188b1bc78..d8e5e50b7c5a 100644
> --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
> +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.c
> @@ -900,6 +900,158 @@ UsbBuildDescTable (
>    return EFI_SUCCESS;
>  }
> 
> +/**
> +  Copy the interface descriptor
> +
> +  @param[out]  DescBuf               The buffer of raw descriptor.
> +  @param[in]   SrcBuf                The buffer of raw descriptor
> +**/
> +VOID
> +UsbCopyInterfaceDesc (
> +  OUT USB_INTERFACE_DESC  *DescBuf,
> +  IN  USB_INTERFACE_DESC  *SrcBuf
> +  )
> +{
> +  UINTN                   Index, IndexI;
> +
> +  ASSERT ((DescBuf != NULL) && (SrcBuf != NULL));
> +
> +  if (DescBuf->NumOfSetting == SrcBuf->NumOfSetting) {
> +    DescBuf->ActiveIndex = SrcBuf->ActiveIndex;
> +
> +    for (Index = 0; Index < DescBuf->NumOfSetting; Index++) {
> +      CopyMem (&DescBuf->Settings[Index]->Desc,
> +                &SrcBuf->Settings[Index]->Desc,
> +                sizeof(EFI_USB_INTERFACE_DESCRIPTOR));
> +
> +      if (DescBuf->Settings[Index]->Desc.NumEndpoints ==
> +           SrcBuf->Settings[Index]->Desc.NumEndpoints) {
> +
> +        for (IndexI = 0; IndexI < DescBuf->Settings[Index]->Desc.NumEndpoints;
> +          IndexI++) {
> +          CopyMem (DescBuf->Settings[Index]->Endpoints[IndexI],
> +                    SrcBuf->Settings[Index]->Endpoints[IndexI],
> +                    sizeof(USB_ENDPOINT_DESC));
> +        }
> +      }
> +    }
> +  }
> +}
> +
> +/**
> +  Copy the configuration descriptor and its interfaces.
> +
> +  @param[out]  DescBuf               The buffer of raw descriptor.
> +  @param[in]   SrcBuf                The buffer of raw descriptor
> +**/
> +VOID
> +UsbCopyConfigDesc (
> +  OUT USB_CONFIG_DESC   *DescBuf,
> +  IN  USB_CONFIG_DESC   *SrcBuf
> +  )
> +{
> +  UINTN                   Index;
> +
> +  ASSERT ((DescBuf != NULL) && (SrcBuf != NULL));
> +
> +  if (DescBuf->Desc.NumInterfaces == SrcBuf->Desc.NumInterfaces) {
> +    CopyMem (&DescBuf->Desc, &SrcBuf->Desc,
> + sizeof(EFI_USB_CONFIG_DESCRIPTOR));
> +
> +    for (Index = 0; Index < DescBuf->Desc.NumInterfaces; Index++) {
> +      UsbCopyInterfaceDesc (DescBuf->Interfaces[Index], SrcBuf-
> >Interfaces[Index]);
> +    }
> +  }
> +}
> +
> +/**
> +  Rebuild the whole array of descriptors.
> +
> +  @param[in]  UsbDev                The Usb device.
> +
> +  @retval EFI_SUCCESS           The descriptor table is build.
> +  @retval EFI_DEVICE_ERROR      Invalid config
> +  @retval Others                Command error when get descriptor.
> +**/
> +EFI_STATUS
> +UsbRebuildDescTable (
> +  IN USB_DEVICE           *UsbDev
> +  )
> +{
> +  EFI_USB_CONFIG_DESCRIPTOR *Config;
> +  USB_CONFIG_DESC           *ConfigDesc;
> +  UINT8                     NumConfig;
> +  EFI_STATUS                Status;
> +  UINT8                     Index;
> +
> +  //
> +  // Override the device descriptor, Current device fail but the
> + original  // descriptor pointer array is still exist.
> +  //
> +  Status  = UsbCtrlGetDesc (
> +              UsbDev,
> +              USB_DESC_TYPE_DEVICE,
> +              0,
> +              0,
> +              UsbDev->DevDesc,
> +              sizeof (EFI_USB_DEVICE_DESCRIPTOR)
> +              );
> +
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "UsbRebuildDescTable: failed to override device
> descriptor - %r\n", Status));
> +    return Status;
> +  }
> +
> +  NumConfig = UsbDev->DevDesc->Desc.NumConfigurations;
> +  if (NumConfig == 0) {
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "UsbReuildDescTable: device has %d configures\n",
> + NumConfig));
> +
> +  //
> +  // Read each configurations, then parse them  //  for (Index = 0;
> + Index < NumConfig; Index++) {
> +    Config = UsbGetOneConfig (UsbDev, Index);
> +
> +    if (Config == NULL) {
> +      DEBUG ((DEBUG_INFO, "UsbRebuildDescTable: failed to get configure
> + (index %d)\n", Index));
> +
> +      //
> +      // If we can get the default descriptor, it is likely that the
> +      // device is still operational.
> +      //
> +      if (Index == 0) {
> +        return EFI_DEVICE_ERROR;
> +      }
> +
> +      break;
> +    }
> +
> +    ConfigDesc = UsbParseConfigDesc ((UINT8 *) Config,
> + Config->TotalLength);
> +
> +    FreePool (Config);
> +
> +    if (ConfigDesc == NULL) {
> +      DEBUG ((DEBUG_ERROR, "UsbRebuildDescTable: failed to parse
> + configure (index %d)\n", Index));
> +
> +      //
> +      // If we can get the default descriptor, it is likely that the
> +      // device is still operational.
> +      //
> +      if (Index == 0) {
> +        return EFI_DEVICE_ERROR;
> +      }
> +
> +      break;
> +    } else {
> +      UsbCopyConfigDesc (UsbDev->DevDesc->Configs[Index], ConfigDesc);
> +      UsbFreeConfigDesc (ConfigDesc);


For the above codes in the 'else' block, how about:
1. Free the origin descriptor in UsbDev->DevDesc->Configs
2. Assign the new descriptor 'ConfigDesc' to UsbDev->DevDesc->Configs
By doing so, I think the memory copy of the descriptors can be skipped.

The reminder of the patch looks good to me.

Best Regards,
Hao Wu


> +    }
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> 
>  /**
>    Set the device's address.
> diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.h
> b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.h
> index 7b0c77fdc79c..54367133241a 100644
> --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.h
> +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.h
> @@ -169,6 +169,20 @@ UsbBuildDescTable (
>    IN USB_DEVICE           *UsbDev
>    );
> 
> +/**
> +  Rebuild the whole array of descriptors.
> +
> +  @param[in]  UsbDev                The Usb device.
> +
> +  @retval EFI_SUCCESS           The descriptor table is build.
> +  @retval EFI_DEVICE_ERROR      Invalid config
> +  @retval Others                Command error when get descriptor.
> +**/
> +EFI_STATUS
> +UsbRebuildDescTable (
> +  IN USB_DEVICE           *UsbDev
> +  );
> +
>  /**
>    Set the device's address.
> 
> --
> 2.25.1.windows.1