From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9A5B521AF39AB for ; Wed, 31 May 2017 04:38:59 -0700 (PDT) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 31 May 2017 04:39:59 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,423,1491289200"; d="scan'208";a="108662393" Received: from ray-dev.ccr.corp.intel.com ([10.239.9.1]) by fmsmga005.fm.intel.com with ESMTP; 31 May 2017 04:39:58 -0700 From: Ruiyu Ni To: edk2-devel@lists.01.org Cc: Feng Tian , Star Zeng , Hao A Wu Date: Wed, 31 May 2017 19:39:54 +0800 Message-Id: <20170531113955.65448-2-ruiyu.ni@intel.com> X-Mailer: git-send-email 2.12.2.windows.2 In-Reply-To: <20170531113955.65448-1-ruiyu.ni@intel.com> References: <20170531113955.65448-1-ruiyu.ni@intel.com> Subject: [PATCH 1/2] MdeModulePkg/UsbBus: Fix system hang when failed to uninstall UsbIo X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 31 May 2017 11:38:59 -0000 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 Cc: Feng Tian Cc: Star Zeng Cc: Hao A Wu --- 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.
+Copyright (c) 2004 - 2017, Intel Corporation. All rights reserved.
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.
+Copyright (c) 2007 - 2017, Intel Corporation. All rights reserved.
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.
+Copyright (c) 2007 - 2017, Intel Corporation. All rights reserved.
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