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

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

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