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
+);
+









-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96502): https://edk2.groups.io/g/devel/message/96502
Mute This Topic: https://groups.io/mt/95123697/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-


Reply via email to