From: "Oram, Isaac W" <isaac.w.oram@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
"mikuback@linux.microsoft.com" <mikuback@linux.microsoft.com>,
"Desimone, Nathaniel L" <nathaniel.l.desimone@intel.com>
Cc: "Chaganty, Rangasai V" <rangasai.v.chaganty@intel.com>,
Liming Gao <gaoliming@byosoft.com.cn>,
Michael Kubacki <michael.kubacki@microsoft.com>
Subject: Re: [edk2-devel] [edk2-platforms] [PATCH v1 0/9] IpmiFeaturePkg: Add IPMI transport drivers
Date: Thu, 4 Mar 2021 23:33:06 +0000 [thread overview]
Message-ID: <MW3PR11MB4747DB52ED24438924529792D0979@MW3PR11MB4747.namprd11.prod.outlook.com> (raw)
In-Reply-To: <d84304fc-5e0f-55cb-41cd-b4a387bcc951@linux.microsoft.com>
Michael,
Thanks for the feedback.
I was torn between aligning with the proprietary version and cleaning it up.
My concern is if we do too much cleanup, it will delay adoption.
I did the minimum that we know is required for ECC tool to pass coding style tool and avoid EFI prefixes.
I would like delay refactoring and cleaning up until it is in the open where people can easily follow the code evolution from proprietary to open source. I am looking to develop some maintainers for this feature package, and the cleanup would be a good way to ramp them into active open participation and put them on the path to becoming maintainers.
Anyway, your feedback is noted and I will put those on the to do list for completing this.
Regards,
Isaac
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Wednesday, March 3, 2021 11:23 AM
To: devel@edk2.groups.io; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
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: Re: [edk2-devel] [edk2-platforms] [PATCH v1 0/9] IpmiFeaturePkg: Add IPMI transport drivers
Looked over the series at a high-level and the feature structure looks fine. For the series:
Acked-by: Michael Kubacki <michael.kubacki@microsoft.com>
I didn't look closely at implementation but there's a few things I noticed.
There's a few typos. I'd suggest running a spell check to clean those up. Some quick examples:
ServerManagement.h:
"Generic Definations for Server Management Header File."
typedef struct {
LINERIZATION_TYPE Linearization; // L
IpmiTransportPei.h:
"IPMI Ttransport PPI Header File."
There was a mix of function documentation styles though that might be something to clean up later.
I saw some globals that did not appear necessary. For example, mIpmiTransport in SmmIpmiBaseLib.c seems to be located before every access.
Thanks,
Michael
On 3/1/2021 6:27 PM, Nate DeSimone wrote:
> 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/SmmIpmiBaseL
> ib/SmmIpmiBaseLib.inf
>
next prev parent reply other threads:[~2021-03-04 23:33 UTC|newest]
Thread overview: 15+ 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 [this message]
2021-03-05 11:06 ` Nhi Pham
2021-03-11 19:29 ` Chaganty, Rangasai V
[not found] <166865922B67DC3E.27030@groups.io>
2021-04-05 18:45 ` [edk2-devel] " Nate DeSimone
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=MW3PR11MB4747DB52ED24438924529792D0979@MW3PR11MB4747.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