Hi Hao, Please see the comments inline.
> -----Original Message----- > From: Wu, Hao A <hao.a...@intel.com> > Sent: Monday, May 11, 2020 1:25 PM > To: Jiang, Guomin <guomin.ji...@intel.com>; devel@edk2.groups.io > Cc: jeremy.lin...@arm.com; Wang, Jian J <jian.j.w...@intel.com>; Ni, Ray > <ray...@intel.com> > Subject: RE: [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the > description table after Reset Device > > > -----Original Message----- > > From: Jiang, Guomin <guomin.ji...@intel.com> > > Sent: Saturday, May 9, 2020 4:27 PM > > To: devel@edk2.groups.io > > Cc: jeremy.lin...@arm.com; Wang, Jian J <jian.j.w...@intel.com>; Wu, > > Hao A <hao.a...@intel.com>; Ni, Ray <ray...@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. I will add the brief summary in next version patch. > > One more inline comments below: > > > > > > Signed-off-by: Guomin Jiang <guomin.ji...@intel.com> > > Cc: Jian J Wang <jian.j.w...@intel.com> > > Cc: Hao A Wu <hao.a...@intel.com> > > Cc: Ray Ni <ray...@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. Current memory layout as below: UsbDev->DevDesc --> [many configs] --> Configs[0] --> [many interfaces] --> interface[0] --> [many endpoints] --> endpoint[0] --> endpoint[1] ... --> endpoint[NumEndpoints-1] --> Configs[1] --> [many interfaces] --> endpoint[0] --> endpoint[1] ... --> endpoint[NumEndpoints-1] ... --> Configs[NumConfig-1] --> [many interfaces] --> endpoint[0] --> endpoint[1] ... --> endpoint[NumEndpoints-1] Some code may refer the original interface and endpoint, if free it, it may be dereferenced. In previous practice, I use UsbFreeDevDesc() function to free the buffer but observe the issue. > > 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#59056): https://edk2.groups.io/g/devel/message/59056 Mute This Topic: https://groups.io/mt/74091912/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
