[PATCH 0/2] MdeModulePkg/UsbBus: Fix system hang when failed to uninstall UsbIo

[PATCH 0/2] MdeModulePkg/UsbBus: Fix system hang when failed to uninstall UsbIo

[Ruiyu Ni]

Please refer to first patch for details.
Patch 2/2 is to correct debug message to ease future debugging.

Ruiyu Ni (2):
  MdeModulePkg/UsbBus: Fix system hang when failed to uninstall UsbIo
  MdeModulePkg/UsbBus: Correct debug message

 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.h     |  3 +-
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c  | 87 +++++++----------------------
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.c | 17 +++---
 3 files changed, 27 insertions(+), 80 deletions(-)

-- 
2.12.2.windows.2

[PATCH 1/2] MdeModulePkg/UsbBus: Fix system hang when failed to uninstall UsbIo

[Ruiyu Ni]

When "reconnect -r" is typed in shell, UsbFreeInterface() is called
to uninstall the UsbIo and DevicePath. But When a UsbIo is opened
by a driver and that driver rejects to close the UsbIo in Stop(),
the uninstall doesn't succeed.
But UsbFreeInterface () frees the DevicePath memory without check
whether the uninstall succeeds.
It leads to the DXE core database contain a DevicePath instance but
that instance's memory is freed.
Assertion happens when someone calls InstallProtocol(DevicePath)
because the InstallProtocol() checks all DevicePath instance to
find whether the same one exits in database.

We haven't seen any USB device driver which rejects to close UsbIo
in Stop(), but it's very likely.

The patch removes IsManaged flag from USB_INTERFACE structure because
this flag cannot reflect the managing status of the USB interface.
When the USB BUS driver is started first time, it creates all UsbIo
instances but only call ConnectController() for those specified by
RemainingDevicePath. Later platform BDS may call ConnectController
for certain UsbIo instances, though some upper layer drivers are
started to mange the UsbIo instances, the IsManaged flag is still
FALSE because these drivers are not started by UsbBus driver.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.h     |  3 +-
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c  | 87 +++++++----------------------
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.c | 17 +++---
 3 files changed, 27 insertions(+), 80 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.h b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.h
index 9ede83ab7e..e8a55c584c 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.h
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.h
@@ -2,7 +2,7 @@
 
     Usb Bus Driver Binding and Bus IO Protocol.
 
-Copyright (c) 2004 - 2012, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2004 - 2017, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -216,7 +216,6 @@ struct _USB_INTERFACE {
   EFI_HANDLE                Handle;
   EFI_USB_IO_PROTOCOL       UsbIo;
   EFI_DEVICE_PATH_PROTOCOL  *DevicePath;
-  BOOLEAN                   IsManaged;
 
   //
   // Hub device special data
diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
index ea54d37c93..1514b63eb9 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
@@ -2,7 +2,7 @@
 
     Usb bus enumeration support.
 
-Copyright (c) 2007 - 2014, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2007 - 2017, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -59,21 +59,9 @@ UsbFreeInterface (
   IN USB_INTERFACE        *UsbIf
   )
 {
-  UsbCloseHostProtoByChild (UsbIf->Device->Bus, UsbIf->Handle);
-
-  gBS->UninstallMultipleProtocolInterfaces (
-         UsbIf->Handle,
-         &gEfiDevicePathProtocolGuid,
-         UsbIf->DevicePath,
-         &gEfiUsbIoProtocolGuid,
-         &UsbIf->UsbIo,
-         NULL
-         );
-
   if (UsbIf->DevicePath != NULL) {
     FreePool (UsbIf->DevicePath);
   }
-
   FreePool (UsbIf);
 }
 
@@ -279,13 +267,12 @@ UsbConnectDriver (
     // Only recursively wanted usb child device
     //
     if (UsbBusIsWantedUsbIO (UsbIf->Device->Bus, UsbIf)) {
-      OldTpl            = UsbGetCurrentTpl ();
+      OldTpl = UsbGetCurrentTpl ();
       DEBUG ((EFI_D_INFO, "UsbConnectDriver: TPL before connect is %d, %p\n", (UINT32)OldTpl, UsbIf->Handle));
 
       gBS->RestoreTPL (TPL_CALLBACK);
 
-      Status            = gBS->ConnectController (UsbIf->Handle, NULL, NULL, TRUE);
-      UsbIf->IsManaged  = (BOOLEAN)!EFI_ERROR (Status);
+      gBS->ConnectController (UsbIf->Handle, NULL, NULL, TRUE);
 
       DEBUG ((EFI_D_INFO, "UsbConnectDriver: TPL after connect is %d\n", (UINT32)UsbGetCurrentTpl()));
       ASSERT (UsbGetCurrentTpl () == TPL_CALLBACK);
@@ -444,57 +431,6 @@ UsbSelectConfig (
   return EFI_SUCCESS;
 }
 
-
-/**
-  Disconnect the USB interface with its driver.
-
-  @param  UsbIf                 The interface to disconnect driver from.
-
-**/
-EFI_STATUS
-UsbDisconnectDriver (
-  IN USB_INTERFACE        *UsbIf
-  )
-{
-  EFI_TPL                 OldTpl;
-  EFI_STATUS              Status;
-
-  //
-  // Release the hub if it's a hub controller, otherwise
-  // disconnect the driver if it is managed by other drivers.
-  //
-  Status = EFI_SUCCESS;
-  if (UsbIf->IsHub) {
-    Status = UsbIf->HubApi->Release (UsbIf);
-
-  } else if (UsbIf->IsManaged) {
-    //
-    // This function is called in both UsbIoControlTransfer and
-    // the timer callback in hub enumeration. So, at least it is
-    // called at TPL_CALLBACK. Some driver sitting on USB has
-    // twisted TPL used. It should be no problem for us to connect
-    // or disconnect at CALLBACK.
-    //
-    OldTpl           = UsbGetCurrentTpl ();
-    DEBUG ((EFI_D_INFO, "UsbDisconnectDriver: old TPL is %d, %p\n", (UINT32)OldTpl, UsbIf->Handle));
-
-    gBS->RestoreTPL (TPL_CALLBACK);
-
-    Status = gBS->DisconnectController (UsbIf->Handle, NULL, NULL);
-    if (!EFI_ERROR (Status)) {
-      UsbIf->IsManaged = FALSE;
-    }
-    
-    DEBUG (( EFI_D_INFO, "UsbDisconnectDriver: TPL after disconnect is %d, %d\n", (UINT32)UsbGetCurrentTpl(), Status));
-    ASSERT (UsbGetCurrentTpl () == TPL_CALLBACK);
-
-    gBS->RaiseTPL (OldTpl);
-  }
-  
-  return Status;
-}
-
-
 /**
   Remove the current device configuration.
 
@@ -522,8 +458,23 @@ UsbRemoveConfig (
     if (UsbIf == NULL) {
       continue;
     }
+    if (UsbIf->IsHub) {
+      Status = UsbIf->HubApi->Release (UsbIf);
+    }
+
+    ASSERT (UsbIf->Handle != NULL);
+    Status = gBS->UninstallMultipleProtocolInterfaces (
+                   UsbIf->Handle,
+                   &gEfiDevicePathProtocolGuid,
+                   UsbIf->DevicePath,
+                   &gEfiUsbIoProtocolGuid,
+                   &UsbIf->UsbIo,
+                   NULL
+                   );
+    if (!EFI_ERROR (Status)) {
+      UsbCloseHostProtoByChild (UsbIf->Device->Bus, UsbIf->Handle);
+    }
 
-    Status = UsbDisconnectDriver (UsbIf);
     if (!EFI_ERROR (Status)) {
       UsbFreeInterface (UsbIf);
       Device->Interfaces[Index] = NULL;
diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.c b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.c
index 399b7a3b60..abce63d734 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.c
@@ -2,7 +2,7 @@
 
     Wrapper function for usb host controller interface.
 
-Copyright (c) 2007 - 2010, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2007 - 2017, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -1360,15 +1360,12 @@ UsbBusRecursivelyConnectWantedUsbIo (
     UsbIf   = USB_INTERFACE_FROM_USBIO (UsbIo);
 
     if (UsbBusIsWantedUsbIO (Bus, UsbIf)) {
-      if (!UsbIf->IsManaged) {
-        //
-        // Recursively connect the wanted Usb Io handle
-        //
-        DEBUG ((EFI_D_INFO, "UsbConnectDriver: TPL before connect is %d\n", (UINT32)UsbGetCurrentTpl ()));
-        Status            = gBS->ConnectController (UsbIf->Handle, NULL, NULL, TRUE);
-        UsbIf->IsManaged  = (BOOLEAN)!EFI_ERROR (Status);
-        DEBUG ((EFI_D_INFO, "UsbConnectDriver: TPL after connect is %d\n", (UINT32)UsbGetCurrentTpl()));
-      }
+      //
+      // Recursively connect the wanted Usb Io handle
+      //
+      DEBUG ((EFI_D_INFO, "UsbConnectDriver: TPL before connect is %d\n", (UINT32)UsbGetCurrentTpl ()));
+      gBS->ConnectController (UsbIf->Handle, NULL, NULL, TRUE);
+      DEBUG ((EFI_D_INFO, "UsbConnectDriver: TPL after connect is %d\n", (UINT32)UsbGetCurrentTpl()));
     }
   }
 
-- 
2.12.2.windows.2

[PATCH 2/2] MdeModulePkg/UsbBus: Correct debug message

[Ruiyu Ni]

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.c b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.c
index abce63d734..d168d19214 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.c
@@ -1363,9 +1363,9 @@ UsbBusRecursivelyConnectWantedUsbIo (
       //
       // Recursively connect the wanted Usb Io handle
       //
-      DEBUG ((EFI_D_INFO, "UsbConnectDriver: TPL before connect is %d\n", (UINT32)UsbGetCurrentTpl ()));
+      DEBUG ((EFI_D_INFO, "UsbBusRecursivelyConnectWantedUsbIo: TPL before connect is %d\n", (UINT32)UsbGetCurrentTpl ()));
       gBS->ConnectController (UsbIf->Handle, NULL, NULL, TRUE);
-      DEBUG ((EFI_D_INFO, "UsbConnectDriver: TPL after connect is %d\n", (UINT32)UsbGetCurrentTpl()));
+      DEBUG ((EFI_D_INFO, "UsbBusRecursivelyConnectWantedUsbIo: TPL after connect is %d\n", (UINT32)UsbGetCurrentTpl()));
     }
   }
 
-- 
2.12.2.windows.2