Re: [PATCH] MdeModulePkg SerialDxe: Handle Timeout change more robustly

["Zeng, Star" <star.zeng@intel.com>]

Laszlo,

I agree it could be more rigorous to compare against the original parameters.
Please check the V2 patch at https://lists.01.org/pipermail/edk2-devel/2017-November/016968.html.

Julien,

Please help take the test on your case with the V2 patch.

Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Tuesday, November 7, 2017 5:43 AM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Julien Grall <julien.grall@linaro.org>; Ni, Ruiyu <ruiyu.ni@intel.com>
Subject: Re: [PATCH] MdeModulePkg SerialDxe: Handle Timeout change more robustly

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.grall@linaro.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@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 <lersek@redhat.com>

Julien, will you test this?

Thanks!
Laszlo