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

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

[Julien Grall]

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 )
+      return RETURN_SUCCESS;
 
   return Status;
 }
-- 
2.11.0

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

[Laszlo Ersek]

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

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

[Ni, Ruiyu]

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

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

[Zeng, Star]

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

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

[Julien Grall]

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