Hi Laszlo & Star,
On 27/10/17 13:38, Laszlo Ersek wrote:
On 10/27/17 05:20, Zeng, Star wrote:
Hi,
The TimeOut handling in SerialRead() in SerialDxe(MdeModulepkg), IsaSerialRead() in
IsaSerialDxe(IntelFrameworkModulePkg) and SerialRead() in PciSioSerialDxe(MdeModulePkg)
are consistent, and we did not see this kind of "slow down" before.
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.
The slow down could be observed on BaseSerialPortLib16550 if you pass
invalid parameters. Therefore EFI_INVALID_PARAMETER will be returned.
But some implementations of SerialPortSetAttributes() just return RETURN_UNSUPPORTED, for
example XenConsoleSerialPortLib, then Timeout value will be not changed and kept to be
1000000 (1s), "slow down" will be observed.
Here, how about 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.
I can't speak authoritatively on Xen's behalf, of course, but option (2)
appears sane to me -- it is a virtual serial port; in theory it should
be able to accept all these parameter values.
(My understanding is that the virtual serial port need not change its
*behavior* based on the changed attributes. I.e., when keystrokes are
available, it doesn't have to slow down itself in providing those
keystrokes, just so it match the baud rate.)
That's correct, none of the parameters but Timeout matters for Xen. But
given that other driver are using that value, would not it be better to
use the patch suggested by Start (see below)?
And SerialDxe may can be enhanced like below to be more robust.
You definitely want such patch in the tree as EFI_UNSUPPORTED is a valid
return parameter and used by other serial driver (for instance
DxeEmuSerialPortLib).
==========
6ec9c40f91fc675ee77f3e54aea4e5a41a2de504
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) &&
I would also check EFI_INVALID_PARAMETER as the implementation may not
support some values, but it is still fine to modify the Timeout.
+ (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;
+ }
}
//
====================
Cheers,
--
Julien Grall
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
