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 <[email protected]>; Julien Grall <[email protected]>; Zeng, Star <[email protected]>; Dong, Eric <[email protected]>; [email protected]; [email protected] Cc: [email protected] 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:[email protected]] On Behalf Of > Laszlo Ersek > Sent: Wednesday, October 18, 2017 8:12 PM > To: Julien Grall <[email protected]>; Zeng, Star > <[email protected]>; Dong, Eric <[email protected]>; > [email protected]; [email protected] > Cc: [email protected] > 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 <[email protected]> > > --- > > 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 > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
