From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 3C1B32034CF7D for ; Tue, 7 Nov 2017 10:27:05 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 96209800A4; Tue, 7 Nov 2017 18:31:04 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 96209800A4 Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-123-130.rdu2.redhat.com [10.10.123.130]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8E2A160BE3; Tue, 7 Nov 2017 18:31:03 +0000 (UTC) From: Laszlo Ersek To: Star Zeng , edk2-devel@lists.01.org Cc: Ruiyu Ni References: <1510018605-84896-1-git-send-email-star.zeng@intel.com> <7752d682-9976-8873-e702-46fb31a49965@redhat.com> Message-ID: <588ce623-44f5-4115-2f1e-b65edc7171ae@redhat.com> Date: Tue, 7 Nov 2017 19:31:02 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <7752d682-9976-8873-e702-46fb31a49965@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Tue, 07 Nov 2017 18:31:04 +0000 (UTC) Subject: Re: [PATCH V2] MdeModulePkg SerialDxe: Handle Timeout change more robustly 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: Tue, 07 Nov 2017 18:27:05 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit oops, forgot to write down my only comment: On 11/07/17 19:27, Laszlo Ersek wrote: > On 11/07/17 02:36, Star Zeng wrote: >> https://lists.01.org/pipermail/edk2-devel/2017-October/016479.html >> reported "Xen Console input very slow in recent UEFI" that appears >> after 4cf3f37c87ba1f9d58072444bd735e40e4779e70 "MdeModulePkg >> SerialDxe: Process timeout consistently in SerialRead". >> >> Julien did more debugging and find out the following is happening in >> TerminalConInTimerHandler (MdeModulePkg/Universal/Console/TerminalDxe) >> when a character is received: >> 1) GetControl will return EFI_SERIAL_INPUT_BUFFER_EMPTY unset >> => Entering in the loop to fetch character from the serial >> 2) GetOneKeyFromSerial() >> => Return directly with the character read >> 3) Looping as the fifo is not full and no error >> 4) GetOneKeyFromSerial() -> SerialRead() >> => No more character so SerialPortPoll() will return FALSE and loop >> until timeout >> => Return EFI_TIMEOUT >> 5) Exiting the loop from TerminalConInTimerHandler >> 6) Characters are printed >> >> After some investigation, I found it is related to the Timeout value. >> >> The Timeout is 1000000 (1s) by default to follow UEFI spec. >> And the Terminal driver will recalculate and set the Timeout value >> based on the properties of UART in TerminalDriverBindingStart()/ >> TerminalConInTimerHandler(). >> >> SerialInTimeOut = 0; >> if (Mode->BaudRate != 0) { >> // >> // According to BAUD rate to calculate the timeout value. >> // >> SerialInTimeOut = (1 + Mode->DataBits + Mode->StopBits) * >> 2 * 1000000 / (UINTN) Mode->BaudRate; >> } >> >> For example, based on the PCD values of PcdUartDefaultBaudRate, >> PcdUartDefaultDataBits and PcdUartDefaultStopBits, SerialInTimeOut = >> (1 + 8 + 1) * 2 * 1000000 / (UINTN) 115200 = 173 (us). >> >> When SerialDxe is used, >> TerminalDriverBindingStart()/TerminalConInTimerHandler() -> >> SerialIo->SetAttributes() -> >> SerialSetAttributes() -> >> SerialPortSetAttributes() >> >> Some implementations of SerialPortSetAttributes() could handle the >> input parameters and return RETURN_SUCCESS, for example >> BaseSerialPortLib16550, then Timeout value will be changed to 173 (us), >> no "slow down" will be observed. >> But some implementations of SerialPortSetAttributes() just return >> RETURN_UNSUPPORTED, for example XenConsoleSerialPortLib, then Timeout >> value will be not changed and kept 1000000 (1s), "slow down" will be >> observed. >> >> SerialPortLib instance can be enhanced to >> 1. Handle the input parameters and return status accordingly instead of >> just returning RETURN_UNSUPPORTED in SerialPortSetAttributes(). >> 2. Just return RETURN_SUCCESS instead of RETURN_UNSUPPORTED in >> SerialPortSetAttributes() if the instance does not care the input >> parameters at all. >> >> And SerialDxe can also be enhanced like this patch to be more robust >> to handle Timeout change. >> >> Cc: Julien Grall >> Cc: Laszlo Ersek >> Cc: Ruiyu Ni >> Compare against the original parameters >> Suggested-by: Laszlo Ersek >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Star Zeng >> >> V2: Compare against the original parameters >> --- >> MdeModulePkg/Universal/SerialDxe/SerialIo.c | 45 +++++++++++++++++++++++++++-- >> 1 file changed, 42 insertions(+), 3 deletions(-) >> >> diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c >> index ebcd92726314..5be77e7acfb0 100644 >> --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c >> +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c >> @@ -280,12 +280,51 @@ SerialSetAttributes ( >> IN EFI_STOP_BITS_TYPE StopBits >> ) >> { >> - EFI_STATUS Status; >> - EFI_TPL Tpl; >> + EFI_STATUS Status; >> + EFI_TPL Tpl; >> + UINT64 OriginalBaudRate; >> + UINT32 OriginalReceiveFifoDepth; >> + UINT32 OriginalTimeout; >> + EFI_PARITY_TYPE OriginalParity; >> + UINT8 OriginalDataBits; >> + EFI_STOP_BITS_TYPE OriginalStopBits; >> >> + // >> + // Preserve the original input values in case >> + // SerialPortSetAttributes() updates the input/output parameters even on error. This line is 81 characters long; for better edk2 style conformance, please consider wrapping it, before you push the patch. Thanks! Laszlo >> + // >> + OriginalBaudRate = BaudRate; >> + OriginalReceiveFifoDepth = ReceiveFifoDepth; >> + OriginalTimeout = Timeout; >> + OriginalParity = Parity; >> + OriginalDataBits = DataBits; >> + OriginalStopBits = StopBits; >> Status = SerialPortSetAttributes (&BaudRate, &ReceiveFifoDepth, &Timeout, &Parity, &DataBits, &StopBits); >> if (EFI_ERROR (Status)) { >> - return Status; >> + // >> + // If it is just to set Timeout value and unsupported is returned, >> + // do not return error. >> + // >> + if ((Status == EFI_UNSUPPORTED) && >> + (This->Mode->Timeout != OriginalTimeout) && >> + (This->Mode->ReceiveFifoDepth == OriginalReceiveFifoDepth) && >> + (This->Mode->BaudRate == OriginalBaudRate) && >> + (This->Mode->DataBits == (UINT32) OriginalDataBits) && >> + (This->Mode->Parity == (UINT32) OriginalParity) && >> + (This->Mode->StopBits == (UINT32) OriginalStopBits)) { >> + // >> + // Restore to the original input values. >> + // >> + BaudRate = OriginalBaudRate; >> + ReceiveFifoDepth = OriginalReceiveFifoDepth; >> + Timeout = OriginalTimeout; >> + Parity = OriginalParity; >> + DataBits = OriginalDataBits; >> + StopBits = OriginalStopBits; >> + Status = EFI_SUCCESS; >> + } else { >> + return Status; >> + } >> } >> >> // >> > > Reviewed-by: Laszlo Ersek > > Thanks! > Laszlo >