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

["Zeng, Star" <star.zeng@intel.com>]

I also agree the mapping.

Julien,
There is a typo in commit log " instropecting " should be " introspecting ".
Anyway, you may need the commit log according to the mapping, you can take care of the typo in the updated commit log.


Thanks,
Star

-----Original Message-----
From: Ni, Ruiyu 
Sent: Thursday, October 19, 2017 11:03 AM
To: Laszlo Ersek <lersek@redhat.com>; 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

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