Liming,
Pragma once is an alternative to include guards. If we want to
remove the use of the include guard macros that start with '_',
we can consider using pragma once everywhere.
https://en.wikipedia.org/wiki/Pragma_once
If other include files have packed structure requirements,
then they should do it in their own files. No include file
should depend on #pragma pack from another include file.
Mike
> -----Original Message-----
> From: Gao, Liming <liming....@intel.com>
> Sent: Monday, July 27, 2020 7:07 PM
> 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 (#63379): https://edk2.groups.io/g/devel/message/63379
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]
-=-=-=-=-=-=-=-=-=-=-=-
