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