[edk2] [PATCH V2 0/3] Some improvements on serial terminal

Change default terminal type to be consistent with default
ConIn/ConOut device path, which is now determined by TTY_TERMINAL
flag, TTYTERM or VT100.

I can't say this is a bug, as we can pass the whole console device
path to ConnectController, and TerminalDxe driver will pick up the
terminal in the remaining device path. However, in rare circumstances,
the console devices may be disconnected with the driver, and they will
be ignored by ConPlatformDxe until we pass the device path explicitly
just as BDS.

Changing default terminal type to be the same with console device
path could help serial terminal be reconnected with normal connect
controller operation.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <***@linaro.org>
---
ArmVirtPkg/ArmVirtQemu.dsc | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index fbc2b12..e62624f 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -141,9 +141,15 @@
!if $(TTY_TERMINAL) == TRUE
gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenMsg(7D916D80-5BB1-458C-A48F-E25FDD51EF94)"
gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenMsg(7D916D80-5BB1-458C-A48F-E25FDD51EF94)"
+ ## Terminal Type - TTY, consistent with ConOut/ConIn Device Path.
+ gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|4
!else
gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenVt100()"
gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenVt100()"
+ ## Terminal Type - VT100, consistent with ConOut/ConIn Device Path.
+ ## When Intel BDS is enabled, the above ConOut/ConIn device path is useless,
+ ## but we still use VT100 terminal type when TTY_TERMINAL is not TRUE.
+ gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1
!endif
gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3

--
2.1.4

Laszlo Ersek

2015-07-13 11:15:56 UTC

Post by Heyi Guo
Change default terminal type to be consistent with default
ConIn/ConOut device path, which is now determined by TTY_TERMINAL
flag, TTYTERM or VT100.
I can't say this is a bug, as we can pass the whole console device
path to ConnectController, and TerminalDxe driver will pick up the
terminal in the remaining device path. However, in rare circumstances,
the console devices may be disconnected with the driver, and they will
be ignored by ConPlatformDxe until we pass the device path explicitly
just as BDS.
Changing default terminal type to be the same with console device
path could help serial terminal be reconnected with normal connect
controller operation.
Contributed-under: TianoCore Contribution Agreement 1.0
---
ArmVirtPkg/ArmVirtQemu.dsc | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index fbc2b12..e62624f 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -141,9 +141,15 @@
!if $(TTY_TERMINAL) == TRUE
gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenMsg(7D916D80-5BB1-458C-A48F-E25FDD51EF94)"
gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenMsg(7D916D80-5BB1-458C-A48F-E25FDD51EF94)"
+ ## Terminal Type - TTY, consistent with ConOut/ConIn Device Path.
+ gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|4
!else
gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenVt100()"
gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenVt100()"
+ ## Terminal Type - VT100, consistent with ConOut/ConIn Device Path.
+ ## When Intel BDS is enabled, the above ConOut/ConIn device path is useless,
+ ## but we still use VT100 terminal type when TTY_TERMINAL is not TRUE.
+ gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1
!endif
gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3

Reviewed-by: Laszlo Ersek <***@redhat.com>

Ard Biesheuvel

2015-07-13 11:22:03 UTC

I am OK with the patch. However, the value '4' is not defined for
PcdDefaultTerminalType is not defined in MdePkg.
Is that a concern to anyone? I am aware that we suggested to Roy not
to touch MdePkg with his TtyTerm changes, but it seems odd to just use
'4' here

Laszlo Ersek

2015-07-13 11:34:00 UTC

See "MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h", commit
6e3227c8. (The TTYTERMTYPE macro.)

What should belong to MdePkg (but can't, unless it gets standardized) is
the textual representation for the new VenMsg(GUID).

... Hm, okay, you do have a point, I just checked "MdePkg/MdePkg.dec",
and it does list the values 0-3. However, the PCD is not used inside
MdePkg, it is only declared there.

So, I think that this patch is okay. If another terminal type gets
standardized *before* Roy's TTYTERM, then:
- that type will get a new (different) GUID,
- and it will get value 4 in "MdePkg/MdePkg.dec". (And, in Terminal.h.)

The new GUID will obviously not conflict, and the current TTYTERMTYPE=4
constant will be possible to shift up to TTYTERMTYPE=5, at that point.
Yes, referring DSCs will have to be updated as well.

This is not 100% "clean", admittedly. I think the root issue is that the
PCD should not be declared in MdePkg at all; at least (off the top off
my head) I doubt the numeric value comes from the UEFI spec. So, maybe
someone can submit a patch that moves the PCD declaration from MdePkg to
MdeModulePkg. Otherwise, I'd be fine with the above "plan". So my R-b
stands.

Thanks
Laszlo

Laszlo Ersek

2015-07-14 10:15:36 UTC

See "MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h", commit
6e3227c8. (The TTYTERMTYPE macro.)
What should belong to MdePkg (but can't, unless it gets standardized) is
the textual representation for the new VenMsg(GUID).
... Hm, okay, you do have a point, I just checked "MdePkg/MdePkg.dec",
and it does list the values 0-3. However, the PCD is not used inside
MdePkg, it is only declared there.
So, I think that this patch is okay. If another terminal type gets
- that type will get a new (different) GUID,
- and it will get value 4 in "MdePkg/MdePkg.dec". (And, in Terminal.h.)
The new GUID will obviously not conflict, and the current TTYTERMTYPE=4
constant will be possible to shift up to TTYTERMTYPE=5, at that point.
Yes, referring DSCs will have to be updated as well.
This is not 100% "clean", admittedly. I think the root issue is that the
PCD should not be declared in MdePkg at all; at least (off the top off
my head) I doubt the numeric value comes from the UEFI spec. So, maybe
someone can submit a patch that moves the PCD declaration from MdePkg to
MdeModulePkg. Otherwise, I'd be fine with the above "plan". So my R-b
stands.

So what do you think, Ard? With Feng's R-b for patches #2 and #3, we
could commit this series to SVN -- if you agree with #1.

Thanks
Laszlo

Ard Biesheuvel

2015-07-14 10:18:52 UTC

See "MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h", commit
6e3227c8. (The TTYTERMTYPE macro.)
What should belong to MdePkg (but can't, unless it gets standardized) is
the textual representation for the new VenMsg(GUID).
... Hm, okay, you do have a point, I just checked "MdePkg/MdePkg.dec",
and it does list the values 0-3. However, the PCD is not used inside
MdePkg, it is only declared there.
So, I think that this patch is okay. If another terminal type gets
- that type will get a new (different) GUID,
- and it will get value 4 in "MdePkg/MdePkg.dec". (And, in Terminal.h.)
The new GUID will obviously not conflict, and the current TTYTERMTYPE=4
constant will be possible to shift up to TTYTERMTYPE=5, at that point.
Yes, referring DSCs will have to be updated as well.
This is not 100% "clean", admittedly. I think the root issue is that the
PCD should not be declared in MdePkg at all; at least (off the top off
my head) I doubt the numeric value comes from the UEFI spec. So, maybe
someone can submit a patch that moves the PCD declaration from MdePkg to
MdeModulePkg. Otherwise, I'd be fine with the above "plan". So my R-b
stands.

So what do you think, Ard? With Feng's R-b for patches #2 and #3, we
could commit this series to SVN -- if you agree with #1.

Yes, I am fine with this series. Please go ahead and commit it, or I
can do it instead if you're busy.

In the mean time, I will follow up with a patch that moves
PcdDefaultTerminalType to MdeModulePkg, and one that adds the missing
#4 in the comments.

Cheers,
Ard.

Ard Biesheuvel

2015-07-14 10:27:06 UTC

See "MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h", commit
6e3227c8. (The TTYTERMTYPE macro.)
What should belong to MdePkg (but can't, unless it gets standardized) is
the textual representation for the new VenMsg(GUID).
... Hm, okay, you do have a point, I just checked "MdePkg/MdePkg.dec",
and it does list the values 0-3. However, the PCD is not used inside
MdePkg, it is only declared there.
So, I think that this patch is okay. If another terminal type gets
- that type will get a new (different) GUID,
- and it will get value 4 in "MdePkg/MdePkg.dec". (And, in Terminal.h.)
The new GUID will obviously not conflict, and the current TTYTERMTYPE=4
constant will be possible to shift up to TTYTERMTYPE=5, at that point.
Yes, referring DSCs will have to be updated as well.
This is not 100% "clean", admittedly. I think the root issue is that the
PCD should not be declared in MdePkg at all; at least (off the top off
my head) I doubt the numeric value comes from the UEFI spec. So, maybe
someone can submit a patch that moves the PCD declaration from MdePkg to
MdeModulePkg. Otherwise, I'd be fine with the above "plan". So my R-b
stands.

So what do you think, Ard? With Feng's R-b for patches #2 and #3, we
could commit this series to SVN -- if you agree with #1.

Yes, I am fine with this series. Please go ahead and commit it, or I
can do it instead if you're busy.
In the mean time, I will follow up with a patch that moves
PcdDefaultTerminalType to MdeModulePkg, and one that adds the missing
#4 in the comments.

On second thought, perhaps it might be slightly cleaner to introduce
gEfiMdeModulePkgTokenSpaceGuid.PcdDefaultTerminalType first and move
existing code in TerminalDxe to it?

Laszlo Ersek

2015-07-14 10:48:32 UTC

See "MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h", commit
6e3227c8. (The TTYTERMTYPE macro.)
What should belong to MdePkg (but can't, unless it gets standardized) is
the textual representation for the new VenMsg(GUID).
... Hm, okay, you do have a point, I just checked "MdePkg/MdePkg.dec",
and it does list the values 0-3. However, the PCD is not used inside
MdePkg, it is only declared there.
So, I think that this patch is okay. If another terminal type gets
- that type will get a new (different) GUID,
- and it will get value 4 in "MdePkg/MdePkg.dec". (And, in Terminal.h.)
The new GUID will obviously not conflict, and the current TTYTERMTYPE=4
constant will be possible to shift up to TTYTERMTYPE=5, at that point.
Yes, referring DSCs will have to be updated as well.
This is not 100% "clean", admittedly. I think the root issue is that the
PCD should not be declared in MdePkg at all; at least (off the top off
my head) I doubt the numeric value comes from the UEFI spec. So, maybe
someone can submit a patch that moves the PCD declaration from MdePkg to
MdeModulePkg. Otherwise, I'd be fine with the above "plan". So my R-b
stands.

So what do you think, Ard? With Feng's R-b for patches #2 and #3, we
could commit this series to SVN -- if you agree with #1.

Yes, I am fine with this series. Please go ahead and commit it, or I
can do it instead if you're busy.
In the mean time, I will follow up with a patch that moves
PcdDefaultTerminalType to MdeModulePkg, and one that adds the missing
#4 in the comments.

On second thought, perhaps it might be slightly cleaner to introduce
gEfiMdeModulePkgTokenSpaceGuid.PcdDefaultTerminalType first and move
existing code in TerminalDxe to it?

If Heyi doesn't mind, I think that would be the cleanest, yes.

Let's hope your "move PcdDefaultTerminalType to MdeModulePkg" patch
series will get reviewed quickly, and then Heyi can post a v3. Heyi, is
that okay with you?

Thanks
Laszlo

Laszlo Ersek

2015-07-15 11:20:23 UTC

Heyi,

can you please prepend another patch to this series that updates the
PcdDefaultTerminalType declaration in MdePkg/MdePkg.dec, with the new
value 4, and a hint that TTYTERM, identified by value 4, is not defined
by the UEFI spec?

Please see here (unless you've seen it already... hm, right, you were Cc'd):

http://thread.gmane.org/gmane.comp.bios.tianocore.devel/17359/focus=17421

Thanks!
Laszlo

Heyi Guo

2015-07-15 23:30:27 UTC

No problem :)

Heyi

Post by Laszlo Ersek
Heyi,

can you please prepend another patch to this series that updates the
PcdDefaultTerminalType declaration in MdePkg/MdePkg.dec, with the new
value 4, and a hint that TTYTERM, identified by value 4, is not defined
by the UEFI spec?
http://thread.gmane.org/gmane.comp.bios.tianocore.devel/17359/focus=17421
Thanks!
Laszlo

Heyi Guo

2015-07-13 03:24:36 UTC

1. Get default terminal type from PCD rather than using PCANSI
directly in BuildTeminalDevpath;
2. Only terminal type is needed to create an TerminalDev instance, so
remove the useless code of creating and freeing DefaultNode.
3. Some white space refining.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <***@linaro.org>
---
.../Universal/Console/TerminalDxe/Terminal.c | 27 ++++------------------
1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
index 75bfdec..6fde3b2 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
@@ -258,14 +258,13 @@ BuildTerminalDevpath (
EFI_STATUS Status;

TerminalDevicePath = NULL;
- TerminalType = PCANSITYPE;

//
// Use the RemainingDevicePath to determine the terminal type
//
Node = (VENDOR_DEVICE_PATH *) RemainingDevicePath;
if (Node == NULL) {
- TerminalType = PCANSITYPE;
+ TerminalType = PcdGet8 (PcdDefaultTerminalType);

} else if (CompareGuid (&Node->Guid, &gEfiPcAnsiGuid)) {

@@ -545,7 +544,6 @@ TerminalDriverBindingStart (
EFI_SERIAL_IO_PROTOCOL *SerialIo;
EFI_DEVICE_PATH_PROTOCOL *ParentDevicePath;
VENDOR_DEVICE_PATH *Node;
- VENDOR_DEVICE_PATH *DefaultNode;
EFI_SERIAL_IO_MODE *Mode;
UINTN SerialInTimeOut;
TERMINAL_DEV *TerminalDevice;
@@ -565,9 +563,8 @@ TerminalDriverBindingStart (
UINTN ModeCount;

TerminalDevice = NULL;
- DefaultNode = NULL;
- ConInSelected = FALSE;
- ConOutSelected = FALSE;
+ ConInSelected = FALSE;
+ ConOutSelected = FALSE;
NullRemaining = FALSE;
SimTxtInInstalled = FALSE;
SimTxtOutInstalled = FALSE;
@@ -709,23 +706,14 @@ TerminalDriverBindingStart (
}

//
- // If RemainingDevicePath is NULL, then create default device path node
+ // If RemainingDevicePath is NULL, use default terminal type
//
if (RemainingDevicePath == NULL) {
- DefaultNode = AllocateZeroPool (sizeof (VENDOR_DEVICE_PATH));
- if (DefaultNode == NULL) {
- Status = EFI_OUT_OF_RESOURCES;
- goto Error;
- }
-
TerminalType = PcdGet8 (PcdDefaultTerminalType);
//
// Must be between PCANSITYPE (0) and TTYTERMTYPE (4)
//
ASSERT (TerminalType <= TTYTERMTYPE);
-
- CopyMem (&DefaultNode->Guid, gTerminalType[TerminalType], sizeof (EFI_GUID));
- RemainingDevicePath = (EFI_DEVICE_PATH_PROTOCOL *) DefaultNode;
} else if (!IsDevicePathEnd (RemainingDevicePath)) {
//
// If RemainingDevicePath isn't the End of Device Path Node,
@@ -1183,9 +1171,6 @@ TerminalDriverBindingStart (
goto Error;
}
}
- if (DefaultNode != NULL) {
- FreePool (DefaultNode);
- }

return EFI_SUCCESS;

@@ -1254,10 +1239,6 @@ Error:
}
}

- if (DefaultNode != NULL) {
- FreePool (DefaultNode);
- }
-
This->Stop (This, Controller, 0, NULL);

return Status;

--
2.1.4

Heyi Guo

2015-07-13 03:24:35 UTC

This is bug fix for TerminalDxe: NullRemaining should be set to FALSE
by fault and then be set to TRUE conditionally.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <***@linaro.org>
---
MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
index 597b15d..75bfdec 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
@@ -568,7 +568,7 @@ TerminalDriverBindingStart (
DefaultNode = NULL;
ConInSelected = FALSE;
ConOutSelected = FALSE;
- NullRemaining = TRUE;
+ NullRemaining = FALSE;
SimTxtInInstalled = FALSE;
SimTxtOutInstalled = FALSE;
FirstEnter = FALSE;

--
2.1.4

Tian, Feng

2015-07-14 00:21:30 UTC