From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (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 01BB981DAE for ; Tue, 8 Nov 2016 00:41:35 -0800 (PST) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga102.jf.intel.com with ESMTP; 08 Nov 2016 00:41:38 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,609,1473145200"; d="scan'208";a="28638076" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga006.fm.intel.com with ESMTP; 08 Nov 2016 00:41:38 -0800 Received: from FMSMSX109.amr.corp.intel.com (10.18.116.9) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 8 Nov 2016 00:41:38 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx109.amr.corp.intel.com (10.18.116.9) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 8 Nov 2016 00:41:37 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.206]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.96]) with mapi id 14.03.0248.002; Tue, 8 Nov 2016 16:41:30 +0800 From: "Zeng, Star" To: "Ni, Ruiyu" , "edk2-devel@lists.01.org" CC: "Dong, Eric" , "Zeng, Star" Thread-Topic: [PATCH] MdeModulePkg/PciSioSerial: Fix a bug that wrongly produces 2 UARTs Thread-Index: AQHSOWjTAuProBISGEOZsU+K8vQvVqDOxOgQ Date: Tue, 8 Nov 2016 08:41:30 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103959726B@shsmsx102.ccr.corp.intel.com> References: <20161108023547.22784-1-ruiyu.ni@intel.com> In-Reply-To: <20161108023547.22784-1-ruiyu.ni@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] MdeModulePkg/PciSioSerial: Fix a bug that wrongly produces 2 UARTs X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Nov 2016 08:41:36 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Star Zeng -----Original Message----- From: Ni, Ruiyu=20 Sent: Tuesday, November 8, 2016 10:36 AM To: edk2-devel@lists.01.org Cc: Zeng, Star ; Dong, Eric Subject: [PATCH] MdeModulePkg/PciSioSerial: Fix a bug that wrongly produces= 2 UARTs When PciSioSerial is firstly started with a non-NULL remaining device path,= the UART instance is created using the parameters specified in the remaini= ng device path. Later when the driver is started again on the same UART con= troller with NULL remaining device path, the correct logic is to directly r= eturn SUCCESS instead of current buggy implementation which wrongly produce= s another UART using the default parameters. The bug causes two UARTs are created when the UART is configured in 57600 b= aud rate. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ruiyu Ni Cc: Star Zeng Cc: Eric Dong --- MdeModulePkg/Bus/Pci/PciSioSerialDxe/Serial.c | 120 ++++++++++++++--------= ---- 1 file changed, 64 insertions(+), 56 deletions(-) diff --git a/MdeModulePkg/Bus/Pci/PciSioSerialDxe/Serial.c b/MdeModulePkg/B= us/Pci/PciSioSerialDxe/Serial.c index aeafee2..a487836 100644 --- a/MdeModulePkg/Bus/Pci/PciSioSerialDxe/Serial.c +++ b/MdeModulePkg/Bus/Pci/PciSioSerialDxe/Serial.c @@ -1,7 +1,7 @@ /** @file Serial driver for PCI or SIO UARTS. =20 -Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
This program and the accompanying materials are licensed and made availab= le under the terms and conditions of the BSD License which accompanies thi= s distribution. The full text of the license may be found at @@ -863,67 +8= 63,75 @@ SerialControllerDriverStart ( ControllerNumber =3D 0; ContainsControllerNode =3D FALSE; SerialDevices =3D GetChildSerialDevices (Controller, IoProtocolGuid, &Se= rialDeviceCount); - // - // If the SerialIo instance specified by RemainingDevicePath is already = created, - // update the attributes/control. - // - if ((SerialDeviceCount !=3D 0) && (RemainingDevicePath !=3D NULL)) { - Uart =3D (UART_DEVICE_PATH *) SkipControllerDevicePathNode (RemainingD= evicePath, &ContainsControllerNode, &ControllerNumber); - for (Index =3D 0; Index < SerialDeviceCount; Index++) { - ASSERT ((SerialDevices !=3D NULL) && (SerialDevices[Index] !=3D NULL= )); - if ((!SerialDevices[Index]->ContainsControllerNode && !ContainsContr= ollerNode) || - (SerialDevices[Index]->ContainsControllerNode && ContainsControl= lerNode && SerialDevices[Index]->Instance =3D=3D ControllerNumber) - ) { - SerialIo =3D &SerialDevices[Index]->SerialIo; - Status =3D EFI_INVALID_PARAMETER; - // - // Pass NULL ActualBaudRate to VerifyUartParameters to disallow ba= udrate degrade. - // DriverBindingStart() shouldn't create a handle with different U= ART device path. - // - if (VerifyUartParameters (SerialDevices[Index]->ClockRate, Uart->B= audRate, Uart->DataBits, - (EFI_PARITY_TYPE) Uart->Parity, (EFI_STO= P_BITS_TYPE) Uart->StopBits, NULL, NULL)) { - Status =3D SerialIo->SetAttributes ( - SerialIo, - Uart->BaudRate, - SerialIo->Mode->ReceiveFifoDepth, - SerialIo->Mode->Timeout, - (EFI_PARITY_TYPE) Uart->Parity, - Uart->DataBits, - (EFI_STOP_BITS_TYPE) Uart->StopBits - ); - } - FlowControl =3D (UART_FLOW_CONTROL_DEVICE_PATH *) NextDevicePathNo= de (Uart); - if (!EFI_ERROR (Status) && IsUartFlowControlDevicePathNode (FlowCo= ntrol)) { - Status =3D SerialIo->GetControl (SerialIo, &Control); - if (!EFI_ERROR (Status)) { - if (ReadUnaligned32 (&FlowControl->FlowControlMap) =3D=3D UART= _FLOW_CONTROL_HARDWARE) { - Control |=3D EFI_SERIAL_HARDWARE_FLOW_CONTROL_ENABLE; - } else { - Control &=3D ~EFI_SERIAL_HARDWARE_FLOW_CONTROL_ENABLE; + + if (SerialDeviceCount !=3D 0) { + if (RemainingDevicePath =3D=3D NULL) { + // + // If the SerialIo instance is already created, NULL as RemainingDev= icePath is treated + // as to create the same SerialIo instance. + // + return EFI_SUCCESS; + } else { + // + // Update the attributes/control of the SerialIo instance specified = by RemainingDevicePath. + // + Uart =3D (UART_DEVICE_PATH *) SkipControllerDevicePathNode (Remainin= gDevicePath, &ContainsControllerNode, &ControllerNumber); + for (Index =3D 0; Index < SerialDeviceCount; Index++) { + ASSERT ((SerialDevices !=3D NULL) && (SerialDevices[Index] !=3D NU= LL)); + if ((!SerialDevices[Index]->ContainsControllerNode && !ContainsCon= trollerNode) || + (SerialDevices[Index]->ContainsControllerNode && ContainsContr= ollerNode && SerialDevices[Index]->Instance =3D=3D ControllerNumber) + ) { + SerialIo =3D &SerialDevices[Index]->SerialIo; + Status =3D EFI_INVALID_PARAMETER; + // + // Pass NULL ActualBaudRate to VerifyUartParameters to disallow = baudrate degrade. + // DriverBindingStart() shouldn't create a handle with different= UART device path. + // + if (VerifyUartParameters (SerialDevices[Index]->ClockRate, Uart-= >BaudRate, Uart->DataBits, + (EFI_PARITY_TYPE) Uart->Parity, (EFI_S= TOP_BITS_TYPE) Uart->StopBits, NULL, NULL)) { + Status =3D SerialIo->SetAttributes ( + SerialIo, + Uart->BaudRate, + SerialIo->Mode->ReceiveFifoDepth, + SerialIo->Mode->Timeout, + (EFI_PARITY_TYPE) Uart->Parity, + Uart->DataBits, + (EFI_STOP_BITS_TYPE) Uart->StopBits + ); + } + FlowControl =3D (UART_FLOW_CONTROL_DEVICE_PATH *) NextDevicePath= Node (Uart); + if (!EFI_ERROR (Status) && IsUartFlowControlDevicePathNode (Flow= Control)) { + Status =3D SerialIo->GetControl (SerialIo, &Control); + if (!EFI_ERROR (Status)) { + if (ReadUnaligned32 (&FlowControl->FlowControlMap) =3D=3D UA= RT_FLOW_CONTROL_HARDWARE) { + Control |=3D EFI_SERIAL_HARDWARE_FLOW_CONTROL_ENABLE; + } else { + Control &=3D ~EFI_SERIAL_HARDWARE_FLOW_CONTROL_ENABLE; + } + // + // Clear the bits that are not allowed to pass to SetControl + // + Control &=3D (EFI_SERIAL_REQUEST_TO_SEND | EFI_SERIAL_DATA_T= ERMINAL_READY | + EFI_SERIAL_HARDWARE_LOOPBACK_ENABLE | EFI_SERIAL= _SOFTWARE_LOOPBACK_ENABLE | + EFI_SERIAL_HARDWARE_FLOW_CONTROL_ENABLE); + Status =3D SerialIo->SetControl (SerialIo, Control); } - // - // Clear the bits that are not allowed to pass to SetControl - // - Control &=3D (EFI_SERIAL_REQUEST_TO_SEND | EFI_SERIAL_DATA_TER= MINAL_READY | - EFI_SERIAL_HARDWARE_LOOPBACK_ENABLE | EFI_SERIAL_S= OFTWARE_LOOPBACK_ENABLE | - EFI_SERIAL_HARDWARE_FLOW_CONTROL_ENABLE); - Status =3D SerialIo->SetControl (SerialIo, Control); } + break; } - break; } - } - if (Index !=3D SerialDeviceCount) { - // - // Directly return if the SerialIo instance specified by RemainingDe= vicePath is found and updated. - // Otherwise continue to create the instance specified by RemainingD= evicePath. - // - if (SerialDevices !=3D NULL) { - FreePool (SerialDevices); + if (Index !=3D SerialDeviceCount) { + // + // Directly return if the SerialIo instance specified by Remaining= DevicePath is found and updated. + // Otherwise continue to create the instance specified by Remainin= gDevicePath. + // + if (SerialDevices !=3D NULL) { + FreePool (SerialDevices); + } + return Status; } - return Status; } - } + } =20 if (RemainingDevicePath !=3D NULL) { Uart =3D (UART_DEVICE_PATH *) SkipControllerDevicePathNode (RemainingD= evicePath, &ContainsControllerNode, &ControllerNumber); -- 2.9.0.windows.1