public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Nate DeSimone" <nathaniel.l.desimone@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
	"michael.kubacki@outlook.com" <michael.kubacki@outlook.com>
Cc: "Gao, Liming" <liming.gao@intel.com>,
	"Kinney, Michael D" <michael.d.kinney@intel.com>,
	"Jiang, Guomin" <guomin.jiang@intel.com>,
	"Xu, Wei6" <wei6.xu@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 2/6] FmpDevicePkg: Add LastAttemptStatus.h
Date: Tue, 11 Aug 2020 00:31:31 +0000	[thread overview]
Message-ID: <A66060EB-1847-44F9-B9C7-C24F5CA6DAD8@intel.com> (raw)
In-Reply-To: <MWHPR07MB3440529E82D342E5EF281A18E9440@MWHPR07MB3440.namprd07.prod.outlook.com>

My feedback:

#1: Why is LastAttemptStatus.h in PrivateInclude? Seems like something you would want to have as a public header.

#2: If someone inserts a new enum value in the middle of LAST_ATTEMPT_STATUS_EXPANDED_ERROR_LIST it will make it difficult to decode error codes in the future. Either put a comment that new error code should go on the bottom. Or add some space between each entry using something like this:

enum LAST_ATTEMPT_STATUS_EXPANDED_ERROR_LIST
{
  LAST_ATTEMPT_STATUS_DRIVER_ERROR_GET_FMP_HEADER = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN,
  LAST_ATTEMPT_STATUS_DRIVER_ERROR_PROGRESS_CALLBACK_ERROR = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN + 10,
  LAST_ATTEMPT_STATUS_DRIVER_ERROR_CHECK_POWER_API = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN + 20,
  LAST_ATTEMPT_STATUS_DRIVER_ERROR_CHECK_SYS_THERMAL_API = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN + 30,
  LAST_ATTEMPT_STATUS_DRIVER_ERROR_THERMAL = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN + 40,
  LAST_ATTEMPT_STATUS_DRIVER_ERROR_CHECK_SYS_ENV_API = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN + 50,
  LAST_ATTEMPT_STATUS_DRIVER_ERROR_SYSTEM_ENV = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN + 60,

Then you can insert something in the middle by adding +5.

Thanks,
Nate

On 8/10/20, 1:28 PM, "devel@edk2.groups.io on behalf of Michael Kubacki" <devel@edk2.groups.io on behalf of michael.kubacki@outlook.com> wrote:

    From: Michael Kubacki <michael.kubacki@microsoft.com>

    Introduces a header file to contain Last Attempt Status codes that
    define granular FmpDevicePkg usage of the UEFI Specification
    defined vendor range. The vendor range is described in UEFI
    Specification 2.8A section 23.4.

    With this change, FmpDevicePkg currently defines three subranges of
    the Last Attempt Status vendor range:
      1. Driver - Codes returned from operations performed by the
         FmpDxe driver.
      2. Dependency - Codes returned from FMP dependency related
         functionality (e.g. FmpDependencyLib).
      3. Library - Codes returned from FmpDeviceLib instances.

    Cc: Liming Gao <liming.gao@intel.com>
    Cc: Michael D Kinney <michael.d.kinney@intel.com>
    Cc: Guomin Jiang <guomin.jiang@intel.com>
    Cc: Wei6 Xu <wei6.xu@intel.com>
    Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
    ---
     FmpDevicePkg/PrivateInclude/LastAttemptStatus.h | 81 ++++++++++++++++++++
     1 file changed, 81 insertions(+)

    diff --git a/FmpDevicePkg/PrivateInclude/LastAttemptStatus.h b/FmpDevicePkg/PrivateInclude/LastAttemptStatus.h
    new file mode 100644
    index 000000000000..01e96b23edad
    --- /dev/null
    +++ b/FmpDevicePkg/PrivateInclude/LastAttemptStatus.h
    @@ -0,0 +1,81 @@
    +/** @file
    +  Defines last attempt status codes used in FmpDevicePkg.
    +
    +  Copyright (c) Microsoft Corporation.<BR>
    +
    +  SPDX-License-Identifier: BSD-2-Clause-Patent
    +
    +**/
    +
    +#ifndef __FMP_LAST_ATTEMPT_STATUS_H__
    +#define __FMP_LAST_ATTEMPT_STATUS_H__
    +
    +//
    +// Size of the error code range for FMP driver-specific errors
    +//
    +#define LAST_ATTEMPT_STATUS_DRIVER_ERROR_COUNT              0x80
    +
    +//
    +// Size of the error code range for FMP dependency related errors
    +//
    +#define LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_COUNT          0x20
    +
    +#define LAST_ATTEMPT_STATUS_DRIVER_MAX_ERROR_CODE_VALUE     LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN + \
    +                                                            LAST_ATTEMPT_STATUS_DRIVER_ERROR_COUNT - 1
    +
    +#define LAST_ATTEMPT_STATUS_DEPENDENCY_MAX_ERROR_CODE_VALUE LAST_ATTEMPT_STATUS_DRIVER_MAX_ERROR_CODE_VALUE + \
    +                                                            LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_COUNT
    +
    +#define LAST_ATTEMPT_STATUS_LIBRARY_MAX_ERROR_CODE_VALUE    LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MAX - 1
    +
    +//
    +// Last attempt status codes defined for additional granularity in the FMP stack.
    +//
    +// These codes are defined within the higher-level UEFI specification defined UNSUCCESSFUL_VENDOR_RANGE.
    +//
    +// The following last attempt status code ranges are defined for the following corresponding component:
    +//   * LAST_ATTEMPT_STATUS_DRIVER - FMP driver
    +//   * LAST_ATTEMPT_STATUS_DEPENDENCY - FMP dependency functionality
    +//   * LAST_ATTEMPT_STATUS_LIBRARY - FMP device library instances
    +//
    +enum LAST_ATTEMPT_STATUS_EXPANDED_ERROR_LIST
    +{
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_GET_FMP_HEADER                 = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_PROGRESS_CALLBACK_ERROR        ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_CHECK_POWER_API                ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_CHECK_SYS_THERMAL_API          ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_THERMAL                        ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_CHECK_SYS_ENV_API              ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_SYSTEM_ENV                     ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_GET_FMP_HEADER_SIZE            ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_GET_ALL_HEADER_SIZE            ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_GET_FMP_HEADER_VERSION         ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_IMAGE_NOT_PROVIDED             ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_IMAGE_NOT_UPDATABLE            ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_INVALID_CERTIFICATE            ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_INVALID_IMAGE_INDEX            ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_INVALID_KEY_LENGTH             ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_INVALID_KEY_LENGTH_VALUE       ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_VERSION_TOO_LOW                ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_DEVICE_LOCKED                  ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_IMAGE_AUTH_FAILURE             ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_PROTOCOL_ARG_MISSING           ,
    +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_MAX_ERROR_CODE                 = LAST_ATTEMPT_STATUS_DRIVER_MAX_ERROR_CODE_VALUE,
    +
    +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_GET_DEPEX_FAILURE          ,
    +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_NO_END_OPCODE              ,
    +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_UNKNOWN_OPCODE             ,
    +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_MEMORY_ALLOCATION_FAILED   ,
    +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_GUID_BEYOND_DEPEX          ,
    +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_VERSION_BEYOND_DEPEX       ,
    +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_VERSION_STR_BEYOND_DEPEX   ,
    +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_FMP_NOT_FOUND              ,
    +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_PUSH_FAILURE               ,
    +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_POP_FAILURE                ,
    +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_MAX_ERROR_CODE             = LAST_ATTEMPT_STATUS_DEPENDENCY_MAX_ERROR_CODE_VALUE,
    +
    +  LAST_ATTEMPT_STATUS_LIBRARY_ERROR_MIN_ERROR_CODE                ,
    +  LAST_ATTEMPT_STATUS_LIBRARY_ERROR_MAX_ERROR_CODE                = LAST_ATTEMPT_STATUS_LIBRARY_MAX_ERROR_CODE_VALUE
    +};
    +
    +#endif
    -- 
    2.28.0.windows.1


    



  reply	other threads:[~2020-08-11  0:31 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20200810202753.1318-1-michael.kubacki@outlook.com>
2020-08-10 20:27 ` [PATCH v2 1/6] MdePkg/SystemResourceTable.h: Add vendor range values Michael Kubacki
2020-08-10 20:27 ` [PATCH v2 2/6] FmpDevicePkg: Add LastAttemptStatus.h Michael Kubacki
2020-08-11  0:31   ` Nate DeSimone [this message]
2020-08-11 17:46     ` [edk2-devel] " Michael Kubacki
2020-08-11 18:57       ` Michael Kubacki
2020-08-21  5:33         ` Nate DeSimone
2020-08-21 21:23           ` Michael Kubacki
2020-08-24 17:22             ` Nate DeSimone
2020-08-25  0:30               ` Michael Kubacki
     [not found]               ` <957bfe53-d369-2f06-441f-c5c1bb74aeb2@outlook.com>
2020-08-28 17:46                 ` Michael Kubacki
2020-08-10 20:27 ` [PATCH v2 3/6] FmpDevicePkg/FmpDxe: Add check image path Last Attempt Status capability Michael Kubacki
2020-08-10 20:27 ` [PATCH v2 4/6] FmpDevicePkg/FmpDxe: Improve set image path Last Attempt Status granularity Michael Kubacki
2020-08-10 20:27 ` [PATCH v2 5/6] FmpDevicePkg: Add Last Attempt Status support to dependency libs Michael Kubacki
2020-08-10 20:27 ` [PATCH v2 6/6] FmpDevicePkg/FmpDeviceLib: Add Last Attempt Status to Check/Set API Michael Kubacki

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=A66060EB-1847-44F9-B9C7-C24F5CA6DAD8@intel.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