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

Message ID 20171129172823.2906-3-julien.grall@linaro.org
State New
Headers show
Series
  • MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not supported
Related show

Commit Message

Julien Grall Nov. 29, 2017, 5:28 p.m.
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(-)

-- 
2.11.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek Nov. 29, 2017, 6:24 p.m. | #1
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

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

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