public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Kubacki, Michael A" <michael.a.kubacki@intel.com>
To: "Chen, Marc W" <marc.w.chen@intel.com>,
	"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Chaganty, Rangasai V" <rangasai.v.chaganty@intel.com>,
	"Gao, Liming" <liming.gao@intel.com>,
	"Zhang, Shenglei" <shenglei.zhang@intel.com>
Subject: Re: [edk2-devel][edk2-platforms][PATCH] IntelSiliconPkg/Feature/SmmAccess/*: Fix incorrect Docygen comment
Date: Wed, 8 Jan 2020 19:49:04 +0000	[thread overview]
Message-ID: <BY5PR11MB448435FC066AFAABF054EB0EB53E0@BY5PR11MB4484.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20191226065308.15572-1-marc.w.chen@intel.com>

edk2-platforms\Silicon\Intel\IntelSiliconPkg\Include\Library\SmmAccessLib.h
* I think the description would be clearer by making the following change:
   "An API to install a PEI_MM_ACCESS_PPI PPI for controlling SMM mode memory access basically for S3 resume usage."
   to
   "An API to install an instance of EFI_PEI_MM_ACCESS_PPI. This PPI is commonly used to control SMM mode memory access for S3 resume."

edk2-platforms\Silicon\Intel\IntelSiliconPkg\Feature\SmmAccess\Library\PeiSmmAccessLib\PeiSmmAccessLib.c
* I suggest aligning the " General purpose services available to every PEIM." on the same vertical column as the other parameter text in the function description.
* Same update to the function description for PeiInstallSmmAccessPei () that was suggested in SmmAccessLib.h

edk2-platforms\Silicon\Intel\IntelSiliconPkg\Feature\SmmAccess\SmmAccessDxe\SmmAccessDriver.h
edk2-platforms\Silicon\Intel\IntelSiliconPkg\Feature\SmmAccess\SmmAccessDxe\SmmAccessDriver.c
* The function description for SmmAccessDriverEntryPoint () is inconsistent in these files.
   * Some return values in the description in SmmAccessDriver.c are missing from the description in SmmAccessDriver.h

With these updates:
Reviewed-by: Michael Kubacki <michael.a.kubacki@intel.com>

Thanks,
Michael

> -----Original Message-----
> From: Chen, Marc W <marc.w.chen@intel.com>
> Sent: Wednesday, December 25, 2019 10:53 PM
> To: devel@edk2.groups.io
> Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Zhang, Shenglei <shenglei.zhang@intel.com>; Chen, Marc W
> <marc.w.chen@intel.com>
> Subject: [edk2-devel][edk2-platforms][PATCH]
> IntelSiliconPkg/Feature/SmmAccess/*: Fix incorrect Docygen comment
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2436
> 
> Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Shenglei Zhang <shenglei.zhang@intel.com>
> Signed-off-by: Marc Chen <marc.w.chen@intel.com>
> ---
>  .../Library/PeiSmmAccessLib/PeiSmmAccessLib.c         | 19 ++++++++----------
> -
>  .../Feature/SmmAccess/SmmAccessDxe/SmmAccessDriver.c  |  6 +++---
> .../Feature/SmmAccess/SmmAccessDxe/SmmAccessDriver.h  | 13 +++++----
> ----
>  .../IntelSiliconPkg/Include/Library/SmmAccessLib.h    |  5 +----
>  4 files changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLi
> b/PeiSmmAccessLib.c
> b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLi
> b/PeiSmmAccessLib.c
> index da141cfa0e..61da7ea0bd 100644
> ---
> a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLi
> b/PeiSmmAccessLib.c
> +++
> b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAcce
> +++ ssLib/PeiSmmAccessLib.c
> @@ -46,9 +46,9 @@ typedef struct {
>    The use of "open" means that the memory is visible from all PEIM
>    and SMM agents.
> 
> +  @param[in] PeiServices      -  General purpose services available to every
> PEIM.
>    @param[in] This             -  Pointer to the SMM Access Interface.
>    @param[in] DescriptorIndex  -  Region of SMRAM to Open.
> -  @param[in] PeiServices      -  General purpose services available to every
> PEIM.
> 
>    @retval EFI_SUCCESS            -  The region was successfully opened.
>    @retval EFI_DEVICE_ERROR       -  The region could not be opened because
> locked by
> @@ -193,12 +193,12 @@ Lock (
>    ranges that are possible for SMRAM access, based upon the
>    memory controller capabilities.
> 
> -  @param[in] PeiServices   - General purpose services available to every
> PEIM.
> -  @param[in] This          -  Pointer to the SMRAM Access Interface.
> -  @param[in] SmramMapSize  -  Pointer to the variable containing size of the
> -                              buffer to contain the description information.
> -  @param[in] SmramMap      -  Buffer containing the data describing the
> Smram
> -                              region descriptors.
> +  @param[in] PeiServices        - General purpose services available to every
> PEIM.
> +  @param[in] This               -  Pointer to the SMRAM Access Interface.
> +  @param[in, out] SmramMapSize  -  Pointer to the variable containing size
> of the
> +                                   buffer to contain the description information.
> +  @param[in, out] SmramMap      -  Buffer containing the data describing the
> Smram
> +                                   region descriptors.
> 
>    @retval EFI_BUFFER_TOO_SMALL  -  The user did not provide a sufficient
> buffer.
>    @retval EFI_SUCCESS           -  The user provided a sufficiently-sized buffer.
> @@ -234,10 +234,7 @@ GetCapabilities (
>  /**
>    This function is to install an SMM Access PPI
>    - <b>Introduction</b> \n
> -    A module to install a PPI for controlling SMM mode memory access
> basically for S3 resume usage.
> -
> -  - @result
> -    Publish _EFI_PEI_MM_ACCESS_PPI.
> +    An API to install a PEI_MM_ACCESS_PPI PPI for controlling SMM mode
> memory access basically for S3 resume usage.
> 
>      @retval EFI_SUCCESS           - Ppi successfully started and installed.
>      @retval EFI_NOT_FOUND         - Ppi can't be found.
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/SmmAccessDxe/SmmAc
> cessDriver.c
> b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/SmmAccessDxe/SmmAc
> cessDriver.c
> index 3d3c4ab206..9409345f6b 100644
> ---
> a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/SmmAccessDxe/SmmAc
> cessDriver.c
> +++
> b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/SmmAccessDxe/SmmAc
> +++ cessDriver.c
> @@ -18,7 +18,7 @@ static SMM_ACCESS_PRIVATE_DATA  mSmmAccess;
>    @param[in] SystemTable     - Pointer to the EFI System Table
> 
>    @retval EFI_SUCCESS           - Protocol was installed successfully
> -  @exception EFI_UNSUPPORTED    - Protocol was not installed
> +  @retval EFI_UNSUPPORTED       - Protocol was not installed
>    @retval EFI_NOT_FOUND         - Protocol can't be found.
>    @retval EFI_OUT_OF_RESOURCES  - Protocol does not have enough
> resources to initialize the driver.
>  **/
> @@ -229,9 +229,9 @@ Lock (
>    memory controller capabilities.
> 
>    @param[in] This                  - Pointer to the SMRAM Access Interface.
> -  @param[in] SmramMapSize          - Pointer to the variable containing size of
> the
> +  @param[in, out] SmramMapSize     - Pointer to the variable containing size
> of the
>                                       buffer to contain the description information.
> -  @param[in] SmramMap              - Buffer containing the data describing the
> Smram
> +  @param[in, out] SmramMap         - Buffer containing the data describing
> the Smram
>                                       region descriptors.
> 
>    @retval EFI_BUFFER_TOO_SMALL  - The user did not provide a sufficient
> buffer.
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/SmmAccessDxe/SmmAc
> cessDriver.h
> b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/SmmAccessDxe/SmmAc
> cessDriver.h
> index c0ff3a250b..a2ea6332fa 100644
> ---
> a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/SmmAccessDxe/SmmAc
> cessDriver.h
> +++
> b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/SmmAccessDxe/SmmAc
> +++ cessDriver.h
> @@ -59,9 +59,6 @@ typedef struct {
>      Please refer the SMM Protocols section in the attached SMM CIS
> Specification version 0.9 for further details.
>      This driver is required if SMM is supported. Proper configuration of SMM
> registers is recommended even if SMM is not supported.
> 
> -  - @result
> -    Publishes the _EFI_SMM_ACCESS_PROTOCOL: Documented in the
> System Management Mode Core Interface Specification, available at the
> URL: http://www.intel.com/technology/framework/spec.htm
> -
>    - <b>Porting Recommendations</b> \n
>      No modification of this module is recommended.  Any modification should
> be done in compliance with the _EFI_SMM_ACCESS_PROTOCOL protocol
> definition.
> 
> @@ -69,7 +66,7 @@ typedef struct {
>    @param[in] SystemTable     - Pointer to the EFI System Table
> 
>    @retval EFI_SUCCESS     - Protocol was installed successfully
> -  @exception EFI_UNSUPPORTED - Protocol was not installed
> +  @retval EFI_UNSUPPORTED - Protocol was not installed
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -142,10 +139,10 @@ Lock (
>    memory controller capabilities.
> 
>    @param[in] This                  - Pointer to the SMRAM Access Interface.
> -  @param[in] SmramMapSize          - Pointer to the variable containing size of
> the
> -                            buffer to contain the description information.
> -  @param[in] SmramMap              - Buffer containing the data describing the
> Smram
> -                            region descriptors.
> +  @param[in, out] SmramMapSize     - Pointer to the variable containing size
> of the
> +                                     buffer to contain the description information.
> +  @param[in, out] SmramMap         - Buffer containing the data describing
> the Smram
> +                                     region descriptors.
> 
>    @retval EFI_BUFFER_TOO_SMALL  - The user did not provide a sufficient
> buffer.
>    @retval EFI_SUCCESS           - The user provided a sufficiently-sized buffer.
> diff --git a/Silicon/Intel/IntelSiliconPkg/Include/Library/SmmAccessLib.h
> b/Silicon/Intel/IntelSiliconPkg/Include/Library/SmmAccessLib.h
> index f658bac68c..0a287b75b2 100644
> --- a/Silicon/Intel/IntelSiliconPkg/Include/Library/SmmAccessLib.h
> +++ b/Silicon/Intel/IntelSiliconPkg/Include/Library/SmmAccessLib.h
> @@ -11,10 +11,7 @@
>  /**
>    This function is to install an SMM Access PPI
>    - <b>Introduction</b> \n
> -    A module to install a PPI for controlling SMM mode memory access
> basically for S3 resume usage.
> -
> -  - @result
> -    Publish _PEI_MM_ACCESS_PPI.
> +    An API to install a PEI_MM_ACCESS_PPI PPI for controlling SMM mode
> memory access basically for S3 resume usage.
> 
>      @retval EFI_SUCCESS           - Ppi successfully started and installed.
>      @retval EFI_NOT_FOUND         - Ppi can't be found.
> --
> 2.16.2.windows.1


      parent reply	other threads:[~2020-01-08 19:49 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-26  6:53 [edk2-devel][edk2-platforms][PATCH] IntelSiliconPkg/Feature/SmmAccess/*: Fix incorrect Docygen comment Marc W Chen
2019-12-31  6:17 ` Zhang, Shenglei
2020-01-03  1:04   ` Liming Gao
2020-01-06  8:33 ` Zhang, Shenglei
2020-01-08 19:49 ` Kubacki, Michael A [this message]

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=BY5PR11MB448435FC066AFAABF054EB0EB53E0@BY5PR11MB4484.namprd11.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