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 69CA12034CF7D for ; Tue, 7 Nov 2017 10:23:54 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CCAF2C0587CA; Tue, 7 Nov 2017 18:27:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com CCAF2C0587CA Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.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 C31345D9CD; Tue, 7 Nov 2017 18:27:52 +0000 (UTC) To: Star Zeng , edk2-devel@lists.01.org Cc: Ruiyu Ni References: <1510018605-84896-1-git-send-email-star.zeng@intel.com> From: Laszlo Ersek Message-ID: <7752d682-9976-8873-e702-46fb31a49965@redhat.com> Date: Tue, 7 Nov 2017 19:27:51 +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: <1510018605-84896-1-git-send-email-star.zeng@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Tue, 07 Nov 2017 18:27:53 +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:23:54 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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. > + // > + 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