Hi Jeremy, I know that you want to continue booting without ASSERT.
But what I care is that why the device error happened and if the change is the correct error handing. The change can make the ASSERT disappear, but the device still not work after it, so there are some other potential issue or the device itself have some error rather than firmware. So the change can't be checked in the master now. > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jeremy > Linton > Sent: Wednesday, July 15, 2020 10:48 PM > To: Jiang, Guomin <guomin.ji...@intel.com>; devel@edk2.groups.io > Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>; > Ni, Ray <ray...@intel.com> > Subject: Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: > Rebuild the description table after Reset Device > > > Hi, > > On 7/14/20 8:07 PM, Jiang, Guomin wrote: > > Hi Jeremy, > > > > We test many different device and found some device haven't reproduced > the issue. > > > > We need to figure out the root cause rather than work around. > > Yes, there are two problems in my case. The first is specific to the device, > in > that it can be put into a state (low power spun down IIRC) where the > individual > disks are responding with a 5/2400 (invalid command). But, in theory there > are a lot of cases like this where marginal cables/bad devices/etc, are going > to > result in errors. > > The second problem is this one, where any device that the subsystem thinks > needs resetting (as part of the recovery process) crashes the firmware. This > bug is pretty clear, the descriptor cache being maintained by XHCI is being > cleared, and the xhci driver itself is depending on that cache to handle > operations. Without fixing it, there are null pointer dereferences on just > about any error condition that can happen on the USB bus. This is a far larger > problem IMHO than whether or not a particular device is working. In my case, > I don't need/want the JBOD to be used by the firmware, but I do want the > machine to boot when that device is plugged in. > > So, either the usb subsystem uses the same (re)initialization sequence > following a reset as it does initially, or the XHCI driver needs to be > tweaked to > either not drop the descriptor cache during reset, or rebuild it when it > detects its missing. > > > Thanks, > > > > > > > Best Regards > > Guomin > >> -----Original Message----- > >> From: Jeremy Linton <jeremy.lin...@arm.com> > >> Sent: Wednesday, July 15, 2020 8:19 AM > >> To: Jiang, Guomin <guomin.ji...@intel.com>; devel@edk2.groups.io > >> Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A > >> <hao.a...@intel.com>; Ni, Ray <ray...@intel.com> > >> Subject: Re: [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the > >> description table after Reset Device > >> > >> Hi, > >> > >> On 5/9/20 3:26 AM, Guomin Jiang wrote: > >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2694 > >>> > >>> When the USB fail and then Reset Device, it should rebuild description. > >> > >> > >> I pulled the latest edk2 a few hours ago and I'm still seeing the > >> assert TrsRing != 0 messages on a real XHCI controller with the > >> jmicron JBOD I mentioned earlier. > >> > >> Is there a newer version of this patch? > >> > >> Thanks, > >> > >> > >> > >>> > >>> 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); > >>> + } > >>> + } > >>> + > >>> + 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. > >>> > >>> > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#62651): https://edk2.groups.io/g/devel/message/62651 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] -=-=-=-=-=-=-=-=-=-=-=-
