[PATCH v3 0/3] MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not supported

[PATCH v3 0/3] MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not supported

[Julien Grall]

Hi all,


This is another attempt to fix the console issue when using UEFI in Xen guest.
This new series is based on Laszlo suggestions on the previous version
(see [1]).

For all the changes see in each patch.

Cheers,

[1] https://lists.01.org/pipermail/edk2-devel/2017-October/016181.html

Julien Grall (3):
  MdeModulePkg/SerialDxe: Describe correctly EFI_DEVICE_ERROR for
    SetAttributes
  MdeModulePkg/SerialDxe: Fix return valued in SerialSetAttributes
  MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not
    supported

 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 23 ++++++++++++++++++-----
 MdePkg/Include/Protocol/SerialIo.h          |  5 +++--
 2 files changed, 21 insertions(+), 7 deletions(-)

-- 
2.11.0

[PATCH v3 1/3] MdeModulePkg/SerialDxe: Describe correctly EFI_DEVICE_ERROR for SetAttributes

[Julien Grall]

Per the UEFIv2.7 spec, EFI_DEVICE_ERROR is returned when the serial
device is not functioning correctly. Update the description to avoid
confusion.

Contributed-under: Tianocore Contribution Agreement 1.1
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Reviewed-by: Star Zeng <star.zeng@intel.com>

---
    Star, I found a prototype for SetAttributes in SerialIo.c as well
    and updated the description there. I decided to keep the Reviewed-by
    even with that change. Let me know whether it is fine for you.

    Changes in v3:
        - Modify description of EFI_DEVICE_ERROR above the prototypes
        for SetAttributes as well.
        - Add Star reviewed-by
---
 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 4 ++--
 MdePkg/Include/Protocol/SerialIo.h          | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index 964d0329f4..19fc889c25 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -67,7 +67,7 @@ SerialReset (
                            stop bits.
 
   @retval EFI_SUCCESS      The device was reset.
-  @retval EFI_DEVICE_ERROR The serial device could not be reset.
+  @retval EFI_DEVICE_ERROR The serial device is not functioning correctly.
 
 **/
 EFI_STATUS
@@ -265,7 +265,7 @@ SerialReset (
                            stop bits.
 
   @retval EFI_SUCCESS      The device was reset.
-  @retval EFI_DEVICE_ERROR The serial device could not be reset.
+  @retval EFI_DEVICE_ERROR The serial device is not functioning correctly.
 
 **/
 EFI_STATUS
diff --git a/MdePkg/Include/Protocol/SerialIo.h b/MdePkg/Include/Protocol/SerialIo.h
index 31cd46614e..84cb34364d 100644
--- a/MdePkg/Include/Protocol/SerialIo.h
+++ b/MdePkg/Include/Protocol/SerialIo.h
@@ -126,7 +126,7 @@ EFI_STATUS
                            stop bits.
 
   @retval EFI_SUCCESS      The device was reset.
-  @retval EFI_DEVICE_ERROR The serial device could not be reset.
+  @retval EFI_DEVICE_ERROR The serial device is not functioning correctly.
 
 **/
 typedef
-- 
2.11.0

[PATCH v3 2/3] MdeModulePkg/SerialDxe: Fix return valued in SerialSetAttributes

[Julien Grall]

SerialSetAttributes is meant to match the behavior of the function
EFI_SERIAL_IO_PROTOCOL.SetAttributes() in the UEFI spec (v2.7). This
means the function can only return:
    - EFI_SUCCESS
    - EFI_INVALID_PARAMETER
    - EFI_DEVICE_ERROR

However the function SerialPortSetAttributes may also validly return
EFI_UNSUPPORTED. For instance this is the case of the Xen Console
driver.

EFI_UNSUPPORTED could be also interpreted as "One or more of the attributes
has an unsupported value". So return EFI_INVALID_PARAMETER in that case.

Lastly, to prevent another return slipping in the future, all the errors
but EFI_INVALID_PARAMETERR and EFI_UNSUPPORTED will return
EFI_DEVICE_ERROR.

Contributed-under: Tianocore Contribution Agreement 1.1
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Reviewed-by: Star Zeng <star.zeng@intel.com>

---

    Star, I found a prototype for SetAttributes in SerialIo.c as well
    and updated the description there. I decided to keep the Reviewed-by
    even with that change. Let me know whether it is fine for you.

    Changes in v3:
        - Add description of EFI_INVALID_PARAMETER above the prototypes
        for SetAttributes as well.
        - Add Star reviewed-by
---
 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 14 +++++++++-----
 MdePkg/Include/Protocol/SerialIo.h          |  5 +++--
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index 19fc889c25..ee10ec7e05 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -66,8 +66,9 @@ SerialReset (
                            value of DefaultStopBits will use the device's default number of
                            stop bits.
 
-  @retval EFI_SUCCESS      The device was reset.
-  @retval EFI_DEVICE_ERROR The serial device is not functioning correctly.
+  @retval EFI_SUCCESS           The device was reset.
+  @retval EFI_INVALID_PARAMETER One or more attributes has an unsupported value.
+  @retval EFI_DEVICE_ERROR      The serial device is not functioning correctly.
 
 **/
 EFI_STATUS
@@ -264,8 +265,9 @@ SerialReset (
                            value of DefaultStopBits will use the device's default number of
                            stop bits.
 
-  @retval EFI_SUCCESS      The device was reset.
-  @retval EFI_DEVICE_ERROR The serial device is not functioning correctly.
+  @retval EFI_SUCCESS           The device was reset.
+  @retval EFI_INVALID_PARAMETER One or more attributes has an unsupported value.
+  @retval EFI_DEVICE_ERROR      The serial device is not functioning correctly.
 
 **/
 EFI_STATUS
@@ -323,8 +325,10 @@ SerialSetAttributes (
       DataBits = OriginalDataBits;
       StopBits = OriginalStopBits;
       Status = EFI_SUCCESS;
+    } else if (Status == EFI_INVALID_PARAMETER || Status == EFI_UNSUPPORTED) {
+      return EFI_INVALID_PARAMETER;
     } else {
-      return Status;
+      return EFI_DEVICE_ERROR;
     }
   }
 
diff --git a/MdePkg/Include/Protocol/SerialIo.h b/MdePkg/Include/Protocol/SerialIo.h
index 84cb34364d..1263dc4fe9 100644
--- a/MdePkg/Include/Protocol/SerialIo.h
+++ b/MdePkg/Include/Protocol/SerialIo.h
@@ -125,8 +125,9 @@ EFI_STATUS
                            value of DefaultStopBits will use the device's default number of
                            stop bits.
 
-  @retval EFI_SUCCESS      The device was reset.
-  @retval EFI_DEVICE_ERROR The serial device is not functioning correctly.
+  @retval EFI_SUCCESS           The device was reset.
+  @retval EFI_INVALID_PARAMETER One or more attributes has an unsupported value.
+  @retval EFI_DEVICE_ERROR      The serial device is not functioning correctly.
 
 **/
 typedef
-- 
2.11.0

Re: [PATCH v3 2/3] MdeModulePkg/SerialDxe: Fix return valued in SerialSetAttributes

[Laszlo Ersek]

On 11/29/17 18:28, Julien Grall wrote:
> SerialSetAttributes is meant to match the behavior of the function
> EFI_SERIAL_IO_PROTOCOL.SetAttributes() in the UEFI spec (v2.7). This
> means the function can only return:
>     - EFI_SUCCESS
>     - EFI_INVALID_PARAMETER
>     - EFI_DEVICE_ERROR
> 
> However the function SerialPortSetAttributes may also validly return
> EFI_UNSUPPORTED. For instance this is the case of the Xen Console
> driver.
> 
> EFI_UNSUPPORTED could be also interpreted as "One or more of the attributes
> has an unsupported value". So return EFI_INVALID_PARAMETER in that case.
> 
> Lastly, to prevent another return slipping in the future, all the errors
> but EFI_INVALID_PARAMETERR and EFI_UNSUPPORTED will return

"EFI_INVALID_PARAMETERR" has a typo here.

Other than that, series

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo


> EFI_DEVICE_ERROR.
> 
> Contributed-under: Tianocore Contribution Agreement 1.1
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Reviewed-by: Star Zeng <star.zeng@intel.com>
> 
> ---
> 
>     Star, I found a prototype for SetAttributes in SerialIo.c as well
>     and updated the description there. I decided to keep the Reviewed-by
>     even with that change. Let me know whether it is fine for you.
> 
>     Changes in v3:
>         - Add description of EFI_INVALID_PARAMETER above the prototypes
>         for SetAttributes as well.
>         - Add Star reviewed-by
> ---
>  MdeModulePkg/Universal/SerialDxe/SerialIo.c | 14 +++++++++-----
>  MdePkg/Include/Protocol/SerialIo.h          |  5 +++--
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> index 19fc889c25..ee10ec7e05 100644
> --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> @@ -66,8 +66,9 @@ SerialReset (
>                             value of DefaultStopBits will use the device's default number of
>                             stop bits.
>  
> -  @retval EFI_SUCCESS      The device was reset.
> -  @retval EFI_DEVICE_ERROR The serial device is not functioning correctly.
> +  @retval EFI_SUCCESS           The device was reset.
> +  @retval EFI_INVALID_PARAMETER One or more attributes has an unsupported value.
> +  @retval EFI_DEVICE_ERROR      The serial device is not functioning correctly.
>  
>  **/
>  EFI_STATUS
> @@ -264,8 +265,9 @@ SerialReset (
>                             value of DefaultStopBits will use the device's default number of
>                             stop bits.
>  
> -  @retval EFI_SUCCESS      The device was reset.
> -  @retval EFI_DEVICE_ERROR The serial device is not functioning correctly.
> +  @retval EFI_SUCCESS           The device was reset.
> +  @retval EFI_INVALID_PARAMETER One or more attributes has an unsupported value.
> +  @retval EFI_DEVICE_ERROR      The serial device is not functioning correctly.
>  
>  **/
>  EFI_STATUS
> @@ -323,8 +325,10 @@ SerialSetAttributes (
>        DataBits = OriginalDataBits;
>        StopBits = OriginalStopBits;
>        Status = EFI_SUCCESS;
> +    } else if (Status == EFI_INVALID_PARAMETER || Status == EFI_UNSUPPORTED) {
> +      return EFI_INVALID_PARAMETER;
>      } else {
> -      return Status;
> +      return EFI_DEVICE_ERROR;
>      }
>    }
>  
> diff --git a/MdePkg/Include/Protocol/SerialIo.h b/MdePkg/Include/Protocol/SerialIo.h
> index 84cb34364d..1263dc4fe9 100644
> --- a/MdePkg/Include/Protocol/SerialIo.h
> +++ b/MdePkg/Include/Protocol/SerialIo.h
> @@ -125,8 +125,9 @@ EFI_STATUS
>                             value of DefaultStopBits will use the device's default number of
>                             stop bits.
>  
> -  @retval EFI_SUCCESS      The device was reset.
> -  @retval EFI_DEVICE_ERROR The serial device is not functioning correctly.
> +  @retval EFI_SUCCESS           The device was reset.
> +  @retval EFI_INVALID_PARAMETER One or more attributes has an unsupported value.
> +  @retval EFI_DEVICE_ERROR      The serial device is not functioning correctly.
>  
>  **/
>  typedef
> 

[PATCH v3 3/3] MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not supported

[Julien Grall]

After commit 91cc526b15 "MdeModulePkg/SerialDxe: Fix not able to change
serial attributes", serial is initialized using the reset method that
will call SetAttributes.

However, SetAttributes may return EFI_INVALID_PARAMETER when a driver
does not support some parameters. This will be propagated by the reset
function and lead to UEFI failing to get the console setup.

For instance, this is the case when using the Xen console driver.

Fix it by introspecting the result and return EFI_SUCCESS when the
SetAttributes report an invalid parameter (i.e EFI_INVALID_PARAMETER).

Contributed-under: Tianocore Contribution Agreement 1.1
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Reviewed-by: Star Zeng <star.zeng@intel.com>

---
    Changes in v3:
        - Add Star reviewed-by
        - Fix typoes in the commit message
---
 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index ee10ec7e05..e18cc7ed51 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -240,6 +240,15 @@ SerialReset (
                    (EFI_STOP_BITS_TYPE) This->Mode->StopBits
                    );
 
+  //
+  // The serial device may not support some of the attributes. To prevent
+  // later failure, always return EFI_SUCCESS when SetAttributes is returning
+  // EFI_INVALID_PARAMETER.
+  //
+  if (Status == EFI_INVALID_PARAMETER) {
+    return EFI_SUCCESS;
+  }
+
   return Status;
 }
 
-- 
2.11.0

Re: [PATCH v3 0/3] MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not supported

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com> and pushed the patches at https://github.com/tianocore/edk2/compare/f1f7190bf3bf...7ce5af40c98b with the typo (pointed out by Laszlo) fixed in [Patch 2/3].

Thanks,
Star
-----Original Message-----
From: Julien Grall [mailto:julien.grall@linaro.org] 
Sent: Thursday, November 30, 2017 1:28 AM
To: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; pankaj.bansal@nxp.com; lersek@redhat.com; leif.lindholm@linaro.org
Cc: edk2-devel@lists.01.org; Julien Grall <julien.grall@linaro.org>
Subject: [PATCH v3 0/3] MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not supported

Hi all,


This is another attempt to fix the console issue when using UEFI in Xen guest.
This new series is based on Laszlo suggestions on the previous version (see [1]).

For all the changes see in each patch.

Cheers,

[1] https://lists.01.org/pipermail/edk2-devel/2017-October/016181.html

Julien Grall (3):
  MdeModulePkg/SerialDxe: Describe correctly EFI_DEVICE_ERROR for
    SetAttributes
  MdeModulePkg/SerialDxe: Fix return valued in SerialSetAttributes
  MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not
    supported

 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 23 ++++++++++++++++++-----
 MdePkg/Include/Protocol/SerialIo.h          |  5 +++--
 2 files changed, 21 insertions(+), 7 deletions(-)

--
2.11.0