From: "Chaganty, Rangasai V" <rangasai.v.chaganty@intel.com>
To: "Desimone, Nathaniel L" <nathaniel.l.desimone@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Oram, Isaac W" <isaac.w.oram@intel.com>,
Liming Gao <gaoliming@byosoft.com.cn>,
Michael Kubacki <michael.kubacki@microsoft.com>
Subject: Re: [edk2-platforms] [PATCH v1 0/9] IpmiFeaturePkg: Add IPMI transport drivers
Date: Thu, 11 Mar 2021 19:29:02 +0000 [thread overview]
Message-ID: <DM6PR11MB44768A2E3573853134075F95B6909@DM6PR11MB4476.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20210302022804.8641-1-nathaniel.l.desimone@intel.com>
Overall structure looks good. For the series:
Reviewed-by: Sai Chaganty <rangasai.v.chaganty@intel.com>
There are few, rather general comments which I think can be queued up as on-going improvement after the initial check-in.
1. Copyright year for newly added files should begin with the current year // this is probably should be taken care before the first checkin
2. Ensure new GUIDs are created for newly added modules and interfaces, to avoid any potential conflicts.
3. Null instance libraries (e.g. IpmiBaseLibNull.inf) has classes defined that are not necessarily in use (e.g DebugLib). While it may not be a functional failure, it's a good practice to declare only those classes that are being used in the implementation.
4. ReadMe needs to be filled out completely.
5. Function description are found to be placed after the function (e.g. UpdateErrorStatus(), IpmiSendCommandToBmc() in IpmiBmc.c). The description comment block should precede the function.
6. Found instances where return status is marked as "Status" in description. Suggest to add possible return values in the function description
7. Found cases where we have two APIs with different names but with similar implementation - e.g. IpmiSendCommandToBmc() and PeiIpmiSendCommandToBmc(). Can these be converged?
8. Please check and address relative path issue in . inf (e.g. GenericIpmi.inf)
9. In IpmiInit.c, Please remove the following, if not applicable
#ifdef FAST_VIDEO_SUPPORT
#include <Protocol/VideoPrint.h>
#endif
10. Consider adding .fdf definition under \Include that lists the IPMI modules to be dispatched. This will simplify the integration of this feature on a platform.
11. Need to make sure package build is working.
Regards,
Sai
-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Sent: Monday, March 01, 2021 6:28 PM
To: devel@edk2.groups.io
Cc: Oram, Isaac W <isaac.w.oram@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Michael Kubacki <michael.kubacki@microsoft.com>
Subject: [edk2-platforms] [PATCH v1 0/9] IpmiFeaturePkg: Add IPMI transport drivers
From: Isaac Oram <isaac.w.oram@intel.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3242
Implement an open source generic IPMI transport driver.
Provides the ability to communicate with a BMC over IPMI in MinPlatform board packages.
New changes:
1. Added GenericIpmi
2. Added IPMI base libs
3. Added transport PPI and protocol
4. Updated IPMI command request and response data size from
UINT8 to UINT32 in IPMI transport layer to be compatible
with EDK2 industry standard IPMI commands.
6. Added the completion code in the first byte of all IPMI
response data to be compatible with EDK2 industry standard
IPMI commands.
Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael Kubacki <michael.kubacki@microsoft.com>
Signed-off-by: Isaac Oram <isaac.w.oram@intel.com>
Co-authored-by: Nate DeSimone <nathaniel.l.desimone@intel.com>
Isaac Oram (9):
IpmiFeaturePkg: Add IPMI driver Include headers
IpmiFeaturePkg: Add IpmiBaseLib and IpmiCommandLib
IpmiFeaturePkg: Add IpmiInit drivers
IpmiFeaturePkg: Add GenericIpmi driver common code
IpmiFeaturePkg: Add GenericIpmi PEIM
IpmiFeaturePkg: Add GenericIpmi DXE Driver
IpmiFeaturePkg: Add GenericIpmi SMM Driver
IpmiFeaturePkg: Add IPMI driver build files
Maintainers.txt: Add IpmiFeaturePkg maintainers
.../GenericIpmi/Common/IpmiBmc.c | 297 +++++++++++
.../GenericIpmi/Common/IpmiBmc.h | 44 ++
.../GenericIpmi/Common/IpmiBmcCommon.h | 179 +++++++
.../GenericIpmi/Common/IpmiHooks.c | 96 ++++
.../GenericIpmi/Common/IpmiHooks.h | 81 +++
.../GenericIpmi/Common/IpmiPhysicalLayer.h | 29 ++
.../GenericIpmi/Common/KcsBmc.c | 483 ++++++++++++++++++
.../GenericIpmi/Common/KcsBmc.h | 236 +++++++++
.../GenericIpmi/Dxe/GenericIpmi.c | 46 ++
.../GenericIpmi/Dxe/GenericIpmi.inf | 66 +++
.../IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c | 452 ++++++++++++++++
.../GenericIpmi/Pei/PeiGenericIpmi.c | 362 +++++++++++++
.../GenericIpmi/Pei/PeiGenericIpmi.h | 138 +++++
.../GenericIpmi/Pei/PeiGenericIpmi.inf | 58 +++
.../GenericIpmi/Pei/PeiIpmiBmc.c | 277 ++++++++++
.../GenericIpmi/Pei/PeiIpmiBmc.h | 38 ++
.../GenericIpmi/Pei/PeiIpmiBmcDef.h | 156 ++++++
.../GenericIpmi/Smm/SmmGenericIpmi.c | 208 ++++++++
.../GenericIpmi/Smm/SmmGenericIpmi.inf | 53 ++
.../IpmiFeaturePkg/Include/IpmiFeature.dsc | 9 +-
.../Include/Library/IpmiBaseLib.h | 50 ++
.../Include/Library/IpmiCommandLib.h | 19 +-
.../Include/Ppi/IpmiTransportPpi.h | 68 +++
.../Include/Protocol/IpmiTransportProtocol.h | 75 +++ .../IpmiFeaturePkg/Include/ServerManagement.h | 337 ++++++++++++
.../IpmiFeaturePkg/Include/SmStatusCodes.h | 98 ++++
.../IpmiFeaturePkg/IpmiFeaturePkg.dec | 22 +-
.../IpmiFeaturePkg/IpmiInit/DxeIpmiInit.c | 4 +-
.../IpmiFeaturePkg/IpmiInit/DxeIpmiInit.inf | 6 +-
.../IpmiFeaturePkg/IpmiInit/PeiIpmiInit.c | 4 +-
.../IpmiFeaturePkg/IpmiInit/PeiIpmiInit.inf | 4 +-
.../Library/IpmiBaseLib/IpmiBaseLib.c | 155 ++++++
.../Library/IpmiBaseLib/IpmiBaseLib.inf | 28 +
.../Library/IpmiBaseLibNull/IpmiBaseLibNull.c | 76 +++
.../IpmiBaseLibNull/IpmiBaseLibNull.inf | 36 ++
.../Library/IpmiCommandLib/IpmiCommandLib.inf | 4 +-
.../IpmiCommandLib/IpmiCommandLibNetFnApp.c | 7 +-
.../IpmiCommandLibNetFnChassis.c | 51 +-
.../IpmiCommandLibNetFnStorage.c | 7 +-
.../IpmiCommandLibNetFnTransport.c | 7 +-
.../Library/PeiIpmiBaseLib/PeiIpmiBaseLib.c | 111 ++++
.../Library/PeiIpmiBaseLib/PeiIpmiBaseLib.inf | 30 ++
.../Library/SmmIpmiBaseLib/SmmIpmiBaseLib.c | 180 +++++++
.../Library/SmmIpmiBaseLib/SmmIpmiBaseLib.inf | 29 ++
.../IpmiFeaturePkg/Readme.md | 4 +-
Maintainers.txt | 6 +
46 files changed, 4694 insertions(+), 32 deletions(-) create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmc.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiBmcCommon.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiHooks.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/IpmiPhysicalLayer.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/KcsBmc.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Common/KcsBmc.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/GenericIpmi.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/GenericIpmi.inf
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Dxe/IpmiInit.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiGenericIpmi.inf
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmc.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmc.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Pei/PeiIpmiBmcDef.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Smm/SmmGenericIpmi.inf
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/Library/IpmiBaseLib.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/Ppi/IpmiTransportPpi.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/Protocol/IpmiTransportProtocol.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/ServerManagement.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Include/SmStatusCodes.h
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiBaseLib/IpmiBaseLib.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiBaseLib/IpmiBaseLib.inf
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiBaseLibNull/IpmiBaseLibNull.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/IpmiBaseLibNull/IpmiBaseLibNull.inf
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/PeiIpmiBaseLib/PeiIpmiBaseLib.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/PeiIpmiBaseLib/PeiIpmiBaseLib.inf
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/SmmIpmiBaseLib/SmmIpmiBaseLib.c
create mode 100644 Features/Intel/OutOfBandManagement/IpmiFeaturePkg/Library/SmmIpmiBaseLib/SmmIpmiBaseLib.inf
--
2.27.0.windows.1
prev parent reply other threads:[~2021-03-11 19:29 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-02 2:27 [edk2-platforms] [PATCH v1 0/9] IpmiFeaturePkg: Add IPMI transport drivers Nate DeSimone
2021-03-02 2:27 ` [edk2-platforms] [PATCH v1 1/9] IpmiFeaturePkg: Add IPMI driver Include headers Nate DeSimone
2021-03-02 2:27 ` [edk2-platforms] [PATCH v1 2/9] IpmiFeaturePkg: Add IpmiBaseLib and IpmiCommandLib Nate DeSimone
2021-03-02 2:27 ` [edk2-platforms] [PATCH v1 3/9] IpmiFeaturePkg: Add IpmiInit driver DEPEXs Nate DeSimone
2021-03-02 2:27 ` [edk2-platforms] [PATCH v1 4/9] IpmiFeaturePkg: Add GenericIpmi driver common code Nate DeSimone
2021-03-02 2:28 ` [edk2-platforms] [PATCH v1 5/9] IpmiFeaturePkg: Add GenericIpmi PEIM Nate DeSimone
2021-03-02 2:28 ` [edk2-platforms] [PATCH v1 6/9] IpmiFeaturePkg: Add GenericIpmi DXE Driver Nate DeSimone
2021-03-02 2:28 ` [edk2-platforms] [PATCH v1 7/9] IpmiFeaturePkg: Add GenericIpmi SMM Driver Nate DeSimone
2021-03-02 2:28 ` [edk2-platforms] [PATCH v1 8/9] IpmiFeaturePkg: Add IPMI driver build files Nate DeSimone
2021-03-02 2:28 ` [edk2-platforms] [PATCH v1 9/9] Maintainers.txt: Add IpmiFeaturePkg maintainers Nate DeSimone
2021-03-03 19:22 ` [edk2-devel] [edk2-platforms] [PATCH v1 0/9] IpmiFeaturePkg: Add IPMI transport drivers Michael Kubacki
2021-03-04 23:33 ` Oram, Isaac W
2021-03-05 11:06 ` Nhi Pham
2021-03-11 19:29 ` Chaganty, Rangasai V [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=DM6PR11MB44768A2E3573853134075F95B6909@DM6PR11MB4476.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