From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Wed, 17 Jul 2019 05:41:19 -0700 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 283E8309178C; Wed, 17 Jul 2019 12:41:19 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-5.ams2.redhat.com [10.36.117.5]) by smtp.corp.redhat.com (Postfix) with ESMTP id 905005C232; Wed, 17 Jul 2019 12:41:16 +0000 (UTC) Subject: Re: [PATCH v1 1/1] Maintainers.txt: Fine-grained review ownership for MdeModulePkg To: Hao A Wu , devel@edk2.groups.io Cc: Andrew Fish , Leif Lindholm , Michael D Kinney , Dandan Bi , Eric Dong , Liming Gao , Ray Ni , Star Zeng , Zhichao Gao References: <20190717014437.19944-1-hao.a.wu@intel.com> <20190717014437.19944-2-hao.a.wu@intel.com> From: "Laszlo Ersek" Message-ID: <665a5165-b859-a3bd-82b7-278e9c01471e@redhat.com> Date: Wed, 17 Jul 2019 14:41:15 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190717014437.19944-2-hao.a.wu@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.41]); Wed, 17 Jul 2019 12:41:19 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 07/17/19 03:44, Hao A Wu wrote: > This commit add the reviewers information for modules within MdeModulePkg. > > For now the modules list includes: > ACPI > BDS > Console and Graphic > Core services (PEI, DXE and Runtime) > Device and Peripheral > Firmware Update > HII and UI > Reset > S3 > SMBIOS > SMM > Variable > > Please note that, for MdeModulePkg components not included in the above > list, the reviewers will fall back to the package maintainers. > > Cc: Andrew Fish > Cc: Laszlo Ersek > Cc: Leif Lindholm > Cc: Michael D Kinney > Cc: Dandan Bi > Cc: Eric Dong > Cc: Liming Gao > Cc: Ray Ni > Cc: Star Zeng > Cc: Zhichao Gao > Signed-off-by: Hao A Wu > --- > Maintainers.txt | 151 +++++++++++++++++++- > 1 file changed, 149 insertions(+), 2 deletions(-) > > diff --git a/Maintainers.txt b/Maintainers.txt > index 18fd2ef43c..c1ae02045e 100644 > --- a/Maintainers.txt > +++ b/Maintainers.txt > @@ -186,11 +186,158 @@ F: MdeModulePkg/ > W: https://github.com/tianocore/tianocore.github.io/wiki/MdeModulePkg > M: Jian J Wang > M: Hao A Wu > + (1) First comment here. This is actually a comment on the patch submission process, not the patch itself. I *suggest* (but do not *require*) that such changes be formatted for submission with at least -U6 (i.e. 6 lines of context, at the minimum). That makes the review much easier. Anyway, to explain the change to myself: basically this cuts the existent MdeModulePkg section in half, keeping Jian and Hao (both "M") assigned at the top-level, and pushing down all other folks ("R"s: Ray and Star) to other subsystems. That's fine. > +MdeModulePkg: ACPI modules > +F: MdeModulePkg/Universal/Acpi/ > +F: MdeModulePkg/Include/*Acpi*.h > +R: Dandan Bi > +R: Liming Gao Basic formatting (F followed by R) is fine. (2) General request (applies to all sections added in this patch): I suggest to sort the "F" patterns lexicographically. That will make future changes to the F patterns more localized and cleaner. > + > +MdeModulePkg: BDS modules > +F: MdeModulePkg/*BootManager*/ > +F: MdeModulePkg/Universal/BdsDxe/ > +F: MdeModulePkg/Universal/LoadFileOnFv2/ > +F: MdeModulePkg/Universal/SecurityStubDxe/Defer3rdPartyImageLoad.* > +F: MdeModulePkg/Universal/DevicePathDxe/ > +F: MdeModulePkg/Universal/DriverHealthManagerDxe/ > +F: MdeModulePkg/Include/Library/UefiBootManagerLib.h > +R: Zhichao Gao > +R: Ray Ni > + > +MdeModulePkg: Console and Graphic modules (3) Suggesting a typo fix: "Graphic*s*" modules. "Graphic modules" suggests they are modules restricted from an under-aged audience. :) (I'm not a native English speaker though.) > +F: MdeModulePkg/*Logo*/ > +F: MdeModulePkg/Library/BaseBmpSupportLib/ > +F: MdeModulePkg/Library/FrameBufferBltLib/ > +F: MdeModulePkg/Universal/Console/ > +F: MdeModulePkg/Include/*Logo*.h > +F: MdeModulePkg/Include/Guid/ConnectConInEvent.h > +F: MdeModulePkg/Include/Guid/Console*.h > +F: MdeModulePkg/Include/Guid/StandardErrorDevice.h > +F: MdeModulePkg/Include/Guid/TtyTerm.h > +F: MdeModulePkg/Include/Library/BmpSupportLib.h > +F: MdeModulePkg/Include/Library/FrameBufferBltLib.h > +R: Zhichao Gao > +R: Ray Ni > + > +MdeModulePkg: Core services (PEI, DXE and Runtime) modules > +F: MdeModulePkg/*Mem*/ > +F: MdeModulePkg/*SectionExtract*/ > +F: MdeModulePkg/*StatusCode*/ > +F: MdeModulePkg/Application/DumpDynPcd/ > +F: MdeModulePkg/Core/Dxe/ > +F: MdeModulePkg/Core/DxeIplPeim/ > +F: MdeModulePkg/Core/Pei/ > +F: MdeModulePkg/Core/RuntimeDxe/ > +F: MdeModulePkg/Library/*Decompress*/ > +F: MdeModulePkg/Library/*Perf*/ > +F: MdeModulePkg/Library/DxeSecurityManagementLib/ > +F: MdeModulePkg/Universal/PCD/ > +F: MdeModulePkg/Universal/PlatformDriOverrideDxe/ > +F: MdeModulePkg/Universal/SecurityStubDxe/SecurityStub.c > +F: MdeModulePkg/Include/*Mem*.h > +F: MdeModulePkg/Include/*Pcd*.h > +F: MdeModulePkg/Include/*Perf*.h > +F: MdeModulePkg/Include/*StatusCode*.h > +F: MdeModulePkg/Include/Guid/Crc32GuidedSectionExtraction.h > +F: MdeModulePkg/Include/Guid/EventExitBootServiceFailed.h > +F: MdeModulePkg/Include/Guid/IdleLoopEvent.h > +F: MdeModulePkg/Include/Guid/LoadModuleAtFixedAddress.h > +F: MdeModulePkg/Include/Guid/LzmaDecompress.h > +F: MdeModulePkg/Include/Library/SecurityManagementLib.h > +R: Dandan Bi > +R: Liming Gao > + > +MdeModulePkg: Device and Peripheral modules > +F: MdeModulePkg/*PciHostBridge*/ > +F: MdeModulePkg/*Serial*/ > +F: MdeModulePkg/Bus/ > +F: MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/ > +F: MdeModulePkg/Universal/Disk/ > +F: MdeModulePkg/Universal/PcatSingleSegmentPciCfg2Pei/ > +F: MdeModulePkg/Include/*Ata*.h > +F: MdeModulePkg/Include/*IoMmu*.h > +F: MdeModulePkg/Include/*NonDiscoverableDevice*.h > +F: MdeModulePkg/Include/*NvmExpress*.h > +F: MdeModulePkg/Include/*SerialPort*.h > +F: MdeModulePkg/Include/*SdMmc*.h > +F: MdeModulePkg/Include/*Ufs*.h > +F: MdeModulePkg/Include/*Usb*.h > +F: MdeModulePkg/Include/Guid/S3StorageDeviceInitList.h > +F: MdeModulePkg/Include/Guid/RecoveryDevice.h > +F: MdeModulePkg/Include/Library/PciHostBridgeLib.h > +F: MdeModulePkg/Include/Ppi/StorageSecurityCommand.h > +F: MdeModulePkg/Include/Protocol/Ps2Policy.h > +R: Hao A Wu > R: Ray Ni > - (especially for Bus, Universal/Console, Universal/Disk, > - Universal/BdsDxe and related libraries, header files) > + > +MdeModulePkg: Firmware Update modules > +F: MdeModulePkg/*Capsule*/ > +F: MdeModulePkg/Library/DisplayUpdateProgressLib*/ > +F: MdeModulePkg/Library/FmpAuthenticationLibNull/ > +F: MdeModulePkg/Universal/Esrt*/ > +F: MdeModulePkg/Include/*Capsule*.h > +F: MdeModulePkg/Include/Library/DisplayUpdateProgressLib.h > +F: MdeModulePkg/Include/Library/FmpAuthenticationLib.h > +F: MdeModulePkg/Include/Protocol/EsrtManagement.h > +F: MdeModulePkg/Include/Protocol/FirmwareManagementProgress.h > +R: Hao A Wu > +R: Liming Gao > + > +MdeModulePkg: HII and UI modules > +F: MdeModulePkg/*FileExplorer*/ > +F: MdeModulePkg/*Hii*/ > +F: MdeModulePkg/*Ui*/ > +F: MdeModulePkg/Application/BootManagerMenuApp/ > +F: MdeModulePkg/Library/CustomizedDisplayLib/ > +F: MdeModulePkg/Universal/DisplayEngineDxe/ > +F: MdeModulePkg/Universal/DriverSampleDxe/ > +F: MdeModulePkg/Universal/SetupBrowserDxe/ > +F: MdeModulePkg/Include/*FileExplorer*.h > +F: MdeModulePkg/Include/*FormBrowser*.h > +F: MdeModulePkg/Include/*Hii*.h > +F: MdeModulePkg/Include/Library/CustomizedDisplayLib.h > +F: MdeModulePkg/Include/Protocol/DisplayProtocol.h > +R: Dandan Bi > +R: Eric Dong > + > +MdeModulePkg: Reset modules > +F: MdeModulePkg/*Reset*/ > +F: MdeModulePkg/Include/*Reset*.h > +R: Zhichao Gao > +R: Ray Ni > + > +MdeModulePkg: S3 modules (4) Can we say "ACPI S3 modules"? (Not insisting, just suggesting.) > +F: MdeModulePkg/*LockBox*/ > +F: MdeModulePkg/Library/*S3*/ > +F: MdeModulePkg/Include/*BootScript*.h > +F: MdeModulePkg/Include/*LockBox*.h > +F: MdeModulePkg/Include/*S3*.h > +R: Hao A Wu > +R: Eric Dong > + > +MdeModulePkg: SMBIOS modules > +F: MdeModulePkg/Universal/Smbios*/ > +R: Dandan Bi > R: Star Zeng > > +MdeModulePkg: SMM modules (5) Can we use the more generic term: Management Mode (MM, SMM) modules ? > +F: MdeModulePkg/*Smi*/ > +F: MdeModulePkg/*Smm*/ > +F: MdeModulePkg/Include/*Smi*.h > +F: MdeModulePkg/Include/*Smm*.h > +R: Eric Dong > +R: Ray Ni > + > +MdeModulePkg: Variable modules (6) Can we say: "UEFI Variable modules"? > +F: MdeModulePkg/*Var*/ > +F: MdeModulePkg/Universal/FaultTolerantWrite*/ > +F: MdeModulePkg/Include/*/*FaultTolerantWrite*.h > +F: MdeModulePkg/Include/*/*Var*.h > +F: MdeModulePkg/Include/Guid/SystemNvDataGuid.h > +F: MdeModulePkg/Include/Protocol/SwapAddressRange.h > +R: Hao A Wu > +R: Liming Gao > + (7) Finally, a high level comment: personally, if I were listed as one of the Reviewers, I'd prefer one patch per section added. But, that preference is up to the Reviewers, of course. I guess the only point I'd really like to see fixed is (2) -- the sorting of "F" patterns. The rest is optional, from my side. Thanks! Laszlo > MdePkg > F: MdePkg/ > W: https://github.com/tianocore/tianocore.github.io/wiki/MdePkg >