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

[Laszlo Ersek <lersek@redhat.com>]

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
>