| Message ID | 20171018101951.1890-1-julien.grall@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [edk2] MdeModulePkg/SerialDxe: Do not fail reset when SetAttributes is not supported | expand |
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
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Hi Laszlo, On 18/10/17 13:11, Laszlo Ersek wrote: > 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. Sorry for that, I will fix the coding style in the next version. [...] > 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). Thank you for the detailed explanation. I will wait a couple of days more for feedbacks and then send a new version. > > 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.) Cheers, -- Julien Grall _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 ) + return RETURN_SUCCESS; return Status; }
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(+) -- 2.11.0 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel