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

[Laszlo Ersek <lersek@redhat.com>]

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