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

[Laszlo Ersek <lersek@redhat.com>]

On 10/18/17 12:19, Julien Grall wrote:
> 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 not be supported by the driver and will
> return an error (i.e RETURN_UNSUPPORTED) that will be propagate 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 RETURN_SUCCESS when the
> driver report it is not supported (i.e RETURN_UNSUPPORTED).
> 
> Contributed-under: Tianocore Contribution Agreement 1.1
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  MdeModulePkg/Universal/SerialDxe/SerialIo.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> index ebcd927263..4253e0b8ea 100644
> --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
> @@ -238,6 +238,12 @@ SerialReset (
>                     (UINT8) This->Mode->DataBits,
>                     (EFI_STOP_BITS_TYPE) This->Mode->StopBits
>                     );
> +  //
> +  // The serial device may not support SetAttributes.
> +  // Set the status to RETURN_SUCCESS to prevent later failure.
> +  //
> +  if ( Status == RETURN_UNSUPPORTED )

The extra spaces within the parens are Xen coding style, not edk2 coding
style; please remove them.

> +      return RETURN_SUCCESS;

The edk2 coding style requires braces.

The edk2 coding style requires two spaces as indentation, in this context.

>  
>    return Status;
>  }
> 

The UEFI spec (v2.7) describes the following return values for
EFI_SERIAL_IO_PROTOCOL.Reset():

- EFI_SUCCESS: The serial device was reset.
- EFI_DEVICE_ERROR: The serial device could not be reset.

For the EFI_SERIAL_IO_PROTOCOL.SetAttributes() member function:

- EFI_SUCCESS: The new attributes were set on the serial device.
- EFI_INVALID_PARAMETER: One or more of the attributes has an
                         unsupported value.
- EFI_DEVICE_ERROR: The serial device is not functioning correctly.

In MdeModulePkg/Universal/SerialDxe, the SetAttributes() member function
is implemented by SerialSetAttributes(), and it delegates the operation
to the SerialPortSetAttributes() API from the following library class:

  MdePkg/Include/Library/SerialPortLib.h

The API defines the following return codes:

  @retval RETURN_SUCCESS            The new attributes were set on the
                                    serial device.
  @retval RETURN_UNSUPPORTED        The serial device does not support
                                    this operation.
  @retval RETURN_INVALID_PARAMETER  One or more of the attributes has an
                                    unsupported value.
  @retval RETURN_DEVICE_ERROR       The serial device is not functioning
                                    correctly.

Therefore I think the following should be done:

(1) The SerialPortSetAttributes() implementation in
"OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c" is
correct; returning RETURN_UNSUPPORTED is valid, according to the lib
class header.

(2) The direct propagation of the return value in SerialSetAttributes()
[MdeModulePkg/Universal/SerialDxe/SerialIo.c] does not look correct. It
should implement the following mapping:

RETURN_SUCCESS           -> EFI_SUCCESS
RETURN_UNSUPPORTED       -> EFI_INVALID_PARAMETER
RETURN_INVALID_PARAMETER -> EFI_INVALID_PARAMETER
everything else          -> EFI_DEVICE_ERROR

(That is, the outer interface requires us to map

  "The serial device does not support this operation."

to

  "One or more of the attributes has an unsupported value."

... As long as we want to adhere to the UEFI-2.7 spec.)

(3) The SerialReset() function in
"MdeModulePkg/Universal/SerialDxe/SerialIo.c" transparently propagates
the return value of the internally called SerialSetAttributes()
function. This looks incorrect as well; it should do the following mapping:

EFI_SUCCESS           -> EFI_SUCCESS
EFI_INVALID_PARAMETER -> EFI_SUCCESS
everything else       -> EFI_DEVICE_ERROR

The idea (correctly captured in your patch, IMO) is that we're already
past the SerialPortInitialize() function, so restoring the attributes is
"best effort"; if it fails, we should still report the reset operation
as successful.

I just think that EFI_SERIAL_IO_PROTOCOL.SetAttributes() is not supposed
to return EFI_UNSUPPORTED at all (it is an external interface governed
by the UEFI spec).

I suggest waiting for feedback from Star & Eric. Dependent on their
response, your patch could be good enough (once you fix up the coding
style issues). Or else, they could agree with me that the return value
mapping of SerialSetAttributes() should be corrected first (2), and then
your patch should be please adapted as well (3).

Bonus comment:

(4) The propagation of the SerialPortInitialize() retval in
SerialReset() looks correct, thankfully. (Both callee and caller are
expected to return one of *_SUCCESS and *_DEVICE_ERROR.)

Thanks
Laszlo