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

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

Laszlo,
I agree with your status mapping.
It will make the implementation more clear and easier to maintain.

Thanks/Ray

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Wednesday, October 18, 2017 8:12 PM
> To: Julien Grall <julien.grall@linaro.org>; Zeng, Star <star.zeng@intel.com>;
> Dong, Eric <eric.dong@intel.com>; pankaj.bansal@nxp.com;
> leif.lindholm@linaro.org
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] MdeModulePkg/SerialDxe: Do not fail reset
> when SetAttributes is not supported
> 
> 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
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel