[Patch 0/2] Add comments for new Pcds

[Patch 0/2] Add comments for new Pcds

[Eric Dong]

Add missed Pcds definition and comments in dec and inf files.

Eric Dong (2):
  UefiCpuPkg: Add comments for PCDs definition.
  UefiCpuPkg UefiCpupkg.uni: Add new pcds comments.

 UefiCpuPkg/UefiCpuPkg.dec | 22 ++++++++++++++++++++++
 UefiCpuPkg/UefiCpuPkg.uni |  8 ++++++++
 2 files changed, 30 insertions(+)

-- 
2.10.1.windows.1

[Patch 1/2] UefiCpuPkg: Add comments for PCDs definition.

[Eric Dong]

Cc: Liming Gao <liming.gao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
 UefiCpuPkg/UefiCpuPkg.dec | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index 2ddeab4..1f3a992 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -287,10 +287,32 @@
 
   ## Contains the size of memory required when CPU processor trace is enabled.
   # @Prompt The memory size used for processor trace.
+  #  0x0:  4K,
+  #  0x1:  8K,
+  #  0x2:  16K,
+  #  0x3:  32K,
+  #  0x4:  64K,
+  #  0x5:  128K,
+  #  0x6:  256K,
+  #  0x7:  512K,
+  #  0x8:  1M,
+  #  0x9:  2M,
+  #  0xA:  4M,
+  #  0xB:  8M,
+  #  0xC:  16M,
+  #  0xD:  32M,
+  #  0xE:  64M,
+  #  0xF:  128M,
+  #  0x10: ProcTraceMemDisable
+  # @ValidRange  0x80000001 | 0 - 0x10
   gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize|0x10|UINT32|0x60000012
 
   ## Contains the processor trace output scheme when CPU processor trace is enabled.
   # @Prompt The processor trace output scheme.
+  #  0: Single Range output scheme.<BR>
+  #  1: ToPA(Table of physical address) scheme.<BR>
+  #  2: Invalid scheme.<BR>
+  # @ValidRange  0x80000001 | 0 - 2
   gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme|0x2|UINT8|0x60000015
 
 [UserExtensions.TianoCore."ExtraFiles"]
-- 
2.10.1.windows.1

Re: [Patch 1/2] UefiCpuPkg: Add comments for PCDs definition.

[Kinney, Michael D]

Eric,

Please add the default value settings to the detailed description.

For example, PcdCpuProcTraceMemSize should say that the default
value is 0x10 which disables the processor memory trace.

Also, the .uni file _HELP should contains the same information
as the detailed description, including the set of valid values
and their meanings.  Please see
gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel in 
MdePkg/MdePkg.uni for an example to follow.

Thanks,

Mike

> -----Original Message-----
> From: Dong, Eric
> Sent: Tuesday, August 8, 2017 11:32 PM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: [Patch 1/2] UefiCpuPkg: Add comments for PCDs
> definition.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/UefiCpuPkg.dec | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec
> b/UefiCpuPkg/UefiCpuPkg.dec
> index 2ddeab4..1f3a992 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -287,10 +287,32 @@
> 
>    ## Contains the size of memory required when CPU processor
> trace is enabled.
>    # @Prompt The memory size used for processor trace.
> +  #  0x0:  4K,
> +  #  0x1:  8K,
> +  #  0x2:  16K,
> +  #  0x3:  32K,
> +  #  0x4:  64K,
> +  #  0x5:  128K,
> +  #  0x6:  256K,
> +  #  0x7:  512K,
> +  #  0x8:  1M,
> +  #  0x9:  2M,
> +  #  0xA:  4M,
> +  #  0xB:  8M,
> +  #  0xC:  16M,
> +  #  0xD:  32M,
> +  #  0xE:  64M,
> +  #  0xF:  128M,
> +  #  0x10: ProcTraceMemDisable
> +  # @ValidRange  0x80000001 | 0 - 0x10
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize|0x10|UINT32|0
> x60000012
> 
>    ## Contains the processor trace output scheme when CPU
> processor trace is enabled.
>    # @Prompt The processor trace output scheme.
> +  #  0: Single Range output scheme.<BR>
> +  #  1: ToPA(Table of physical address) scheme.<BR>
> +  #  2: Invalid scheme.<BR>
> +  # @ValidRange  0x80000001 | 0 - 2
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme|0x2|UINT
> 8|0x60000015
> 
>  [UserExtensions.TianoCore."ExtraFiles"]
> --
> 2.10.1.windows.1

[Patch 2/2] UefiCpuPkg UefiCpupkg.uni: Add new pcds comments.

[Eric Dong]

Cc: Liming Gao <liming.gao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
 UefiCpuPkg/UefiCpuPkg.uni | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
index cd0ecab..dd62fc0 100644
--- a/UefiCpuPkg/UefiCpuPkg.uni
+++ b/UefiCpuPkg/UefiCpuPkg.uni
@@ -194,3 +194,11 @@
 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuFeaturesSetting_PROMPT  #language en-US "Actual processor feature settings."
 
 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuFeaturesSetting_HELP  #language en-US "Specifies actual settings for processor features, each bit corresponding to a specific feature."
+
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuProcTraceMemSize_PROMPT  #language en-US "Memory size used by Processor Trace feature."
+
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuProcTraceMemSize_HELP  #language en-US "User input the memory size can be used by processor trace feature."
+
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuProcTraceOutputScheme_PROMPT  #language en-US "Processor Trace output scheme type."
+
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuProcTraceOutputScheme_HELP  #language en-US "User input the processor trace output scheme type."
\ No newline at end of file
-- 
2.10.1.windows.1

Re: [Patch 0/2] Add comments for new Pcds

[Laszlo Ersek]

Eric,

On 08/09/17 08:32, Eric Dong wrote:
> Add missed Pcds definition and comments in dec and inf files.
> 
> Eric Dong (2):
>   UefiCpuPkg: Add comments for PCDs definition.
>   UefiCpuPkg UefiCpupkg.uni: Add new pcds comments.
> 
>  UefiCpuPkg/UefiCpuPkg.dec | 22 ++++++++++++++++++++++
>  UefiCpuPkg/UefiCpuPkg.uni |  8 ++++++++
>  2 files changed, 30 insertions(+)
> 

can you please write actual commit messages for your patches?

In this series, neither patch has a commit message body. Minimally
PcdCpuProcTraceMemSize and PcdCpuProcTraceOutputScheme should be named
in both commit messages.

I've noticed this as a tendency, and I feel it's now time for me to
speak up. Consider the following earlier patch sets:

* [edk2] [Patch v4 0/3] Enable Processor Trace feature
  https://lists.01.org/pipermail/edk2-devel/2017-August/012902.html

No commit message body on either of the three patches.

* [edk2] [Patch 0/3] Enable LMCE feature
  https://lists.01.org/pipermail/edk2-devel/2017-August/012731.html

No commit message body on either of the three patches.

Especially in the LMCE case, I tried to figure out how things fit
together, but the commit messages gave zero information.

IMO, without a commit message body, a patch cannot be considered
complete (unless the patch is trivial). I feel that reviewers shouldn't
accept empty commit messages either.

Commit messages are not there (just) for the author, but for everyone
else (too). Just because a patch is for UefiCpuPkg, someone else might
stumble upon it when bisecting a regression, and they will want to read
the justification. Commit messages also educate other project participants.

It's not necessary to repeat the same old background in every single
commit message, yes. But please provide pointers then, to
specifications, text files, earlier commits (that do have detailed
messages), and so on.

Yes, writing commit messages takes time; sometimes even *more* time than
writing the actual code. That's fine, it's part of open source
development. Please invest the time.

Thank you,
Laszlo

Re: [Patch 0/2] Add comments for new Pcds

[Dong, Eric]

Laszlo,

Thanks for your advice. I will pay more attention to the check in log later. 

Thanks,
Eric

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, August 9, 2017 7:15 PM
To: Dong, Eric <eric.dong@intel.com>
Cc: edk2-devel@lists.01.org; Justen, Jordan L <jordan.l.justen@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Fan, Jeff <jeff.fan@intel.com>
Subject: Re: [edk2] [Patch 0/2] Add comments for new Pcds

Eric,

On 08/09/17 08:32, Eric Dong wrote:
> Add missed Pcds definition and comments in dec and inf files.
> 
> Eric Dong (2):
>   UefiCpuPkg: Add comments for PCDs definition.
>   UefiCpuPkg UefiCpupkg.uni: Add new pcds comments.
> 
>  UefiCpuPkg/UefiCpuPkg.dec | 22 ++++++++++++++++++++++  
> UefiCpuPkg/UefiCpuPkg.uni |  8 ++++++++
>  2 files changed, 30 insertions(+)
> 

can you please write actual commit messages for your patches?

In this series, neither patch has a commit message body. Minimally PcdCpuProcTraceMemSize and PcdCpuProcTraceOutputScheme should be named in both commit messages.

I've noticed this as a tendency, and I feel it's now time for me to speak up. Consider the following earlier patch sets:

* [edk2] [Patch v4 0/3] Enable Processor Trace feature
  https://lists.01.org/pipermail/edk2-devel/2017-August/012902.html

No commit message body on either of the three patches.

* [edk2] [Patch 0/3] Enable LMCE feature
  https://lists.01.org/pipermail/edk2-devel/2017-August/012731.html

No commit message body on either of the three patches.

Especially in the LMCE case, I tried to figure out how things fit together, but the commit messages gave zero information.

IMO, without a commit message body, a patch cannot be considered complete (unless the patch is trivial). I feel that reviewers shouldn't accept empty commit messages either.

Commit messages are not there (just) for the author, but for everyone else (too). Just because a patch is for UefiCpuPkg, someone else might stumble upon it when bisecting a regression, and they will want to read the justification. Commit messages also educate other project participants.

It's not necessary to repeat the same old background in every single commit message, yes. But please provide pointers then, to specifications, text files, earlier commits (that do have detailed messages), and so on.

Yes, writing commit messages takes time; sometimes even *more* time than writing the actual code. That's fine, it's part of open source development. Please invest the time.

Thank you,
Laszlo