public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Dong, Eric" <eric.dong@intel.com>
To: "Laszlo Ersek" <lersek@redhat.com>,
	"Marvin Häuser" <Marvin.Haeuser@outlook.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Ni, Ruiyu" <ruiyu.ni@intel.com>
Subject: Re: [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.
Date: Thu, 16 Aug 2018 00:56:32 +0000	[thread overview]
Message-ID: <ED077930C258884BBCB450DB737E66224AC90B64@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <2bc2265d-9fe2-c095-d261-df96efcac391@redhat.com>

Hi Marvin & Laszlo,

I'm not very clear about the risk to use this function name. I think it is just used in a driver as an internal function, other drivers or libraries can't use it. I think we don't need to add internal prefix to all internal functions in drivers, maybe need for the libraries, right?  Here we need to add internal prefix just because it has similar name with other common API?

Thanks,
Eric

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Wednesday, August 15, 2018 11:30 PM
> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-devel@lists.01.org;
> Dong, Eric <eric.dong@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change
> Memory Type and address limitation.
> 
> On 08/15/18 15:12, Marvin Häuser wrote:
> > Hey Eric and anyone CC'd,
> >
> > Are you sure you want to name the function "AllocateZeroPages"? It's
> > analogous to "AllocateZeroPool", so I could see it becoming an API
> > function at some point, which will conflict with this definition and
> > might silently break UefiCpuPkg compilation if not tested before
> > upstreaming. I usually make any module's private functions static and
> > prefix "Internal" if possible, or, if static cannot be used,
> > non-static plus prefix something derived from the module's name to
> > achieve uniqueness. If I am not mistaken, this could be made static,
> > couldn't it?
> 
> I agree that the function's name is not optimal, primarily because the
> Allocate*Pages() functions tend to take a page count, not a byte count.
> However, I didn't want to ask for another version just because of this; a lot of
> review (and now testing) has gone into this set, and this is just a wart.
> 
> I suggest that -- after the stable tag -- we push v4 as-is; however, Marvin,
> please go ahead and file a TianoCore BZ that depends on 959 (i.e. on the BZ
> currently referenced in patch #5), about fixing up the function name (and
> about making it static).
> 
> Note that an "AllocateZeroPages" function exists in
> "IntelSiliconPkg/Feature/VTd/IntelVTdDxe/TranslationTable.c" as well. I guess
> both functions should be renamed (and likely not to the same new name,
> because they have different parameter lists). And, only the UefiCpuPkg one
> can be made static however. Either way, both packages could be covered by
> the same BZ.


> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

  reply	other threads:[~2018-08-16  0:57 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-15  2:14 [Patch v4 0/5] Change CpuS3Data memory type and address limitation Eric Dong
2018-08-15  2:14 ` [Patch v4 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use GDT/IDT saved in Smram Eric Dong
2018-08-15  5:40   ` Ni, Ruiyu
2018-08-15 13:03   ` Laszlo Ersek
2018-08-15  2:14 ` [Patch v4 2/5] UefiCpuPkg/AcpiCpuData.h: Remove AcpiNVS and Below 4G limitation Eric Dong
2018-08-15  2:14 ` [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation Eric Dong
2018-08-15  5:40   ` Ni, Ruiyu
2018-08-15 13:11   ` Laszlo Ersek
2018-08-15 13:12   ` Marvin Häuser
2018-08-15 15:30     ` Laszlo Ersek
2018-08-16  0:56       ` Dong, Eric [this message]
2018-08-16 12:30         ` Laszlo Ersek
2018-08-16 12:59           ` Marvin Häuser
2018-08-17  1:51             ` Dong, Eric
2018-08-15  2:14 ` [Patch v4 4/5] UefiCpuPkg/CpuS3DataDxe: Remove below 4G limitation Eric Dong
2018-08-15  2:14 ` [Patch v4 5/5] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation Eric Dong
2018-08-15 13:14 ` [Patch v4 0/5] Change CpuS3Data memory type and address limitation Laszlo Ersek
2018-08-15 14:00   ` Laszlo Ersek

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=ED077930C258884BBCB450DB737E66224AC90B64@shsmsx102.ccr.corp.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