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

[PATCH v2 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]).

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 | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

-- 
2.11.0

[PATCH v2 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>
---
 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index 964d0329f4..5946a6e2f3 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -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
-- 
2.11.0

[PATCH v2 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>
---
 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index 5946a6e2f3..08b95a4a33 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -264,8 +264,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 +324,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;
     }
   }
 
-- 
2.11.0

[PATCH v2 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 propated 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 instropecting 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>
---
 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 08b95a4a33..53d2785fed 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -239,6 +239,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 v2 0/3] MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not supported

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com> with some minor comments in the separated patches.

Thanks,
Star
-----Original Message-----
From: Julien Grall [mailto:julien.grall@linaro.org] 
Sent: Saturday, November 25, 2017 12:20 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 v2 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]).

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 | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

--
2.11.0

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

[Zeng, Star]

Could you also update the description for EFI_SERIAL_SET_ATTRIBUTES in MdePkg/Include/Protocol/SerialIo.h? That could be done in a separated patch.

Thanks,
Star
-----Original Message-----
From: Julien Grall [mailto:julien.grall@linaro.org] 
Sent: Saturday, November 25, 2017 12:20 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 v2 1/3] MdeModulePkg/SerialDxe: Describe correctly EFI_DEVICE_ERROR for SetAttributes

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>
---
 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index 964d0329f4..5946a6e2f3 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -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
--
2.11.0

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

[Zeng, Star]

Could you also update the description for EFI_SERIAL_SET_ATTRIBUTES in MdePkg/Include/Protocol/SerialIo.h? That could be done in a separated patch.

+  @retval EFI_INVALID_PARAMETER One or more attributes has an unsupported value.

Thanks,
Star
-----Original Message-----
From: Julien Grall [mailto:julien.grall@linaro.org] 
Sent: Saturday, November 25, 2017 12:20 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 v2 2/3] MdeModulePkg/SerialDxe: Fix return valued in SerialSetAttributes

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>
---
 MdeModulePkg/Universal/SerialDxe/SerialIo.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index 5946a6e2f3..08b95a4a33 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -264,8 +264,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 +324,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;
     }
   }
 
--
2.11.0

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

[Zeng, Star]

Typos in commit log?
Propated -> propagated
Instropecting -> introspecting

Thanks,
Star
-----Original Message-----
From: Julien Grall [mailto:julien.grall@linaro.org] 
Sent: Saturday, November 25, 2017 12:21 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 v2 3/3] MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not supported

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 propated 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 instropecting 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>
---
 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 08b95a4a33..53d2785fed 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -239,6 +239,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 v2 0/3] MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not supported

[Julien Grall]

Hi Star,

On 27/11/17 01:46, Zeng, Star wrote:
> Reviewed-by: Star Zeng <star.zeng@intel.com> with some minor comments in the separated patches.

Thank you for the review. I will resend the series with updating the 
description in the corresponding headers. Would you mind if I keep you 
reviewed-by?

Regards,

> 
> Thanks,
> Star
> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@linaro.org]
> Sent: Saturday, November 25, 2017 12:20 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 v2 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]).
> 
> 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 | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> --
> 2.11.0
> 

-- 
Julien Grall

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

[Zeng, Star]

Sure. :)


Thanks,
Star
-----Original Message-----
From: Julien Grall [mailto:julien.grall@linaro.org] 
Sent: Monday, November 27, 2017 6:39 PM
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; Ni, Ruiyu <ruiyu.ni@intel.com>
Subject: Re: [PATCH v2 0/3] MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not supported

Hi Star,

On 27/11/17 01:46, Zeng, Star wrote:
> Reviewed-by: Star Zeng <star.zeng@intel.com> with some minor comments in the separated patches.

Thank you for the review. I will resend the series with updating the description in the corresponding headers. Would you mind if I keep you reviewed-by?

Regards,

> 
> Thanks,
> Star
> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@linaro.org]
> Sent: Saturday, November 25, 2017 12:20 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 v2 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]).
> 
> 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 | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> --
> 2.11.0
> 

--
Julien Grall

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

[Zeng, Star]

I mean not mind.

-----Original Message-----
From: Zeng, Star 
Sent: Monday, November 27, 2017 6:47 PM
To: Julien Grall <julien.grall@linaro.org>; Dong, Eric <eric.dong@intel.com>; pankaj.bansal@nxp.com; lersek@redhat.com; leif.lindholm@linaro.org
Cc: edk2-devel@lists.01.org; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [PATCH v2 0/3] MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not supported

Sure. :)


Thanks,
Star
-----Original Message-----
From: Julien Grall [mailto:julien.grall@linaro.org]
Sent: Monday, November 27, 2017 6:39 PM
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; Ni, Ruiyu <ruiyu.ni@intel.com>
Subject: Re: [PATCH v2 0/3] MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not supported

Hi Star,

On 27/11/17 01:46, Zeng, Star wrote:
> Reviewed-by: Star Zeng <star.zeng@intel.com> with some minor comments in the separated patches.

Thank you for the review. I will resend the series with updating the description in the corresponding headers. Would you mind if I keep you reviewed-by?

Regards,

> 
> Thanks,
> Star
> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@linaro.org]
> Sent: Saturday, November 25, 2017 12:20 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 v2 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]).
> 
> 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 | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> --
> 2.11.0
> 

--
Julien Grall