From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id EE231210F30DA for ; Thu, 16 Aug 2018 05:30:38 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C84A040711F6; Thu, 16 Aug 2018 12:30:37 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-198.rdu2.redhat.com [10.10.120.198]) by smtp.corp.redhat.com (Postfix) with ESMTP id DC6912142F20; Thu, 16 Aug 2018 12:30:36 +0000 (UTC) To: "Dong, Eric" , =?UTF-8?Q?Marvin_H=c3=a4user?= , "edk2-devel@lists.01.org" Cc: "Ni, Ruiyu" References: <20180815021435.13748-1-eric.dong@intel.com> <20180815021435.13748-4-eric.dong@intel.com> <2bc2265d-9fe2-c095-d261-df96efcac391@redhat.com> From: Laszlo Ersek Message-ID: Date: Thu, 16 Aug 2018 14:30:36 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 16 Aug 2018 12:30:37 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 16 Aug 2018 12:30:37 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Aug 2018 12:30:39 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 08/16/18 02:56, Dong, Eric wrote: > 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? If I understood correctly, there were two points to Marvin's argument: - AllocateZeroPages() is the most likely function name that "MemoryAllocationLib.h" would add, *if* it ever introduced a function for "allocating boot service data pages, plus zeroing them". In that case, it would cause a conflict. - Second, because the function is defined in the same translation unit where it is called from (and there are no other callers), we can make the function STATIC. Regarding the first concern, I don't think it's a very practical one. I'm neutral on the question. My point is only that, if we really want to change the name, I think we should do it separately / incrementally. Regarding the second idea, STATIC is a generally good practice, and we should do that everywhere we can. But, because I don't want to re-test / re-review this series after all this effort, I suggest we do the STATIC thing incrementally as well. Thanks Laszlo > > 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 ; edk2-devel@lists.01.org; >> Dong, Eric >> Cc: Ni, Ruiyu >> 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