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 96F5B202E5E6F for ; Mon, 6 Nov 2017 13:39:20 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9C9D9C057FAF; Mon, 6 Nov 2017 21:43:18 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 9C9D9C057FAF 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-122-215.rdu2.redhat.com [10.10.122.215]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5D8AF60472; Mon, 6 Nov 2017 21:43:17 +0000 (UTC) To: Star Zeng , edk2-devel@lists.01.org Cc: Julien Grall , Ruiyu Ni References: <1509586896-56444-1-git-send-email-star.zeng@intel.com> From: Laszlo Ersek Message-ID: Date: Mon, 6 Nov 2017 22:43:16 +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: <1509586896-56444-1-git-send-email-star.zeng@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Mon, 06 Nov 2017 21:43:18 +0000 (UTC) Subject: Re: [PATCH] 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: Mon, 06 Nov 2017 21:39:20 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Star, On 11/02/17 02:41, 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 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Star Zeng > --- > MdeModulePkg/Universal/SerialDxe/SerialIo.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c > index ebcd92726314..060ea56c2b1a 100644 > --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c > +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c > @@ -285,7 +285,21 @@ SerialSetAttributes ( > > 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 != Timeout) && > + (This->Mode->ReceiveFifoDepth == ReceiveFifoDepth) && > + (This->Mode->BaudRate == BaudRate) && > + (This->Mode->DataBits == (UINT32) DataBits) && > + (This->Mode->Parity == (UINT32) Parity) && > + (This->Mode->StopBits == (UINT32) StopBits)) { > + Status = EFI_SUCCESS; > + } else { > + return Status; > + } > } > > // > is the SerialPortSetAttributes() library API allowed to overwrite -- possibly even with garbage -- the IN OUT parameters if it fails? Practice is inconsistent on this; some library APIs explicitly "taint" output parameters on error, while others explicitly "preserve" input/output parameters on error. Yet others (a few exceptional APIs) output valid / sensible values *even* on error. This API in particular promises neither (in the lib class header), so I don't know. If it is not much burden, I'd prefer comparisons against the original parameters, not against those that were possibly modified by SerialPortSetAttributes(), before it returned EFI_UNSUPPORTED. I'll leave it up to you to decide. Reviewed-by: Laszlo Ersek Julien, will you test this? Thanks! Laszlo