I am not sure if Robert (Cced) has the interest to be the reviewer of the MdePkg/Include/IndustryStandard directory.
> -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Liming Gao > Sent: Tuesday, July 28, 2020 10:07 AM > To: Leif Lindholm <l...@nuviainc.com>; Javeed, Ashraf > <ashraf.jav...@intel.com>; Laszlo Ersek <ler...@redhat.com>; > Sean Brogan <sean.bro...@microsoft.com> > Cc: devel@edk2.groups.io; Kinney, Michael D <michael.d.kin...@intel.com> > Subject: Re: [edk2-devel] [PATCH V4 1/2] MdePkg/Include/IndustryStandard: CXL > 1.1 Registers > > Leif: > > -----Original Message----- > From: Leif Lindholm <l...@nuviainc.com> > Sent: 2020年7月28日 0:14 > To: Gao, Liming <liming....@intel.com> > Cc: devel@edk2.groups.io; Javeed, Ashraf <ashraf.jav...@intel.com>; Kinney, > Michael D <michael.d.kin...@intel.com> > Subject: Re: [edk2-devel] [PATCH V4 1/2] MdePkg/Include/IndustryStandard: CXL > 1.1 Registers > > On Mon, Jul 27, 2020 at 14:55:03 +0000, Gao, Liming wrote: > > > > diff --git a/MdePkg/Include/IndustryStandard/Cxl11.h > > > > b/MdePkg/Include/IndustryStandard/Cxl11.h > > > > new file mode 100644 > > > > index 0000000000..933c1ab817 > > > > --- /dev/null > > > > +++ b/MdePkg/Include/IndustryStandard/Cxl11.h > > > > @@ -0,0 +1,569 @@ > > > > +/** @file > > > > + CXL 1.1 Register definitions > > > > + > > > > + This file contains the register definitions based on the > > > > + Compute Express Link > > > > + (CXL) Specification Revision 1.1. > > > > + > > > > +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR> > > > > +SPDX-License-Identifier: BSD-2-Clause-Patent > > > > + > > > > +**/ > > > > + > > > > +#ifndef _CXL11_H_ > > > > +#define _CXL11_H_ > > > > > > We should not be adding macros with a leading _ - these are intended > > > for toolchain use. > > > > This style is align to other header file. This is the file header > > macro to make sure the header file be included more than once. > > Yes. The other headers should also be changed, but in the meantime it would > be good to stop adding more incorrect ones. > https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/53_include_files#5-3-5-all-include-file- > contents-must-be-protected-by-a-include-guard > > [Liming] Thank for your point. I miss this one, too. Now, most cases don't > follow this rule. So, there is no good example for > the reference. > I agree the rule to apply the strict check for new adding file. I will check > whether ECC has this check point. > > > > > + > > > > +// > > > > +// CXL Flex Bus Device default device and function number // > > > > +Compute Express Link Specification Revision: 1.1 - Chapter 7.1.1 > > > > +// > > > > +#define CXL_DEV_DEV > > > > 0 > > > > +#define CXL_DEV_FUNC > > > > 0 > > > > + > > > > +// > > > > +// Ensure proper structure formats // #pragma pack(1) > > > > > > And this pragma has no function whatsoever with regards to any of > > > the register definition structs below. It would be much better if > > > the structs requiring packing (_DEVICE, _PORT, ...) were grouped > > > together and only those were given this treatment. > > > > > > #pragma pack(1) is *not* a safe default. > > > > > > > I know pack(1) is for the compact structure layout. > > Yes. And it should be used when structs need to be compacted. > All of the bitfield structs are single-variable structs, so the packing has > no effect on them, other than setting the struct > alignment requirements to 1 byte, which will not be correct (or efficient) on > all architectures. > > [Liming] Yes. There is no effect for bitfield structure. This header file > still includes some structure, such as > CXL_1_1_DVSEC_FLEX_BUS_DEVICE. They may have the compact alignment > requirement. > @Javeed, Ashraf, can you conform it? > > > > Now, one final comment - and this is more of a project feature > > > suggestion: > > > Industry standard headers is something fairly special, even in > > > comparison with the rest of MdePkg. *I* would certainly like to > > > ensure I don't miss changes or additions to them. > > > Could we set up a dedicated group of reviewers for this folder only? > > > This need not affect the actual maintainership of MdePkg, just > > > ensure more eyeballs (or screen readers, braille terminals, ...) hit > > > updates here? > > > > > > i.e. something like the below to Maintainers.txt: > > > > > > F: MdePkg/Include/IndustryStandard/ > > > R: Leif ... > > > R: ... > > > R: ... > > > > > > > This is a good suggestion. IndustryStandard needs more feedback. > > Can you send the patch to update Maintainers.txt? > > Sure, I can do that. Any thoughts on others to add than me? > > [Liming] Thanks. Laszlo or Sean may be added if they are also interested in > IndustryStandard header file. > > Thanks > Liming > / > Leif > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#63381): https://edk2.groups.io/g/devel/message/63381 Mute This Topic: https://groups.io/mt/75823529/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
