MdeModulePkg/Universal/SerialDxe/SerialIo.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
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 <heyi.guo@linaro.org>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng <star.zeng@intel.com>
---
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) {
--
2.7.0.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Star, 3 minor comments below. Thanks/Ray > -----Original Message----- > From: Zeng, Star > Sent: Friday, August 4, 2017 4:29 PM > To: edk2-devel@lists.01.org > Cc: Zeng, Star <star.zeng@intel.com>; Heyi Guo <heyi.guo@linaro.org>; Ni, > Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> > Subject: [PATCH] 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 <heyi.guo@linaro.org> > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Star Zeng <star.zeng@intel.com> > --- > 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); 1. can you use EFI_TIMER_PERIOD_MICROSECONDS(1)? > + TimeOut += 10; 2. TImeOut++? > + } > + if (TimeOut >= mSerialIoMode.Timeout) { 3. if (TimeOut == ...) { ? > + break; > + } > + SerialPortRead (Buffer, 1); > + Count++; > + Buffer = (VOID *) ((UINT8 *) Buffer + 1); > } > > if (Count != *BufferSize) { > -- > 2.7.0.windows.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Thanks for the comments. EFI_TIMER_PERIOD_MICROSECONDS is used for timer event according to its definition, and its unit is 100ns. But the unit of mSerialIoMode.Timeout and gBS->Stall() is 1us. +1 may cause more polling of SerialPortPoll(). How about keeping using +10? :) Thanks, Star -----Original Message----- From: Ni, Ruiyu Sent: Friday, August 4, 2017 5:13 PM To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org Cc: Heyi Guo <heyi.guo@linaro.org>; Laszlo Ersek <lersek@redhat.com> Subject: RE: [PATCH] MdeModulePkg SerialDxe: Process timeout consistently in SerialRead Star, 3 minor comments below. Thanks/Ray > -----Original Message----- > From: Zeng, Star > Sent: Friday, August 4, 2017 4:29 PM > To: edk2-devel@lists.01.org > Cc: Zeng, Star <star.zeng@intel.com>; Heyi Guo <heyi.guo@linaro.org>; > Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> > Subject: [PATCH] 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 <heyi.guo@linaro.org> > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Star Zeng <star.zeng@intel.com> > --- > 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); 1. can you use EFI_TIMER_PERIOD_MICROSECONDS(1)? > + TimeOut += 10; 2. TImeOut++? > + } > + if (TimeOut >= mSerialIoMode.Timeout) { 3. if (TimeOut == ...) { ? > + break; > + } > + SerialPortRead (Buffer, 1); > + Count++; > + Buffer = (VOID *) ((UINT8 *) Buffer + 1); > } > > if (Count != *BufferSize) { > -- > 2.7.0.windows.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
I thought unit of Stall is 100ns. Then no issues now. Reviewed-by: Ruiyu Ni <Ruiyu.ni@intel.com> Thanks/Ray > -----Original Message----- > From: Zeng, Star > Sent: Friday, August 4, 2017 5:25 PM > To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org > Cc: Heyi Guo <heyi.guo@linaro.org>; Laszlo Ersek <lersek@redhat.com>; > Zeng, Star <star.zeng@intel.com> > Subject: RE: [PATCH] MdeModulePkg SerialDxe: Process timeout consistently > in SerialRead > > Thanks for the comments. > > EFI_TIMER_PERIOD_MICROSECONDS is used for timer event according to its > definition, and its unit is 100ns. > But the unit of mSerialIoMode.Timeout and gBS->Stall() is 1us. > > +1 may cause more polling of SerialPortPoll(). How about keeping using > ++10? :) > > > Thanks, > Star > -----Original Message----- > From: Ni, Ruiyu > Sent: Friday, August 4, 2017 5:13 PM > To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org > Cc: Heyi Guo <heyi.guo@linaro.org>; Laszlo Ersek <lersek@redhat.com> > Subject: RE: [PATCH] MdeModulePkg SerialDxe: Process timeout consistently > in SerialRead > > Star, > 3 minor comments below. > > Thanks/Ray > > > -----Original Message----- > > From: Zeng, Star > > Sent: Friday, August 4, 2017 4:29 PM > > To: edk2-devel@lists.01.org > > Cc: Zeng, Star <star.zeng@intel.com>; Heyi Guo <heyi.guo@linaro.org>; > > Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> > > Subject: [PATCH] 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 <heyi.guo@linaro.org> > > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Star Zeng <star.zeng@intel.com> > > --- > > 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); > 1. can you use EFI_TIMER_PERIOD_MICROSECONDS(1)? > > > + TimeOut += 10; > 2. TImeOut++? > > > + } > > + if (TimeOut >= mSerialIoMode.Timeout) { > 3. if (TimeOut == ...) { ? > > > + break; > > + } > > + SerialPortRead (Buffer, 1); > > + Count++; > > + Buffer = (VOID *) ((UINT8 *) Buffer + 1); > > } > > > > if (Count != *BufferSize) { > > -- > > 2.7.0.windows.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 <heyi.guo@linaro.org> > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Star Zeng <star.zeng@intel.com> > --- > 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 <star.zeng@intel.com> > 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 <heyi.guo@linaro.org> > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Star Zeng <star.zeng@intel.com> > Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com> > > :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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo, gBS->Stall() layers on top of the Metronome Architectural Protocol. armvirtqemu.dsc: EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf And this implementation layers on top of a TimerLib armvirtqemu.dsc: TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf There are a couple PCDs involved in this module and lib. Maybe the ArmVirtPkg needs to set some different PCD values to get a more accurate gBS->Stall() when running through QEMU. Best regards, Mike > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On > Behalf Of Laszlo Ersek > Sent: Tuesday, August 15, 2017 4:31 PM > To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Heyi Guo > <heyi.guo@linaro.org>; Ard Biesheuvel > <ard.biesheuvel@linaro.org> > Subject: Re: [edk2] [PATCH] MdeModulePkg SerialDxe: Process > timeout consistently in SerialRead > > 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 <heyi.guo@linaro.org> > > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Star Zeng <star.zeng@intel.com> > > --- > > 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 <star.zeng@intel.com> > > 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 <heyi.guo@linaro.org> > > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Star Zeng <star.zeng@intel.com> > > Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com> > > > > :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 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 08/16/17 01:59, Kinney, Michael D wrote: > Laszlo, > > gBS->Stall() layers on top of the Metronome Architectural Protocol. > > armvirtqemu.dsc: EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf > > And this implementation layers on top of a TimerLib > > armvirtqemu.dsc: TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf > > There are a couple PCDs involved in this module and lib. Maybe the > ArmVirtPkg needs to set some different PCD values to get a more accurate > gBS->Stall() when running through QEMU. The issue is different; I checked gBS->Stall() in the above context and it waits for the appropriate time (usually just 1-2 microseconds more than requested). Instead, the following happens: - TerminalConInTimerHandler() in "MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c" calls SerialIo->GetControl(). - If the GetControl() call fails (for any reason), or the returned Control word has EFI_SERIAL_INPUT_BUFFER_EMPTY clear, then TerminalConInTimerHandler() enters a loop: // // Fetch all the keys in the serial buffer, // and insert the byte stream into RawFIFO. // - The loop body calls GetOneKeyFromSerial() --> SerialIo->Read(), with a 1 byte buffer. - The loop runs until the "raw data FIFO buffer" is filled completely (256 byte is the size, apparently -- RAW_FIFO_MAX_NUMBER), or GetOneKeyFromSerial() returns an error. - The SerialPortGetControl() function in "ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c" returns constant RETURN_UNSUPPORTED. According to the library class header "MdePkg/Include/Library/SerialPortLib.h", this is a valid thing to do ("The serial device does not support this operation"). However, it will cause TerminalConInTimerHandler() to *always* enter the loop, even if there is no pending data to read. - Because the input queue is empty, GetOneKeyFromSerial() will take a full second before it times out. - And the final piece of the puzzle is that the event associated with TerminalConInTimerHandler() is signaled at 50Hz (0.02s period, see KEYBOARD_TIMER_INTERVAL), initially. It can dynamically adjust its frequency to the serial device's timeout, but in practice, as soon as the current TerminalConInTimerHandler() frame returns, there's (apparently) another execution queued already. So basically we're stuck in the timer event handler. I think we should implement the missing ("unsupported") functions in "ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c" and possibly in "ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c". I believe we could use "ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c" as an example. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
*Control = 0; if (!SerialPortPoll ()) { *Control = EFI_SERIAL_INPUT_BUFFER_EMPTY; } return RETURN_SUCCESS; As example, the code above (in OvmfPkg\Library\XenConsoleSerialPortLib\XenConsoleSerialPortLib.c) can be simply used in SerialPortGetControl(). Thanks, Star -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Wednesday, August 16, 2017 10:02 AM To: Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Heyi Guo <heyi.guo@linaro.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org> Subject: Re: [edk2] [PATCH] MdeModulePkg SerialDxe: Process timeout consistently in SerialRead On 08/16/17 01:59, Kinney, Michael D wrote: > Laszlo, > > gBS->Stall() layers on top of the Metronome Architectural Protocol. > > armvirtqemu.dsc: EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf > > And this implementation layers on top of a TimerLib > > armvirtqemu.dsc: > TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf > > There are a couple PCDs involved in this module and lib. Maybe the > ArmVirtPkg needs to set some different PCD values to get a more > accurate > gBS->Stall() when running through QEMU. The issue is different; I checked gBS->Stall() in the above context and it waits for the appropriate time (usually just 1-2 microseconds more than requested). Instead, the following happens: - TerminalConInTimerHandler() in "MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c" calls SerialIo->GetControl(). - If the GetControl() call fails (for any reason), or the returned Control word has EFI_SERIAL_INPUT_BUFFER_EMPTY clear, then TerminalConInTimerHandler() enters a loop: // // Fetch all the keys in the serial buffer, // and insert the byte stream into RawFIFO. // - The loop body calls GetOneKeyFromSerial() --> SerialIo->Read(), with a 1 byte buffer. - The loop runs until the "raw data FIFO buffer" is filled completely (256 byte is the size, apparently -- RAW_FIFO_MAX_NUMBER), or GetOneKeyFromSerial() returns an error. - The SerialPortGetControl() function in "ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c" returns constant RETURN_UNSUPPORTED. According to the library class header "MdePkg/Include/Library/SerialPortLib.h", this is a valid thing to do ("The serial device does not support this operation"). However, it will cause TerminalConInTimerHandler() to *always* enter the loop, even if there is no pending data to read. - Because the input queue is empty, GetOneKeyFromSerial() will take a full second before it times out. - And the final piece of the puzzle is that the event associated with TerminalConInTimerHandler() is signaled at 50Hz (0.02s period, see KEYBOARD_TIMER_INTERVAL), initially. It can dynamically adjust its frequency to the serial device's timeout, but in practice, as soon as the current TerminalConInTimerHandler() frame returns, there's (apparently) another execution queued already. So basically we're stuck in the timer event handler. I think we should implement the missing ("unsupported") functions in "ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c" and possibly in "ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c". I believe we could use "ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c" as an example. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Star, On 08/16/17 04:22, Zeng, Star wrote: > *Control = 0; > if (!SerialPortPoll ()) { > *Control = EFI_SERIAL_INPUT_BUFFER_EMPTY; > } > return RETURN_SUCCESS; > > As example, the code above (in > OvmfPkg\Library\XenConsoleSerialPortLib\XenConsoleSerialPortLib.c) can > be simply used in SerialPortGetControl(). Thank you for jogging my memory about this. I found the following commit in the git history: ad7f6bc2e116 ("ArmVirtPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg", 2015-11-26) and then I found our earlier discussion from 2015: http://mid.mail-archive.com/1447752930-32880-12-git-send-email-star.zeng@intel.com http://mid.mail-archive.com/1448243067-1880-12-git-send-email-star.zeng@intel.com Basically the discussion went like this: (1) In your v1 patch, you disconnected the FdtPL011SerialPortLib instance from the SerialPortExtLib class, and implemented the new functions (GetControl(), SetControl(), SetAttributes()) immediately by calling PL011UartLib. (2) I requested that first we just copy the existing code from the EmbeddedPkg/Library/SerialPortExtLibNull instance -- which FdtPL011SerialPortLib had used up to that point --, and call PL011UartLib in a separate patch (later, if at all). (3) In your v3 patch, you chose an approach that was different from *both* copying SerialPortExtLibNull *and* calling PL011UartLib: the new functions would simply return RETURN_UNSUPPORTED. (4) Analyzing the TerminalDxe driver at that point, we agreed: - that a retval check had to be added after the GetControl() call in TerminalDxe, - more importantly, that with the error check in place, it was *equivalent* for GetControl() to return RETURN_UNSUPPORTED -- seen in your v3 patch --, or to return RETURN_SUCCESS and set Control to zero -- seen in the original code in SerialPortExtLibNull. In both cases, we expected TerminalDxe to work correctly, given that GetOneKeyFromSerial() would exit the loop on the first iteration anyway. In other words, setting EFI_SERIAL_INPUT_BUFFER_EMPTY in Control (and returning success) would only be an unimportant optimization. (5) We agreed that calling PL011UartLib from the new APIs could be a future feature addition. And that's the status what we have right now. Except, with the SerialDxe change in commit 4cf3f37c87ba ("MdeModulePkg SerialDxe: Process timeout consistently in SerialRead", 2017-07-18), setting EFI_SERIAL_INPUT_BUFFER_EMPTY in Control is no longer an "unimportant optimization", because now GetOneKeyFromSerial() takes *very long* to exit the loop on the first iteration. So even the first iteration must be prevented if the input queue is empty, and that requires implementing GetControl() for real. I think I'll submit an FdtPL011SerialPortLib patch, similar to your v1 approach. PL011UartLib implements these functions; we should use them. Thanks! Laszlo > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Wednesday, August 16, 2017 10:02 AM > To: Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star > <star.zeng@intel.com>; edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Heyi Guo <heyi.guo@linaro.org>; > Ard Biesheuvel <ard.biesheuvel@linaro.org> > Subject: Re: [edk2] [PATCH] MdeModulePkg SerialDxe: Process timeout > consistently in SerialRead > > On 08/16/17 01:59, Kinney, Michael D wrote: >> Laszlo, >> >> gBS->Stall() layers on top of the Metronome Architectural Protocol. >> >> armvirtqemu.dsc: EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf >> >> And this implementation layers on top of a TimerLib >> >> armvirtqemu.dsc: >> TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf >> >> There are a couple PCDs involved in this module and lib. Maybe the >> ArmVirtPkg needs to set some different PCD values to get a more >> accurate >> gBS->Stall() when running through QEMU. > > The issue is different; I checked gBS->Stall() in the above context > and it waits for the appropriate time (usually just 1-2 microseconds > more than requested). > > Instead, the following happens: > > - TerminalConInTimerHandler() in > "MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c" calls > SerialIo->GetControl(). > > - If the GetControl() call fails (for any reason), or the returned > Control word has EFI_SERIAL_INPUT_BUFFER_EMPTY clear, then > TerminalConInTimerHandler() enters a loop: > > // > // Fetch all the keys in the serial buffer, > // and insert the byte stream into RawFIFO. > // > > - The loop body calls GetOneKeyFromSerial() --> SerialIo->Read(), with > a 1 byte buffer. > > - The loop runs until the "raw data FIFO buffer" is filled completely > (256 byte is the size, apparently -- RAW_FIFO_MAX_NUMBER), or > GetOneKeyFromSerial() returns an error. > > - The SerialPortGetControl() function in > "ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c" > returns constant RETURN_UNSUPPORTED. According to the library class > header "MdePkg/Include/Library/SerialPortLib.h", this is a valid thing > to do ("The serial device does not support this operation"). However, > it will cause TerminalConInTimerHandler() to *always* enter the loop, > even if there is no pending data to read. > > - Because the input queue is empty, GetOneKeyFromSerial() will take a > full second before it times out. > > - And the final piece of the puzzle is that the event associated with > TerminalConInTimerHandler() is signaled at 50Hz (0.02s period, see > KEYBOARD_TIMER_INTERVAL), initially. It can dynamically adjust its > frequency to the serial device's timeout, but in practice, as soon as > the current TerminalConInTimerHandler() frame returns, there's > (apparently) another execution queued already. So basically we're > stuck in the timer event handler. > > I think we should implement the missing ("unsupported") functions in > "ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c" and > possibly in > "ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c". > I believe we could use > "ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c" as an > example. > > Thanks > Laszlo > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.