public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] MdeModulePkg SerialDxe: Handle Timeout change more robustly
@ 2017-11-02  1:41 Star Zeng
  2017-11-06 21:43 ` Laszlo Ersek
  0 siblings, 1 reply; 4+ messages in thread
From: Star Zeng @ 2017-11-02  1:41 UTC (permalink / raw)
  To: edk2-devel; +Cc: Star Zeng, Julien Grall, Laszlo Ersek, Ruiyu Ni

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;
+    }
   }
 
   //
-- 
2.7.0.windows.1



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] MdeModulePkg SerialDxe: Handle Timeout change more robustly
  2017-11-02  1:41 [PATCH] MdeModulePkg SerialDxe: Handle Timeout change more robustly Star Zeng
@ 2017-11-06 21:43 ` Laszlo Ersek
  2017-11-07  1:39   ` Zeng, Star
  0 siblings, 1 reply; 4+ messages in thread
From: Laszlo Ersek @ 2017-11-06 21:43 UTC (permalink / raw)
  To: Star Zeng, edk2-devel; +Cc: Julien Grall, Ruiyu Ni

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] MdeModulePkg SerialDxe: Handle Timeout change more robustly
  2017-11-06 21:43 ` Laszlo Ersek
@ 2017-11-07  1:39   ` Zeng, Star
  2017-11-07 16:09     ` Julien Grall
  0 siblings, 1 reply; 4+ messages in thread
From: Zeng, Star @ 2017-11-07  1:39 UTC (permalink / raw)
  To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Julien Grall, Ni, Ruiyu, Zeng, Star

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] MdeModulePkg SerialDxe: Handle Timeout change more robustly
  2017-11-07  1:39   ` Zeng, Star
@ 2017-11-07 16:09     ` Julien Grall
  0 siblings, 0 replies; 4+ messages in thread
From: Julien Grall @ 2017-11-07 16:09 UTC (permalink / raw)
  To: Zeng, Star, Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu

Hi Star,

On 07/11/17 01:39, Zeng, Star wrote:
> 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.

Thank you for sending the patch. I will test the v2 and tell you the result.

Cheers,

> 
> Thanks,
> Star

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2017-11-07 16:05 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-02  1:41 [PATCH] MdeModulePkg SerialDxe: Handle Timeout change more robustly Star Zeng
2017-11-06 21:43 ` Laszlo Ersek
2017-11-07  1:39   ` Zeng, Star
2017-11-07 16:09     ` Julien Grall

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox