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-boun...@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Wednesday, October 18, 2017 8:12 PM > To: Julien Grall <julien.gr...@linaro.org>; Zeng, Star <star.z...@intel.com>; > Dong, Eric <eric.d...@intel.com>; pankaj.ban...@nxp.com; > leif.lindh...@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.gr...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
