On 11/24/15 02:01, Zeng, Star wrote:
On 2015/11/23 22:01, Laszlo Ersek wrote:
[snip]
(2) although I see that you unified the GetControl / SetControl /
SetAttributes implementations between
- EarlyFdtPL011SerialPortLib and
- FdtPL011SerialPortLib,
the actual implementations are incorrect, plus this doesn't seem to be
what we agreed upon.
Please refer to:
http://thread.gmane.org/gmane.comp.bios.edk2.devel/4370/focus=4408
You wrote "Even the SerialDxe may not work for ConsoleIn (ConsoleOut
should work well) with the functions return RETURN_UNSUPPORTED
(especially SerialPortGetControl())."
So I think that that *all six* functions should return RETURN_SUCCESS.
(And both GetControl functions should also set *Control to zero.)
Again, please just do what
"EmbeddedPkg/Library/SerialPortExtLibNull/SerialPortExtLibNull.c" does.
There will be no difference to return RETURN_SUCCESS or
RETURN_UNSUPPORTED if the interfaces are not to do the real get/set
operation.
The matter what I said is that the value return from
SerialPortGetControl() with *EFI_SERIAL_INPUT_BUFFER_EMPTY*, you can see
the line 568 in TerminalConIn.c.
Okay, thank you for that location. I've been wondering where these
functions are actually called.
So, the code in question is in the TerminalConInTimerHandler() function
-- leading comment: "Timer handler to poll the key from serial" --; it
goes like:
//
// Check whether serial buffer is empty.
//
Status = SerialIo->GetControl (SerialIo, &Control);
if ((Control & EFI_SERIAL_INPUT_BUFFER_EMPTY) == 0) {
//
// Fetch all the keys in the serial buffer,
// and insert the byte stream into RawFIFO.
//
while (!IsRawFiFoFull (TerminalDevice)) {
Status = GetOneKeyFromSerial (TerminalDevice->SerialIo, &Input);
if (EFI_ERROR (Status)) {
if (Status == EFI_DEVICE_ERROR) {
REPORT_STATUS_CODE_WITH_DEVICE_PATH (
EFI_ERROR_CODE | EFI_ERROR_MINOR,
(EFI_PERIPHERAL_REMOTE_CONSOLE | EFI_P_EC_INPUT_ERROR),
TerminalDevice->DevicePath
);
}
break;
}
RawFiFoInsertOneKey (TerminalDevice, Input);
}
}
(1) This code has a bug; it checks the output-only parameter called
"Control" without verifying the Status variable. Worse, "Control" is
never set or initialized before that point, so the code can easily act
upon an indeterminate value.
(2) The EFI_SERIAL_INPUT_BUFFER_EMPTY check looks like an optimization
only. The loop that depends on it copies characters from the serial
terminal into an internal queue, as long as the internal queue has room,
*and* the terminal device can provide characters.
If the EFI_SERIAL_INPUT_BUFFER_EMPTY bit is always clear, then the
GetOneKeyFromSerial() function can still exit the loop early. Namely,
GetOneKeyFromSerial() calls SerialIo->Read(), and as far as I
understand, this series doesn't change the semantics of that
(non-extended) serial library function, in any of the library instances.
So, if 0 bytes could be read, GetOneKeyFromSerial() returns
EFI_NOT_READY, and the outer loop is exited.
(3) Therefore, always returning success and setting Control to zero is a
safe choice for a "null" implementation of GetControl(). This is what
"EmbeddedPkg/Library/SerialPortExtLibNull/SerialPortExtLibNull.c" does.
Returning "unsupported", and setting Control to zero is similarly safe.
However, in general, a function should not modify output parameters
(unless explicitly specified so) if it returns "unsupported".
Returning "unsupported" and *not* setting Control to zero is unsafe at
the moment (although it would be self-consistent for the GetControl()
function itself). The problem is that Control would be indeterminate at
the time of the check, meaning the EFI_SERIAL_INPUT_BUFFER_EMPTY bit
could be set. That would prevent GetOneKeyFromSerial() from being called
at all.
That is why I implemented the
interfaces for FdtPL011SerialPortLib in V2 patch series. And another
EarlyFdtPL011SerialPortLib does not need implement the interfaces or
just return RETURN_UNSUPPORTED as it does not link to SerialDxe, you can
also the implementation of SerialPortRead() and SerialPortPoll() in
EarlyFdtPL011SerialPortLib.c that do nothing.
I've been missing the above GetControl() context all this time.
At this point my opinion is as follows:
- Your patch "[edk2] [PATCH V3 10/12] OvmfPkg XenConsoleSerialPortLib:
Implement Get(Set)Control/SetAttributes" is correct, and my R-b stands.
- Your patch "[edk2] [PATCH V3 11/12] ArmVirtPkg: Use SerialDxe in
MdeModulePkg instead of EmbeddedPkg" (i.e., this patch) is *also*
correct:
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
