Star, On 11/02/17 02:41, Star Zeng wrote: > https://lists.01.org/pipermail/edk2-devel/2017-October/016479.html > reported "Xen Console input very slow in recent UEFI" that appears > after 4cf3f37c87ba1f9d58072444bd735e40e4779e70 "MdeModulePkg > SerialDxe: Process timeout consistently in SerialRead". > > Julien did more debugging and find out the following is happening in > TerminalConInTimerHandler (MdeModulePkg/Universal/Console/TerminalDxe) > when a character is received: > 1) GetControl will return EFI_SERIAL_INPUT_BUFFER_EMPTY unset > => Entering in the loop to fetch character from the serial > 2) GetOneKeyFromSerial() > => Return directly with the character read > 3) Looping as the fifo is not full and no error > 4) GetOneKeyFromSerial() -> SerialRead() > => No more character so SerialPortPoll() will return FALSE and loop > until timeout > => Return EFI_TIMEOUT > 5) Exiting the loop from TerminalConInTimerHandler > 6) Characters are printed > > After some investigation, I found it is related to the Timeout value. > > The Timeout is 1000000 (1s) by default to follow UEFI spec. > And the Terminal driver will recalculate and set the Timeout value > based on the properties of UART in TerminalDriverBindingStart()/ > TerminalConInTimerHandler(). > > SerialInTimeOut = 0; > if (Mode->BaudRate != 0) { > // > // According to BAUD rate to calculate the timeout value. > // > SerialInTimeOut = (1 + Mode->DataBits + Mode->StopBits) * > 2 * 1000000 / (UINTN) Mode->BaudRate; > } > > For example, based on the PCD values of PcdUartDefaultBaudRate, > PcdUartDefaultDataBits and PcdUartDefaultStopBits, SerialInTimeOut = > (1 + 8 + 1) * 2 * 1000000 / (UINTN) 115200 = 173 (us). > > When SerialDxe is used, > TerminalDriverBindingStart()/TerminalConInTimerHandler() -> > SerialIo->SetAttributes() -> > SerialSetAttributes() -> > SerialPortSetAttributes() > > Some implementations of SerialPortSetAttributes() could handle the > input parameters and return RETURN_SUCCESS, for example > BaseSerialPortLib16550, then Timeout value will be changed to 173 (us), > no "slow down" will be observed. > But some implementations of SerialPortSetAttributes() just return > RETURN_UNSUPPORTED, for example XenConsoleSerialPortLib, then Timeout > value will be not changed and kept 1000000 (1s), "slow down" will be > observed. > > SerialPortLib instance can be enhanced to > 1. Handle the input parameters and return status accordingly instead of > just returning RETURN_UNSUPPORTED in SerialPortSetAttributes(). > 2. Just return RETURN_SUCCESS instead of RETURN_UNSUPPORTED in > SerialPortSetAttributes() if the instance does not care the input > parameters at all. > > And SerialDxe can also be enhanced like this patch to be more robust > to handle Timeout change. > > Cc: Julien Grall <julien.gr...@linaro.org> > Cc: Laszlo Ersek <ler...@redhat.com> > Cc: Ruiyu Ni <ruiyu...@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Star Zeng <star.z...@intel.com> > --- > MdeModulePkg/Universal/SerialDxe/SerialIo.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c > b/MdeModulePkg/Universal/SerialDxe/SerialIo.c > index ebcd92726314..060ea56c2b1a 100644 > --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c > +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c > @@ -285,7 +285,21 @@ SerialSetAttributes ( > > Status = SerialPortSetAttributes (&BaudRate, &ReceiveFifoDepth, &Timeout, > &Parity, &DataBits, &StopBits); > if (EFI_ERROR (Status)) { > - return Status; > + // > + // If it is just to set Timeout value and unsupported is returned, > + // do not return error. > + // > + if ((Status == EFI_UNSUPPORTED) && > + (This->Mode->Timeout != Timeout) && > + (This->Mode->ReceiveFifoDepth == ReceiveFifoDepth) && > + (This->Mode->BaudRate == BaudRate) && > + (This->Mode->DataBits == (UINT32) DataBits) && > + (This->Mode->Parity == (UINT32) Parity) && > + (This->Mode->StopBits == (UINT32) StopBits)) { > + Status = EFI_SUCCESS; > + } else { > + return Status; > + } > } > > // >
is the SerialPortSetAttributes() library API allowed to overwrite -- possibly even with garbage -- the IN OUT parameters if it fails? Practice is inconsistent on this; some library APIs explicitly "taint" output parameters on error, while others explicitly "preserve" input/output parameters on error. Yet others (a few exceptional APIs) output valid / sensible values *even* on error. This API in particular promises neither (in the lib class header), so I don't know. If it is not much burden, I'd prefer comparisons against the original parameters, not against those that were possibly modified by SerialPortSetAttributes(), before it returned EFI_UNSUPPORTED. I'll leave it up to you to decide. Reviewed-by: Laszlo Ersek <ler...@redhat.com> Julien, will you test this? Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
