* [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the description table after Reset Device
@ 2020-05-09 8:26 Guomin Jiang
2020-05-11 5:25 ` Wu, Hao A
2020-07-15 0:18 ` Jeremy Linton
0 siblings, 2 replies; 9+ messages in thread
From: Guomin Jiang @ 2020-05-09 8:26 UTC (permalink / raw)
To: devel; +Cc: jeremy.linton, Jian J Wang, Hao A Wu, Ray Ni
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2694
When the USB fail and then Reset Device, it should rebuild description.
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);
+ }
+ }
+
+ 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the description table after Reset Device
2020-05-09 8:26 [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the description table after Reset Device Guomin Jiang
@ 2020-05-11 5:25 ` Wu, Hao A
2020-05-11 8:13 ` Guomin Jiang
2020-07-15 0:18 ` Jeremy Linton
1 sibling, 1 reply; 9+ messages in thread
From: Wu, Hao A @ 2020-05-11 5:25 UTC (permalink / raw)
To: Jiang, Guomin, devel@edk2.groups.io
Cc: jeremy.linton@arm.com, Wang, Jian J, Ni, Ray
> -----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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the description table after Reset Device
2020-05-11 5:25 ` Wu, Hao A
@ 2020-05-11 8:13 ` Guomin Jiang
0 siblings, 0 replies; 9+ messages in thread
From: Guomin Jiang @ 2020-05-11 8:13 UTC (permalink / raw)
To: Wu, Hao A, devel@edk2.groups.io
Cc: jeremy.linton@arm.com, Wang, Jian J, Ni, Ray
Hi Hao,
Please see the comments inline.
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Monday, May 11, 2020 1:25 PM
> To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io
> Cc: jeremy.linton@arm.com; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray
> <ray.ni@intel.com>
> Subject: RE: [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the
> description table after Reset Device
>
> > -----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.
I will add the brief summary in next version patch.
>
> 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.
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the description table after Reset Device
2020-05-09 8:26 [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the description table after Reset Device Guomin Jiang
2020-05-11 5:25 ` Wu, Hao A
@ 2020-07-15 0:18 ` Jeremy Linton
2020-07-15 1:07 ` Guomin Jiang
1 sibling, 1 reply; 9+ messages in thread
From: Jeremy Linton @ 2020-07-15 0:18 UTC (permalink / raw)
To: Guomin Jiang, devel; +Cc: Jian J Wang, Hao A Wu, Ray Ni
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.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);
> + }
> + }
> +
> + 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.
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the description table after Reset Device
2020-07-15 0:18 ` Jeremy Linton
@ 2020-07-15 1:07 ` Guomin Jiang
2020-07-15 14:48 ` Jeremy Linton
0 siblings, 1 reply; 9+ messages in thread
From: Guomin Jiang @ 2020-07-15 1:07 UTC (permalink / raw)
To: Jeremy Linton, devel@edk2.groups.io; +Cc: Wang, Jian J, Wu, Hao A, Ni, Ray
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.
Best Regards
Guomin
> -----Original Message-----
> From: Jeremy Linton <jeremy.linton@arm.com>
> Sent: Wednesday, July 15, 2020 8:19 AM
> To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@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.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);
> > + }
> > + }
> > +
> > + 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.
> >
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the description table after Reset Device
2020-07-15 1:07 ` Guomin Jiang
@ 2020-07-15 14:48 ` Jeremy Linton
2020-07-16 6:05 ` [edk2-devel] " Guomin Jiang
0 siblings, 1 reply; 9+ messages in thread
From: Jeremy Linton @ 2020-07-15 14:48 UTC (permalink / raw)
To: Jiang, Guomin, devel@edk2.groups.io; +Cc: Wang, Jian J, Wu, Hao A, Ni, Ray
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.linton@arm.com>
>> Sent: Wednesday, July 15, 2020 8:19 AM
>> To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io
>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
>> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@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.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);
>>> + }
>>> + }
>>> +
>>> + 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.
>>>
>>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the description table after Reset Device
2020-07-15 14:48 ` Jeremy Linton
@ 2020-07-16 6:05 ` Guomin Jiang
2020-12-09 12:38 ` Srikumar Subramanian
0 siblings, 1 reply; 9+ messages in thread
From: Guomin Jiang @ 2020-07-16 6:05 UTC (permalink / raw)
To: devel@edk2.groups.io, jeremy.linton@arm.com
Cc: Wang, Jian J, Wu, Hao A, Ni, Ray
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.jiang@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Ni, Ray <ray.ni@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.linton@arm.com>
> >> Sent: Wednesday, July 15, 2020 8:19 AM
> >> To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io
> >> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> >> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@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.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);
> >>> + }
> >>> + }
> >>> +
> >>> + 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.
> >>>
> >>>
> >
>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the description table after Reset Device
2020-07-16 6:05 ` [edk2-devel] " Guomin Jiang
@ 2020-12-09 12:38 ` Srikumar Subramanian
2022-11-18 22:54 ` [STILL CRASHING] " Jeremy Linton
0 siblings, 1 reply; 9+ messages in thread
From: Srikumar Subramanian @ 2020-12-09 12:38 UTC (permalink / raw)
To: Guomin Jiang, devel
[-- Attachment #1: Type: text/plain, Size: 10242 bytes --]
Hi Jiang,
Is it the responsibility of the reset routine to know what is wrong with the device? And on this topic I would like to know your thoughts on another alternative which is saving the config in XhcPollPortStatusChange before disabling the slot and restoring it after initializing the device slot again. My rationale for doing this is that, the reset device state diagram according to the specs moves the device from the configured to the default state and does not completely remove device information.
In this regard is it safe to assume that meta-data such as descriptors should be restored. Since in this case a port reset is triggering a device reset but because we disable and later initialize a new slot for an existing device we lose its configuration which we assume does not change.
Let me know your thoughts and inputs on this. And if I needed an immediate solution I would like to know the better option between the one I am proposing right now and the existing suggested solution (which is making a request to the device again for the descriptors.)
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
index 1754a97..d330f21 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
@@ -1,5 +1,5 @@
/**
**/
/** @file
@@ -1587,6 +1587,65 @@ XhcMonitorAsyncRequests (
gBS->RestoreTPL (OldTpl);
}
+/**
+ Save the current configuration for the Slot in Host Controller
+
+ @param Xhc The XHCI instance
+ @param SlotId The Slot ID
+ @param ActiveAlternateSetting The Alternate interface setting that is part of the device context
+ @param ConfDesc The Configuration Descriptors of the device present in the slot
+ @param DevDesc The Device Descriptor of the device present in the slot
+**/
+VOID
+XhcSaveConfig(
+ IN USB_XHCI_INSTANCE *Xhc,
+ IN UINT8 SlotId,
+ OUT UINT8 **ActiveAlternateSetting,
+ OUT EFI_USB_CONFIG_DESCRIPTOR ***ConfDesc,
+ OUT EFI_USB_DEVICE_DESCRIPTOR *DevDesc
+)
+{
+ UINT8 Index;
+ UINT8 IfIndex;
+ UINT8 NumInterfaces;
+ UINT64 ConfSize;
+ EFI_USB_INTERFACE_DESCRIPTOR *IfDesc;
+
+ *DevDesc = Xhc->UsbDevContext[SlotId].DevDesc;
+
+ if(DevDesc->NumConfigurations==0){
+ return;
+ }
+
+ *ConfDesc = AllocateZeroPool(DevDesc->NumConfigurations * sizeof(EFI_USB_CONFIG_DESCRIPTOR*));
+
+ for(Index = 0; Index < DevDesc->NumConfigurations; Index++){
+ NumInterfaces = Xhc->UsbDevContext[SlotId].ConfDesc[Index]->NumInterfaces;
+ IfDesc = (EFI_USB_INTERFACE_DESCRIPTOR *)(Xhc->UsbDevContext[SlotId].ConfDesc[Index] + 1);
+
+ // Calculating the size of the configuration descriptor.
+ // This is needed since the interface and enpoint descriptors are nested within this.
+ // See USB_CONFIG_DESCRIPTOR, USB_INTERFACE_DESCRIPTOR, USB_ENDPOINT_DESCRIPTOR in MdePkg/Include/IndustryStandard/Usb.h
+ // See USB_CONFIG_DESC, USB_INTERFACE_DESC, USB_ENDPOINT_DESC in MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.h
+ ConfSize = sizeof(Xhc->UsbDevContext[SlotId].ConfDesc[Index]);
+ ConfSize += (sizeof(EFI_USB_INTERFACE_DESCRIPTOR) * (NumInterfaces));
+ for(IfIndex = 0; IfIndex < NumInterfaces; IfIndex++){
+ ConfSize += IfDesc->NumEndpoints * sizeof(EFI_USB_ENDPOINT_DESCRIPTOR);
+ IfDesc = (USB_INTERFACE_DESCRIPTOR *)((UINTN)IfDesc + IfDesc->Length);
+ }
+
+ *ConfDesc[Index] = AllocateZeroPool(ConfSize);
+ CopyMem (*ConfDesc[Index] ,Xhc->UsbDevContext[SlotId].ConfDesc[Index], ConfSize);
+ }
+
+ *ActiveAlternateSetting = AllocateZeroPool(Xhc->UsbDevContext[SlotId].ConfDesc[0]->NumInterfaces * sizeof(UINT8));
+
+ CopyMem(*ActiveAlternateSetting, Xhc->UsbDevContext[SlotId].ActiveAlternateSetting, Xhc->UsbDevContext[SlotId].ConfDesc[0]->NumInterfaces * sizeof(UINT8));
+
+}
+
/**
Monitor the port status change. Enable/Disable device slot if there is a device attached/detached.
@@ -1613,6 +1672,15 @@ XhcPollPortStatusChange (
UINT8 SlotId;
USB_DEV_ROUTE RouteChart;
+ EFI_USB_CONFIG_DESCRIPTOR **ConfDescBuffer;
+ EFI_USB_DEVICE_DESCRIPTOR DevDescBuffer;
+ UINT8 *ActiveAlternateSettingBuffer;
+
+ ConfDescBuffer = NULL;
+ ActiveAlternateSettingBuffer = NULL;
+
Status = EFI_SUCCESS;
if ((PortState->PortChangeStatus & (USB_PORT_STAT_C_CONNECTION | USB_PORT_STAT_C_ENABLE | USB_PORT_STAT_C_OVERCURRENT | USB_PORT_STAT_C_RESET)) == 0) {
@@ -1635,6 +1703,14 @@ XhcPollPortStatusChange (
SlotId = XhcRouteStringToSlotId (Xhc, RouteChart);
if (SlotId != 0) {
+
+ // Saving the current host controller configuration for reuse after device reset
+ if((PortState->PortChangeStatus & USB_PORT_STAT_C_RESET) != 0){
+ XhcSaveConfig(Xhc, SlotId, &ActiveAlternateSettingBuffer, &ConfDescBuffer, &DevDescBuffer);
+ }
+
if (Xhc->HcCParams.Data.Csz == 0) {
Status = XhcDisableSlotCmd (Xhc, SlotId);
} else {
@@ -1660,11 +1736,23 @@ XhcPollPortStatusChange (
//
SlotId = XhcRouteStringToSlotId (Xhc, RouteChart);
if ((SlotId == 0) && ((PortState->PortChangeStatus & USB_PORT_STAT_C_RESET) != 0)) {
+
+
+ // Introducing the SlotId as a new parameter in XhcInitializeDeviceSlot in order to get the
+ // new slotId assigned.
if (Xhc->HcCParams.Data.Csz == 0) {
- Status = XhcInitializeDeviceSlot (Xhc, ParentRouteChart, Port, RouteChart, Speed);
+ Status = XhcInitializeDeviceSlot (Xhc, ParentRouteChart, Port, RouteChart, Speed, &SlotId);
} else {
- Status = XhcInitializeDeviceSlot64 (Xhc, ParentRouteChart, Port, RouteChart, Speed);
+ Status = XhcInitializeDeviceSlot64 (Xhc, ParentRouteChart, Port, RouteChart, Speed, &SlotId);
}
+
+ if(!EFI_ERROR(Status) && ConfDescBuffer!=NULL && SlotId!=0){
+ Xhc->UsbDevContext[SlotId].DevDesc = DevDescBuffer;
+ Xhc->UsbDevContext[SlotId].ConfDesc = ConfDescBuffer;
+ Xhc->UsbDevContext[SlotId].ActiveAlternateSetting = ActiveAlternateSettingBuffer;
+ }
+
}
}
@@ -1987,7 +2075,11 @@ XhcInitializeDeviceSlot (
IN USB_DEV_ROUTE ParentRouteChart,
IN UINT16 ParentPort,
IN USB_DEV_ROUTE RouteChart,
- IN UINT8 DeviceSpeed
+ IN UINT8 DeviceSpeed,
+ // OPTIONAL as this need not be used
+ OPTIONAL OUT UINT8 *SlotIdStore
)
{
EFI_STATUS Status;
@@ -2175,6 +2267,12 @@ XhcInitializeDeviceSlot (
Xhc->UsbDevContext[SlotId].XhciDevAddr = DeviceAddress;
}
+ if(SlotIdStore != NULL){
+ *SlotIdStore = SlotId;
+ }
+
return Status;
}
@@ -2197,7 +2295,11 @@ XhcInitializeDeviceSlot64 (
IN USB_DEV_ROUTE ParentRouteChart,
IN UINT16 ParentPort,
IN USB_DEV_ROUTE RouteChart,
- IN UINT8 DeviceSpeed
+ IN UINT8 DeviceSpeed,
+ // OPTIONAL as this need not be used
+ OPTIONAL OUT UINT8 *SlotIdStore
)
{
EFI_STATUS Status;
@@ -2384,6 +2486,13 @@ XhcInitializeDeviceSlot64 (
DEBUG ((EFI_D_INFO, " Address %d assigned successfully\n", DeviceAddress));
Xhc->UsbDevContext[SlotId].XhciDevAddr = DeviceAddress;
}
+
+ if(SlotIdStore != NULL){
+ *SlotIdStore = SlotId;
+ }
+
return Status;
}
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
index 4da9669..64ef878 100644
--- a/PurleyPlatPkg/Override/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
+++ b/PurleyPlatPkg/Override/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
@@ -1,3 +1,6 @@
+/**
+* CONFIDENTIAL (C) Copyright 2020 Hewlett Packard Enterprise Development LP<BR>
+**/
/** @file
This file contains the definition for XHCI host controller schedule routines.
@@ -1120,7 +1123,11 @@ XhcInitializeDeviceSlot (
IN USB_DEV_ROUTE ParentRouteChart,
IN UINT16 ParentPort,
IN USB_DEV_ROUTE RouteChart,
- IN UINT8 DeviceSpeed
+ IN UINT8 DeviceSpeed,
+ // HPE_EXL_SS_QXCR1001775642
+ // OPTIONAL as this need not be used
+ OPTIONAL OUT UINT8 *SlotIdStore
+ // HPE_EXL_SS_QXCR1001775642
);
/**
@@ -1142,7 +1149,11 @@ XhcInitializeDeviceSlot64 (
IN USB_DEV_ROUTE ParentRouteChart,
IN UINT16 ParentPort,
IN USB_DEV_ROUTE RouteChart,
- IN UINT8 DeviceSpeed
+ IN UINT8 DeviceSpeed,
+ // OPTIONAL as this need not be used
+ OPTIONAL OUT UINT8 *SlotIdStore
);
/**
@@ -1458,4 +1469,24 @@ XhcCreateTransferTrb (
IN URB *Urb
);
+/**
+ Save the current configuration for the Slot in Host Controller
+
+ @param Xhc The XHCI instance
+ @param SlotId The Slot ID
+ @param ActiveAlternateSetting The Alternate interface setting that is part of the device context
+ @param ConfDesc The Configuration Descriptors of the device present in the slot
+ @param DevDesc The Device Descriptor of the device present in the slot
+**/
+VOID
+XhcSaveConfig(
+ IN USB_XHCI_INSTANCE *Xhc,
+ IN UINT8 SlotId,
+ OUT UINT8 **ActiveAlternateSetting,
+ OUT EFI_USB_CONFIG_DESCRIPTOR ***ConfDesc,
+ OUT EFI_USB_DEVICE_DESCRIPTOR *DevDesc
+);
+
[-- Attachment #2: Type: text/html, Size: 16485 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [STILL CRASHING] Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the description table after Reset Device
2020-12-09 12:38 ` Srikumar Subramanian
@ 2022-11-18 22:54 ` Jeremy Linton
0 siblings, 0 replies; 9+ messages in thread
From: Jeremy Linton @ 2022-11-18 22:54 UTC (permalink / raw)
To: devel, srikumar-subramanian, Guomin Jiang
Hi,
Dead thread poke, but, its been > 2 years since this problem was pointed
out.
Multiple patches have been proposed, including one by me:
https://edk2.groups.io/g/devel/message/58193
and the USB/XHCI reset logic continues to be broken enough that a wide
range of USB issues can result in boot crashes. Those crashes are the
result of a null pointer in the TrsRing being dereferenced following
reset. In fact it is 100% reproducible on seemingly any edk2 platform
with a XHCI controller given the right USB device (like the really
common JMICRON I pointed out in one of the threads).
I'm not sure this patch is right, because the USB descriptors shouldn't
be cached over reset, but something has to be done to fix this problem.
Thanks,
On 12/9/20 06:38, Srikumar Subramanian via groups.io wrote:
> Hi Jiang,
>
> Is it the responsibility of the reset routine to know what is wrong with the device? And on this topic I would like to know your thoughts on another alternative which is saving the config in XhcPollPortStatusChange before disabling the slot and restoring it after initializing the device slot again. My rationale for doing this is that, the reset device state diagram according to the specs moves the device from the configured to the default state and does not completely remove device information.
> In this regard is it safe to assume that meta-data such as descriptors should be restored. Since in this case a port reset is triggering a device reset but because we disable and later initialize a new slot for an existing device we lose its configuration which we assume does not change.
>
> Let me know your thoughts and inputs on this. And if I needed an immediate solution I would like to know the better option between the one I am proposing right now and the existing suggested solution (which is making a request to the device again for the descriptors.)
>
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> index 1754a97..d330f21 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> @@ -1,5 +1,5 @@
> /**
>
> **/
> /** @file
>
> @@ -1587,6 +1587,65 @@ XhcMonitorAsyncRequests (
> gBS->RestoreTPL (OldTpl);
> }
>
> +/**
> + Save the current configuration for the Slot in Host Controller
> +
> + @param Xhc The XHCI instance
> + @param SlotId The Slot ID
> + @param ActiveAlternateSetting The Alternate interface setting that is part of the device context
> + @param ConfDesc The Configuration Descriptors of the device present in the slot
> + @param DevDesc The Device Descriptor of the device present in the slot
> +**/
> +VOID
> +XhcSaveConfig(
> + IN USB_XHCI_INSTANCE *Xhc,
> + IN UINT8 SlotId,
> + OUT UINT8 **ActiveAlternateSetting,
> + OUT EFI_USB_CONFIG_DESCRIPTOR ***ConfDesc,
> + OUT EFI_USB_DEVICE_DESCRIPTOR *DevDesc
> +)
> +{
> + UINT8 Index;
> + UINT8 IfIndex;
> + UINT8 NumInterfaces;
> + UINT64 ConfSize;
> + EFI_USB_INTERFACE_DESCRIPTOR *IfDesc;
> +
> + *DevDesc = Xhc->UsbDevContext[SlotId].DevDesc;
> +
> + if(DevDesc->NumConfigurations==0){
> + return;
> + }
> +
> + *ConfDesc = AllocateZeroPool(DevDesc->NumConfigurations * sizeof(EFI_USB_CONFIG_DESCRIPTOR*));
> +
> + for(Index = 0; Index < DevDesc->NumConfigurations; Index++){
> + NumInterfaces = Xhc->UsbDevContext[SlotId].ConfDesc[Index]->NumInterfaces;
> + IfDesc = (EFI_USB_INTERFACE_DESCRIPTOR *)(Xhc->UsbDevContext[SlotId].ConfDesc[Index] + 1);
> +
> + // Calculating the size of the configuration descriptor.
> + // This is needed since the interface and enpoint descriptors are nested within this.
> + // See USB_CONFIG_DESCRIPTOR, USB_INTERFACE_DESCRIPTOR, USB_ENDPOINT_DESCRIPTOR in MdePkg/Include/IndustryStandard/Usb.h
> + // See USB_CONFIG_DESC, USB_INTERFACE_DESC, USB_ENDPOINT_DESC in MdeModulePkg/Bus/Usb/UsbBusDxe/UsbDesc.h
> + ConfSize = sizeof(Xhc->UsbDevContext[SlotId].ConfDesc[Index]);
> + ConfSize += (sizeof(EFI_USB_INTERFACE_DESCRIPTOR) * (NumInterfaces));
> + for(IfIndex = 0; IfIndex < NumInterfaces; IfIndex++){
> + ConfSize += IfDesc->NumEndpoints * sizeof(EFI_USB_ENDPOINT_DESCRIPTOR);
> + IfDesc = (USB_INTERFACE_DESCRIPTOR *)((UINTN)IfDesc + IfDesc->Length);
> + }
> +
> + *ConfDesc[Index] = AllocateZeroPool(ConfSize);
> + CopyMem (*ConfDesc[Index] ,Xhc->UsbDevContext[SlotId].ConfDesc[Index], ConfSize);
> + }
> +
> + *ActiveAlternateSetting = AllocateZeroPool(Xhc->UsbDevContext[SlotId].ConfDesc[0]->NumInterfaces * sizeof(UINT8));
> +
> + CopyMem(*ActiveAlternateSetting, Xhc->UsbDevContext[SlotId].ActiveAlternateSetting, Xhc->UsbDevContext[SlotId].ConfDesc[0]->NumInterfaces * sizeof(UINT8));
> +
> +}
> +
> /**
> Monitor the port status change. Enable/Disable device slot if there is a device attached/detached.
>
> @@ -1613,6 +1672,15 @@ XhcPollPortStatusChange (
> UINT8 SlotId;
> USB_DEV_ROUTE RouteChart;
>
> + EFI_USB_CONFIG_DESCRIPTOR **ConfDescBuffer;
> + EFI_USB_DEVICE_DESCRIPTOR DevDescBuffer;
> + UINT8 *ActiveAlternateSettingBuffer;
> +
> + ConfDescBuffer = NULL;
> + ActiveAlternateSettingBuffer = NULL;
> +
> Status = EFI_SUCCESS;
>
> if ((PortState->PortChangeStatus & (USB_PORT_STAT_C_CONNECTION | USB_PORT_STAT_C_ENABLE | USB_PORT_STAT_C_OVERCURRENT | USB_PORT_STAT_C_RESET)) == 0) {
> @@ -1635,6 +1703,14 @@ XhcPollPortStatusChange (
>
> SlotId = XhcRouteStringToSlotId (Xhc, RouteChart);
> if (SlotId != 0) {
> +
> + // Saving the current host controller configuration for reuse after device reset
> + if((PortState->PortChangeStatus & USB_PORT_STAT_C_RESET) != 0){
> + XhcSaveConfig(Xhc, SlotId, &ActiveAlternateSettingBuffer, &ConfDescBuffer, &DevDescBuffer);
> + }
> +
> if (Xhc->HcCParams.Data.Csz == 0) {
> Status = XhcDisableSlotCmd (Xhc, SlotId);
> } else {
> @@ -1660,11 +1736,23 @@ XhcPollPortStatusChange (
> //
> SlotId = XhcRouteStringToSlotId (Xhc, RouteChart);
> if ((SlotId == 0) && ((PortState->PortChangeStatus & USB_PORT_STAT_C_RESET) != 0)) {
> +
> +
> + // Introducing the SlotId as a new parameter in XhcInitializeDeviceSlot in order to get the
> + // new slotId assigned.
> if (Xhc->HcCParams.Data.Csz == 0) {
> - Status = XhcInitializeDeviceSlot (Xhc, ParentRouteChart, Port, RouteChart, Speed);
> + Status = XhcInitializeDeviceSlot (Xhc, ParentRouteChart, Port, RouteChart, Speed, &SlotId);
> } else {
> - Status = XhcInitializeDeviceSlot64 (Xhc, ParentRouteChart, Port, RouteChart, Speed);
> + Status = XhcInitializeDeviceSlot64 (Xhc, ParentRouteChart, Port, RouteChart, Speed, &SlotId);
> }
> +
> + if(!EFI_ERROR(Status) && ConfDescBuffer!=NULL && SlotId!=0){
> + Xhc->UsbDevContext[SlotId].DevDesc = DevDescBuffer;
> + Xhc->UsbDevContext[SlotId].ConfDesc = ConfDescBuffer;
> + Xhc->UsbDevContext[SlotId].ActiveAlternateSetting = ActiveAlternateSettingBuffer;
> + }
>
> +
> }
> }
>
> @@ -1987,7 +2075,11 @@ XhcInitializeDeviceSlot (
> IN USB_DEV_ROUTE ParentRouteChart,
> IN UINT16 ParentPort,
> IN USB_DEV_ROUTE RouteChart,
> - IN UINT8 DeviceSpeed
> + IN UINT8 DeviceSpeed,
>
> + // OPTIONAL as this need not be used
> + OPTIONAL OUT UINT8 *SlotIdStore
>
> )
> {
> EFI_STATUS Status;
> @@ -2175,6 +2267,12 @@ XhcInitializeDeviceSlot (
> Xhc->UsbDevContext[SlotId].XhciDevAddr = DeviceAddress;
> }
>
> + if(SlotIdStore != NULL){
> + *SlotIdStore = SlotId;
> + }
> +
> return Status;
> }
>
> @@ -2197,7 +2295,11 @@ XhcInitializeDeviceSlot64 (
> IN USB_DEV_ROUTE ParentRouteChart,
> IN UINT16 ParentPort,
> IN USB_DEV_ROUTE RouteChart,
> - IN UINT8 DeviceSpeed
> + IN UINT8 DeviceSpeed,
>
> + // OPTIONAL as this need not be used
> + OPTIONAL OUT UINT8 *SlotIdStore
>
> )
> {
> EFI_STATUS Status;
> @@ -2384,6 +2486,13 @@ XhcInitializeDeviceSlot64 (
> DEBUG ((EFI_D_INFO, " Address %d assigned successfully\n", DeviceAddress));
> Xhc->UsbDevContext[SlotId].XhciDevAddr = DeviceAddress;
> }
> +
>
> + if(SlotIdStore != NULL){
> + *SlotIdStore = SlotId;
> + }
>
> +
> return Status;
> }
>
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> index 4da9669..64ef878 100644
> --- a/PurleyPlatPkg/Override/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> +++ b/PurleyPlatPkg/Override/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> @@ -1,3 +1,6 @@
> +/**
> +* CONFIDENTIAL (C) Copyright 2020 Hewlett Packard Enterprise Development LP<BR>
> +**/
> /** @file
>
> This file contains the definition for XHCI host controller schedule routines.
> @@ -1120,7 +1123,11 @@ XhcInitializeDeviceSlot (
> IN USB_DEV_ROUTE ParentRouteChart,
> IN UINT16 ParentPort,
> IN USB_DEV_ROUTE RouteChart,
> - IN UINT8 DeviceSpeed
> + IN UINT8 DeviceSpeed,
> + // HPE_EXL_SS_QXCR1001775642
> + // OPTIONAL as this need not be used
> + OPTIONAL OUT UINT8 *SlotIdStore
> + // HPE_EXL_SS_QXCR1001775642
> );
>
> /**
> @@ -1142,7 +1149,11 @@ XhcInitializeDeviceSlot64 (
> IN USB_DEV_ROUTE ParentRouteChart,
> IN UINT16 ParentPort,
> IN USB_DEV_ROUTE RouteChart,
> - IN UINT8 DeviceSpeed
> + IN UINT8 DeviceSpeed,
>
> + // OPTIONAL as this need not be used
> + OPTIONAL OUT UINT8 *SlotIdStore
>
> );
>
> /**
> @@ -1458,4 +1469,24 @@ XhcCreateTransferTrb (
> IN URB *Urb
> );
>
> +/**
> + Save the current configuration for the Slot in Host Controller
> +
> + @param Xhc The XHCI instance
> + @param SlotId The Slot ID
> + @param ActiveAlternateSetting The Alternate interface setting that is part of the device context
> + @param ConfDesc The Configuration Descriptors of the device present in the slot
> + @param DevDesc The Device Descriptor of the device present in the slot
> +**/
> +VOID
> +XhcSaveConfig(
> + IN USB_XHCI_INSTANCE *Xhc,
> + IN UINT8 SlotId,
> + OUT UINT8 **ActiveAlternateSetting,
> + OUT EFI_USB_CONFIG_DESCRIPTOR ***ConfDesc,
> + OUT EFI_USB_DEVICE_DESCRIPTOR *DevDesc
> +);
> +
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-11-18 22:54 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-09 8:26 [PATCH v3 1/1] MdeModulePkg/UsbBusDxe: Rebuild the description table after Reset Device Guomin Jiang
2020-05-11 5:25 ` Wu, Hao A
2020-05-11 8:13 ` Guomin Jiang
2020-07-15 0:18 ` Jeremy Linton
2020-07-15 1:07 ` Guomin Jiang
2020-07-15 14:48 ` Jeremy Linton
2020-07-16 6:05 ` [edk2-devel] " Guomin Jiang
2020-12-09 12:38 ` Srikumar Subramanian
2022-11-18 22:54 ` [STILL CRASHING] " Jeremy Linton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox