Re: [edk2-devel] [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

On 5/19/20 12:00 PM, Laszlo Ersek wrote:
> On 05/18/20 19:11, Ard Biesheuvel wrote:
>> In the ArmPkg version of PlatformBootManagerLib, we construct a
>> serial device path based on the default settings for baud rate,
>> parity and the number of stop bits, to ensure that a serial console
>> is available even on the very first boot.
>>
>> This assumes that PcdUartDefaultParity or PcdUartDefaultStopBits are
>> not set to '0', meaning 'the default', as there is no default for
>> these when constructing a device path.
>>
>> So add a couple of ASSERT()s to make sure that we catch this condition,
>> since it otherwise ignores the bogus device path silently, which is
>> rather tedious to debug,.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>>   ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> index e6e788e0f107..a030d510aa62 100644
>> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> @@ -583,6 +583,8 @@ PlatformBootManagerBeforeConsole (
>>     //
>>     // Add the hardcoded serial console device path to ConIn, ConOut, ErrOut.
>>     //
>> +  ASSERT (FixedPcdGet8 (PcdUartDefaultParity) > 0);
>> +  ASSERT (FixedPcdGet8 (PcdUartDefaultStopBits) > 0);
>>     ASSERT (FixedPcdGet8 (PcdDefaultTerminalType) == 4);
>>     CopyGuid (&mSerialConsole.TermType.Guid, &gEfiTtyTermGuid);
>>   
>>
> 
> Given that these are fixed PCDs, I'd recommend STATIC_ASSERT(). We've
> recently put STATIC_ASSERT() to use (in "OvmfPkg/MptScsiDxe/MptScsi.c")
> in combination with a fixed PCD:
> 
>    STATIC_ASSERT (
>      FixedPcdGet8 (PcdMptScsiMaxTargetLimit) < 255,
>      "Req supports 255 targets only (max target is 254)"
>      );
> 
> Just an idea of course; if that's out of scope for now, I have nothing
> against this patch.
> 

Thanks. This is actually much better, as it diagnoses potential issues 
with the serial console in the RELEASE build as well, which might be 
tricky to catch otherwise.