Re: [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.

[Laszlo Ersek <lersek@redhat.com>]

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