public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Sami Mujawar" <sami.mujawar@arm.com>
To: Leif Lindholm <leif@nuviainc.com>,
	"devel@edk2.groups.io" <devel@edk2.groups.io>,
	"lersek@redhat.com" <lersek@redhat.com>
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>,
	"graeme.gregory@linaro.org" <graeme.gregory@linaro.org>,
	"tanmay.jagdale@linaro.org" <tanmay.jagdale@linaro.org>
Subject: Re: [edk2-devel] [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count
Date: Wed, 20 May 2020 09:16:15 +0000	[thread overview]
Message-ID: <DB7PR08MB30974E3519129A87226DF57284B60@DB7PR08MB3097.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <20200519111550.GM10467@vanye>

Hi Leif,

Please find my response marked inline as [SAMI]

Regards,

Sami Mujawar

-----Original Message-----
From: Leif Lindholm <leif@nuviainc.com>
Sent: 19 May 2020 12:16 PM
To: devel@edk2.groups.io; lersek@redhat.com
Cc: Ard Biesheuvel <Ard.Biesheuvel@arm.com>; graeme.gregory@linaro.org; tanmay.jagdale@linaro.org; Sami Mujawar <Sami.Mujawar@arm.com>
Subject: Re: [edk2-devel] [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count

+Sami

On Tue, May 19, 2020 at 12:00:02 +0200, 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.

That sounds useful, but should then probably get applied to the TerminalType as well, so could come in as a separate patch on top.

Sami: could this be an alternative resolution for
https://edk2.groups.io/g/devel/message/59740 ?

[SAMI]
For https://edk2.groups.io/g/devel/message/59740 we need a conditional check, as we need to decide between using the PL011UartInteger value or computing it using the UART clock and baud rate.
I tried the following and it fixes the VS2017 warning without the need of a pragma statement.

-  if (FixedPcdGet32 (PL011UartInteger) != 0) {
-    Integer = FixedPcdGet32 (PL011UartInteger);
+  Integer = FixedPcdGet32 (PL011UartInteger);
+  if (Integer != 0) {
     Fractional = FixedPcdGet32 (PL011UartFractional);
   } else {
     // If BAUD rate is zero then replace it with the system default value

The disassembly output (using GCC Objdump) for the above code, with or without this change appears to be the same.

Please let me know if this is more suitable, I can submit a patch with this change.
[/SAMI]

Regards,

Leif


> Thanks,
> Laszlo
>
>
> 
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

  reply	other threads:[~2020-05-20  9:16 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-18 17:11 [PATCH] ArmPkg/PlatformBootManagerLib: reject 'default' parity and stop bit count Ard Biesheuvel
2020-05-18 17:18 ` Leif Lindholm
2020-05-19 10:00 ` [edk2-devel] " Laszlo Ersek
2020-05-19 11:15   ` Leif Lindholm
2020-05-20  9:16     ` Sami Mujawar [this message]
2020-05-19 12:12   ` Ard Biesheuvel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DB7PR08MB30974E3519129A87226DF57284B60@DB7PR08MB3097.eurprd08.prod.outlook.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox