[edk2] [PATCH V3 0/4] Some improvements on serial terminal

[Heyi Guo]

There were some issues when I ran SCT test against Serial IO Protocol
on qemu aarch64, and below patches were made after I went through the
code of TerminalDxe driver and the flow of console devices being
connected.

V2 is based on TTY terminal patches.
V3 updates comment for PcdDefaultTerminalType.

Please help review these patches and provide your comments.

Thanks.

Heyi Guo (4):
MdePkg|EmulatorPkg: Update comment for PcdDefaultTerminalType
ArmVirtPkg: Make terminal type consistent
MdeModulePkg/TerminalDxe: Set NullRemaining to FALSE by default
MdeModulePkg/TerminalDxe: Some improvements

ArmVirtPkg/ArmVirtQemu.dsc | 8 ++++++
EmulatorPkg/EmulatorPkg.dsc | 2 +-
.../Universal/Console/TerminalDxe/Terminal.c | 29 ++++------------------
MdePkg/MdePkg.dec | 3 ++-
4 files changed, 16 insertions(+), 26 deletions(-)

[Heyi Guo]

Update comment for PcdDefaultTerminalType, as a new terminal type
TTYTERM has been added with type value of 4.

The new type is NOT defined in UEFI SPEC v2.5.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <***@linaro.org>
---
EmulatorPkg/EmulatorPkg.dsc | 2 +-
MdePkg/MdePkg.dec | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/EmulatorPkg/EmulatorPkg.dsc b/EmulatorPkg/EmulatorPkg.dsc
index e0c6161..dfcd476 100644
--- a/EmulatorPkg/EmulatorPkg.dsc
+++ b/EmulatorPkg/EmulatorPkg.dsc
@@ -228,7 +228,7 @@
gEmulatorPkgTokenSpaceGuid.PcdEmuCpuModel|L"Intel(R) Processor Model"
gEmulatorPkgTokenSpaceGuid.PcdEmuCpuSpeed|L"3000"

- # 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8
+ # 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8, 4-TTYTERM
gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1

[PcdsDynamicDefault.common.DEFAULT]
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index bda6550..598a6d0 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2012,8 +2012,9 @@
# 1 - VT100<BR>
# 2 - VT100+<BR>
# 3 - UTF8<BR>
+ # 4 - TTYTERM, NOT defined in UEFI SPEC<BR>
# @Prompt Default Terminal Type.
- # @ValidRange 0x80000001 | 0 - 3
+ # @ValidRange 0x80000001 | 0 - 4
gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|0|UINT8|0x00000024

## Error level for hardware recorder.

[Jordan Justen]

Can you just split this into two patches?

I think we should keep patches confined to updating a single module
unless there is a technical reason to touch separate packages in a
single patch.

Or, I suppose if you are making the *exact* same change to more than 3
packages, I guess it seems a little silly to make separate patches.

Post by Heyi Guo
Update comment for PcdDefaultTerminalType, as a new terminal type
TTYTERM has been added with type value of 4.
The new type is NOT defined in UEFI SPEC v2.5.
Contributed-under: TianoCore Contribution Agreement 1.0

In the commit message, you can Cc Andrew and I in the EmulatorPkg
patch here, and git send-email will automatically add the Cc when
sending the email.

You can add Reviewed-by: Jordan Justen <***@intel.com> to
the separate EmulatorPkg patch.

-Jordan

Post by Heyi Guo
---
EmulatorPkg/EmulatorPkg.dsc | 2 +-
MdePkg/MdePkg.dec | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/EmulatorPkg/EmulatorPkg.dsc b/EmulatorPkg/EmulatorPkg.dsc
index e0c6161..dfcd476 100644
--- a/EmulatorPkg/EmulatorPkg.dsc
+++ b/EmulatorPkg/EmulatorPkg.dsc
@@ -228,7 +228,7 @@
gEmulatorPkgTokenSpaceGuid.PcdEmuCpuModel|L"Intel(R) Processor Model"
gEmulatorPkgTokenSpaceGuid.PcdEmuCpuSpeed|L"3000"
- # 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8
+ # 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8, 4-TTYTERM
gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1
[PcdsDynamicDefault.common.DEFAULT]
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index bda6550..598a6d0 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2012,8 +2012,9 @@
# 1 - VT100<BR>
# 2 - VT100+<BR>
# 3 - UTF8<BR>
+ # 4 - TTYTERM, NOT defined in UEFI SPEC<BR>
gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|0|UINT8|0x00000024
## Error level for hardware recorder.
--
2.1.4

[Laszlo Ersek]

Post by Jordan Justen
Can you just split this into two patches?
I think we should keep patches confined to updating a single module
unless there is a technical reason to touch separate packages in a
single patch.
Or, I suppose if you are making the *exact* same change to more than 3
packages, I guess it seems a little silly to make separate patches.

Post by Heyi Guo
Update comment for PcdDefaultTerminalType, as a new terminal type
TTYTERM has been added with type value of 4.
The new type is NOT defined in UEFI SPEC v2.5.
Contributed-under: TianoCore Contribution Agreement 1.0

In the commit message, you can Cc Andrew and I in the EmulatorPkg
patch here, and git send-email will automatically add the Cc when
sending the email.

And please Cc:

M: Michael D Kinney <***@intel.com>
M: Liming Gao <***@intel.com>

on the MdePkg patch.

Thanks
Laszlo

Post by Jordan Justen
the separate EmulatorPkg patch.
-Jordan

Post by Heyi Guo
---
EmulatorPkg/EmulatorPkg.dsc | 2 +-
MdePkg/MdePkg.dec | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/EmulatorPkg/EmulatorPkg.dsc b/EmulatorPkg/EmulatorPkg.dsc
index e0c6161..dfcd476 100644
--- a/EmulatorPkg/EmulatorPkg.dsc
+++ b/EmulatorPkg/EmulatorPkg.dsc
@@ -228,7 +228,7 @@
gEmulatorPkgTokenSpaceGuid.PcdEmuCpuModel|L"Intel(R) Processor Model"
gEmulatorPkgTokenSpaceGuid.PcdEmuCpuSpeed|L"3000"
- # 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8
+ # 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8, 4-TTYTERM
gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1
[PcdsDynamicDefault.common.DEFAULT]
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index bda6550..598a6d0 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2012,8 +2012,9 @@
# 1 - VT100<BR>
# 2 - VT100+<BR>
# 3 - UTF8<BR>
+ # 4 - TTYTERM, NOT defined in UEFI SPEC<BR>
gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|0|UINT8|0x00000024
## Error level for hardware recorder.
--
2.1.4

[Gao, Liming]

Reviewed-by: Liming Gao <***@intel.com>

-----Original Message-----
From: Heyi Guo [mailto:***@linaro.org]
Sent: Thursday, July 16, 2015 10:10 AM
To: edk2-***@lists.sourceforge.net
Subject: [edk2] [PATCH V3 1/4] MdePkg|EmulatorPkg: Update comment for PcdDefaultTerminalType

Update comment for PcdDefaultTerminalType, as a new terminal type TTYTERM has been added with type value of 4.

The new type is NOT defined in UEFI SPEC v2.5.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <***@linaro.org>
---
EmulatorPkg/EmulatorPkg.dsc | 2 +-
MdePkg/MdePkg.dec | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/EmulatorPkg/EmulatorPkg.dsc b/EmulatorPkg/EmulatorPkg.dsc index e0c6161..dfcd476 100644
--- a/EmulatorPkg/EmulatorPkg.dsc
+++ b/EmulatorPkg/EmulatorPkg.dsc
@@ -228,7 +228,7 @@
gEmulatorPkgTokenSpaceGuid.PcdEmuCpuModel|L"Intel(R) Processor Model"
gEmulatorPkgTokenSpaceGuid.PcdEmuCpuSpeed|L"3000"

- # 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8
+ # 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8, 4-TTYTERM
gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1

[PcdsDynamicDefault.common.DEFAULT]
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec index bda6550..598a6d0 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2012,8 +2012,9 @@
# 1 - VT100<BR>
# 2 - VT100+<BR>
# 3 - UTF8<BR>
+ # 4 - TTYTERM, NOT defined in UEFI SPEC<BR>
# @Prompt Default Terminal Type.
- # @ValidRange 0x80000001 | 0 - 3
+ # @ValidRange 0x80000001 | 0 - 4
gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|0|UINT8|0x00000024

## Error level for hardware recorder.
--
2.1.4


------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
edk2-devel mailing list
edk2-***@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

[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
Signed-off-by: Heyi Guo <***@linaro.org>
---
ArmVirtPkg/ArmVirtQemu.dsc | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 0d4f4b0..be2b984 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -140,9 +140,17 @@
!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 - TTYTERM, consistent with ConOut/ConIn Device Path.
+ ## 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8, 4-TTYTERM
+ 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.
+ ## 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8, 4-TTYTERM
+ gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1
!endif
gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3

[Laszlo Ersek]

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 | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 0d4f4b0..be2b984 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -140,9 +140,17 @@
!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 - TTYTERM, consistent with ConOut/ConIn Device Path.
+ ## 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8, 4-TTYTERM
+ 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.
+ ## 0-PCANSI, 1-VT100, 2-VT00+, 3-UTF8, 4-TTYTERM
+ gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1
!endif
gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3

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

[Heyi Guo]

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;

[Laszlo Ersek]

Post by Heyi Guo
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
---
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;

This patch is identical to its counterpart in v2, therefore it should
have been posted with Feng's R-b added, from

http://thread.gmane.org/gmane.comp.bios.tianocore.devel/17244/focus=17309

Ditto for [v2 3/3] <-> [v3 4/4].

Basically, sometime during the preparation of the next version of a
patch series, run a dedicated "git rebase -i", set all the actions to
"reword", and copy the tags (Reviewed-by, Acked-by, Tested-by, etc) from
the feedback you received to the commit messages. (Unless you changed
the patch significantly.)

Thanks
Laszlo

[Heyi Guo]

Post by Laszlo Ersek

Post by Heyi Guo
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
---
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;

This patch is identical to its counterpart in v2, therefore it should
have been posted with Feng's R-b added, from
http://thread.gmane.org/gmane.comp.bios.tianocore.devel/17244/focus=17309
Ditto for [v2 3/3] <-> [v3 4/4].
Basically, sometime during the preparation of the next version of a
patch series, run a dedicated "git rebase -i", set all the actions to
"reword", and copy the tags (Reviewed-by, Acked-by, Tested-by, etc) from
the feedback you received to the commit messages. (Unless you changed
the patch significantly.)

Nice tips :) Will try next time. Thank you :-)

Heyi

Post by Laszlo Ersek
Thanks
Laszlo

[Heyi Guo]

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;

[Ard Biesheuvel]

Post by Heyi Guo
There were some issues when I ran SCT test against Serial IO Protocol
on qemu aarch64, and below patches were made after I went through the
code of TerminalDxe driver and the flow of console devices being
connected.
V2 is based on TTY terminal patches.
V3 updates comment for PcdDefaultTerminalType.
Please help review these patches and provide your comments.

Hello Heyi,

Thanks for sending another version of this series.

I noticed that you didn't add the Reviewed-by's you collected so far.
It is customary to add those to the patches when they have not changed
(or only in a very minor way) since the previous version. This makes
it easier for us poor reviewers to keep track of what goes on.

So after you split the first patch into two, as requested by Jordan,
could you please make sure you add his Reviewed-by in the way he
requested? I.e,, just to the patch to which it applies? And also round
up the other ones you received over the past week?

Thanks,
Ard.

Post by Heyi Guo
MdePkg|EmulatorPkg: Update comment for PcdDefaultTerminalType
ArmVirtPkg: Make terminal type consistent
MdeModulePkg/TerminalDxe: Set NullRemaining to FALSE by default
MdeModulePkg/TerminalDxe: Some improvements
ArmVirtPkg/ArmVirtQemu.dsc | 8 ++++++
EmulatorPkg/EmulatorPkg.dsc | 2 +-
.../Universal/Console/TerminalDxe/Terminal.c | 29 ++++------------------
MdePkg/MdePkg.dec | 3 ++-
4 files changed, 16 insertions(+), 26 deletions(-)
--
2.1.4

[Heyi Guo]

Thanks for your helpful info.

Post by Ard Biesheuvel

Post by Heyi Guo
There were some issues when I ran SCT test against Serial IO Protocol
on qemu aarch64, and below patches were made after I went through the
code of TerminalDxe driver and the flow of console devices being
connected.
V2 is based on TTY terminal patches.
V3 updates comment for PcdDefaultTerminalType.
Please help review these patches and provide your comments.

Hello Heyi,
Thanks for sending another version of this series.
I noticed that you didn't add the Reviewed-by's you collected so far.
It is customary to add those to the patches when they have not changed
(or only in a very minor way) since the previous version. This makes
it easier for us poor reviewers to keep track of what goes on.
So after you split the first patch into two, as requested by Jordan,
could you please make sure you add his Reviewed-by in the way he
requested? I.e,, just to the patch to which it applies? And also round
up the other ones you received over the past week?
Thanks,
Ard.

Post by Heyi Guo
MdePkg|EmulatorPkg: Update comment for PcdDefaultTerminalType
ArmVirtPkg: Make terminal type consistent
MdeModulePkg/TerminalDxe: Set NullRemaining to FALSE by default
MdeModulePkg/TerminalDxe: Some improvements
ArmVirtPkg/ArmVirtQemu.dsc | 8 ++++++
EmulatorPkg/EmulatorPkg.dsc | 2 +-
.../Universal/Console/TerminalDxe/Terminal.c | 29 ++++------------------
MdePkg/MdePkg.dec | 3 ++-
4 files changed, 16 insertions(+), 26 deletions(-)
--
2.1.4

[Laszlo Ersek]

Post by Heyi Guo
There were some issues when I ran SCT test against Serial IO Protocol
on qemu aarch64, and below patches were made after I went through the
code of TerminalDxe driver and the flow of console devices being
connected.
V2 is based on TTY terminal patches.
V3 updates comment for PcdDefaultTerminalType.
Please help review these patches and provide your comments.
Thanks.
MdePkg|EmulatorPkg: Update comment for PcdDefaultTerminalType
ArmVirtPkg: Make terminal type consistent
MdeModulePkg/TerminalDxe: Set NullRemaining to FALSE by default
MdeModulePkg/TerminalDxe: Some improvements
ArmVirtPkg/ArmVirtQemu.dsc | 8 ++++++
EmulatorPkg/EmulatorPkg.dsc | 2 +-
.../Universal/Console/TerminalDxe/Terminal.c | 29 ++++------------------
MdePkg/MdePkg.dec | 3 ++-
4 files changed, 16 insertions(+), 26 deletions(-)

When you send the next version of a patch series (any series), for each
patch:
- please pick up the Reviewed-by tags that you got for the patch in the
last version, if the patch remains unchanged (or just trivially changed)
- or else, if the patch has been changed significantly (or is new), then
please document it either in the blurb, or use "git notes" to attach
such notes (outside of the commit message) to the patch. (See also
git-format-patch --notes.)

For example, looking at

[PATCH V3 2/4] ArmVirtPkg: Make terminal type consistent

now, I got no clue if you changed that patch relative to v2 (which I
reviewed), and if so, how. Therefore now I must save both versions of
the patch (v2 and v3), compare them, and if I find that v3 is unchanged,
or just trivially changed, I have to send Reviewed-by again. If you pick
up tags for a repost, and point out the changes per patch, that saves a
*lot* of time for reviewers.

Thanks
Laszlo