From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 E2AA321E2DA40 for ; Tue, 15 Aug 2017 16:28:08 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 40E7091FE4; Tue, 15 Aug 2017 23:30:33 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 40E7091FE4 Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-68.phx2.redhat.com [10.3.116.68]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8A2A75D6B7; Tue, 15 Aug 2017 23:30:31 +0000 (UTC) To: Star Zeng , edk2-devel@lists.01.org Cc: Ruiyu Ni , Heyi Guo , Ard Biesheuvel , Brijesh Singh References: <1501835363-61956-1-git-send-email-star.zeng@intel.com> From: Laszlo Ersek Message-ID: Date: Wed, 16 Aug 2017 01:30:30 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <1501835363-61956-1-git-send-email-star.zeng@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Tue, 15 Aug 2017 23:30:33 +0000 (UTC) Subject: Re: [PATCH] MdeModulePkg SerialDxe: Process timeout consistently in SerialRead 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, 15 Aug 2017 23:28:09 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Star, On 08/04/17 10:29, Star Zeng wrote: > https://lists.01.org/pipermail/edk2-devel/2017-July/012385.html > reported the timeout processing in SerialRead is not consistent. > > Since SerialPortPoll only checks the status of serial port and > returns immediately, and SerialPortRead does not really implement > a time out mechanism and will always wait for enough input, > it will cause below results: > 1. If there is no serial input at all, this interface will return > timeout immediately without any waiting; > 2. If there is A characters in serial port FIFO, and caller requires > A+1 characters, it will wait until a new input is coming and timeout > will not really occur. > > This patch is to update SerialRead() to check SerialPortPoll() and > read data through SerialPortRead() one byte by one byte, and check > timeout against mSerialIoMode.Timeout if no input. > > Cc: Heyi Guo > Cc: Ruiyu Ni > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Star Zeng > --- > MdeModulePkg/Universal/SerialDxe/SerialIo.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c > index d2383e56dd8f..43d33dba0c2a 100644 > --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c > +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c > @@ -465,11 +465,25 @@ SerialRead ( > ) > { > UINTN Count; > + UINTN TimeOut; > > Count = 0; > > - if (SerialPortPoll ()) { > - Count = SerialPortRead (Buffer, *BufferSize); > + while (Count < *BufferSize) { > + TimeOut = 0; > + while (TimeOut < mSerialIoMode.Timeout) { > + if (SerialPortPoll ()) { > + break; > + } > + gBS->Stall (10); > + TimeOut += 10; > + } > + if (TimeOut >= mSerialIoMode.Timeout) { > + break; > + } > + SerialPortRead (Buffer, 1); > + Count++; > + Buffer = (VOID *) ((UINT8 *) Buffer + 1); > } > > if (Count != *BufferSize) { > This patch breaks the ArmVirtQemu platform's boot process -- it seems to fall into an infinite loop. I guess the above loop(s) never complete? If I revert this patch on top of current master (af0364f01e8c, "Nt32/PlatformBootManagerLib: Enable STD_ERROR on all consoles", 2017-08-14), then ArmVirtQemu boots fine. I found this commit with bisection: > 4cf3f37c87ba1f9d58072444bd735e40e4779e70 is the first bad commit > commit 4cf3f37c87ba1f9d58072444bd735e40e4779e70 > Author: Star Zeng > Date: Tue Jul 18 16:32:16 2017 +0800 > > MdeModulePkg SerialDxe: Process timeout consistently in SerialRead > > https://lists.01.org/pipermail/edk2-devel/2017-July/012385.html > reported the timeout processing in SerialRead is not consistent. > > Since SerialPortPoll only checks the status of serial port and > returns immediately, and SerialPortRead does not really implement > a time out mechanism and will always wait for enough input, > it will cause below results: > 1. If there is no serial input at all, this interface will return > timeout immediately without any waiting; > 2. If there is A characters in serial port FIFO, and caller requires > A+1 characters, it will wait until a new input is coming and timeout > will not really occur. > > This patch is to update SerialRead() to check SerialPortPoll() and > read data through SerialPortRead() one byte by one byte, and check > timeout against mSerialIoMode.Timeout if no input. > > Cc: Heyi Guo > Cc: Ruiyu Ni > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Star Zeng > Reviewed-by: Ruiyu Ni > > :040000 040000 aaa8b75d378ec613b828db938c36a16723583906 d034044918d7f5b8d30783e0a9eccdf3cf21e5c5 M MdeModulePkg Bisection log: > git bisect start > # bad: [af0364f01e8cac95afad01437f13beef90f6640b] Nt32/PlatformBootManagerLib: Enable STD_ERROR on all consoles > git bisect bad af0364f01e8cac95afad01437f13beef90f6640b > # good: [c325e41585e374d40fb36b434e61a1ab0fca5b1c] MdeModulePkg: Fix coding style issues > git bisect good c325e41585e374d40fb36b434e61a1ab0fca5b1c > # good: [636cda51903b4b28e1c9b099c4f22a84e61b09da] OvmfPkg/OvmfPkg.fdf.inc: extract VARS_LIVE_SIZE and VARS_SPARE_SIZE macros > git bisect good 636cda51903b4b28e1c9b099c4f22a84e61b09da > # good: [997b2c543751cb4a3473270c1a7016ade311f01b] BaseTools/GenCrc32: Fix a bug to hand empty file for decode > git bisect good 997b2c543751cb4a3473270c1a7016ade311f01b > # good: [f1658838c267723139711c0b15d98a74980ae4c5] OvmfPkg/IoMmuDxe: abort harder on memory encryption mask failures > git bisect good f1658838c267723139711c0b15d98a74980ae4c5 > # bad: [9458afa33728e64049d465f052b2c5c3ca3e881c] IntelFrameworkModulePkg: Fix Xcode 9 Beta treating 32-bit left shift as undefined > git bisect bad 9458afa33728e64049d465f052b2c5c3ca3e881c > # good: [6e414300b5f19d3045a0d21ad90ac2fe965478a5] EmbeddedPkg/AndroidFastboot: split android boot header > git bisect good 6e414300b5f19d3045a0d21ad90ac2fe965478a5 > # bad: [416d48f755518fd1d202b97be2e9944df6e8f0d4] ShellPkg/drivers: Show Image Name in non-SFO mode > git bisect bad 416d48f755518fd1d202b97be2e9944df6e8f0d4 > # bad: [2913ebb2b550f50a14f105e26995dd095e627ba4] NetworkPkg/HttpBootDxe: Refine the coding style. > git bisect bad 2913ebb2b550f50a14f105e26995dd095e627ba4 > # bad: [9e2a8e928995c3b1bb664b73fd59785055c6b5f6] OvmfPkg/AcpiPlatformDxe: short-circuit the transfer of an empty S3_CONTEXT > git bisect bad 9e2a8e928995c3b1bb664b73fd59785055c6b5f6 > # bad: [4cf3f37c87ba1f9d58072444bd735e40e4779e70] MdeModulePkg SerialDxe: Process timeout consistently in SerialRead > git bisect bad 4cf3f37c87ba1f9d58072444bd735e40e4779e70 > # first bad commit: [4cf3f37c87ba1f9d58072444bd735e40e4779e70] MdeModulePkg SerialDxe: Process timeout consistently in SerialRead In retrospect, I see that you asked me for feedback in the original discussion, at the following URL: https://lists.01.org/pipermail/edk2-devel/2017-July/012406.html Unfortunately, this was on July 18th, and I was on vacation then. (I think I configured my email account to send an automated out-of-office reply.) Looking at the patch now, one idea I have is to keep the time running across all bytes read; that is, use mSerialIoMode.Timeout as a global timeout for the entire SerialRead() call. Unfortunately, the UEFI-2.7 spec defines the "SERIAL_IO_MODE.Timeout" field, and the Timeout parameter of EFI_SERIAL_IO_PROTOCOL.SetAttributes(), inconsistently with each other. First it says (about the field): Timeout If applicable, the number of microseconds to wait before timing out a Read or Write operation. (This is compatible with my idea.) But then it says (in the general description, and about the SetAttributes() parameter): The default attributes for all UART-style serial device interfaces are: [...] a 1,000,000 microsecond timeout *per character* [...] Timeout The requested time out *for a single character* in microseconds. This timeout applies to both the transmit and receive side of the interface. A Timeout value of 0 will use the device's default time out value. (Emphases mine.) ... I added some debug messages to the loops, and the first invocation of the function seems to hang with the following parameters: > SerialRead: BufferSize=1 mSerialIoMode.Timeout=1000000 That is, the caller wants to read a single character (which is not arriving). The timeout is the default 1 second. However, the wait takes much-much longer than 1 second (it appears to be infinite). It seems that gBS->Stall(10) takes much longer than 10 microseconds. According to the UEFI spec, The Stall() function stalls execution on the processor for at least the requested number of microseconds. [...] So Stall() is allowed to wait longer (possibly a lot longer) -- I guess it depends on some timer's resolution. I have another idea related to this -- let me research it a bit later. I might post some patches. Thanks Laszlo