From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web12.10492.1589890379339994970 for ; Tue, 19 May 2020 05:12:59 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 2A6DD30E; Tue, 19 May 2020 05:12:58 -0700 (PDT) Received: from [192.168.1.81] (unknown [10.37.8.132]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C5E613F52E; Tue, 19 May 2020 05:12:56 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count To: Laszlo Ersek , devel@edk2.groups.io Cc: leif@nuviainc.com, graeme.gregory@linaro.org, tanmay.jagdale@linaro.org References: <20200518171148.6113-1-ard.biesheuvel@arm.com> <6f410b08-f868-8b38-60fd-2b863a89658e@redhat.com> From: "Ard Biesheuvel" Message-ID: <4d75abab-d906-369c-6961-df5d9fd2525f@arm.com> Date: Tue, 19 May 2020 14:12:53 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <6f410b08-f868-8b38-60fd-2b863a89658e@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> --- >> 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.