I do not think SerialDxe should handle EFI_INVALID_PARAMETER return value from SerialPortSetAttributes(). If the EFI_INVALID_PARAMETER is because of Timeout value, the Timeout value definitely should not be updated. If the EFI_INVALID_PARAMETER is because of other parameters, the status should be returned to the caller.
Thanks, Star -----Original Message----- From: edk2-devel [mailto:[email protected]] On Behalf Of Laszlo Ersek Sent: Friday, October 27, 2017 11:44 PM To: Julien Grall <[email protected]>; Zeng, Star <[email protected]>; edk2-devel-01 <[email protected]>; [email protected]; Ni, Ruiyu <[email protected]> Cc: Anthony PERARD <[email protected]>; Leif Lindholm <[email protected]>; Ard Biesheuvel <[email protected]> Subject: Re: [edk2] Xen Console input very slow in recent UEFI On 10/27/17 15:19, Julien Grall wrote: > 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. I think you are right. Thanks Laszlo > >>> + (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, > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
