Re: [PATCH] MdeModulePkg SerialDxe: Process timeout consistently in SerialRead

[Laszlo Ersek <lersek@redhat.com>]

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
>