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

[Julien Grall <julien.grall@linaro.org>]

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