Regarding the role assigned to me, Reviewed-by: Ray Ni <ray...@intel.com>
> -----Original Message----- > From: Leif Lindholm <leif.lindh...@linaro.org> > Sent: Wednesday, July 17, 2019 10:37 PM > To: Laszlo Ersek <ler...@redhat.com> > Cc: Wu, Hao A <hao.a...@intel.com>; devel@edk2.groups.io; Andrew Fish > <af...@apple.com>; Kinney, Michael D <michael.d.kin...@intel.com>; Bi, > Dandan <dandan...@intel.com>; Dong, Eric <eric.d...@intel.com>; Gao, > Liming <liming....@intel.com>; Ni, Ray <ray...@intel.com>; Zeng, Star > <star.z...@intel.com>; Gao, Zhichao <zhichao....@intel.com> > Subject: Re: [PATCH v1 1/1] Maintainers.txt: Fine-grained review ownership for > MdeModulePkg > > Top-posting because lazy. > > Basically, I agree with all of Laszlo's comments. > I would be happy for this to remain a single patch, however. Partly because > I'm > lazy. But also because this is effectively "converting" > a single package (MdeModulePkg) into multiple sections. (This is not a strong > preference.) > > I would however prefer a v2 with sorted F:-patterns. > > And of course, Acks/Reviews from everyone affected. > > Best Regards, > > Leif > > On Wed, Jul 17, 2019 at 02:41:15PM +0200, Laszlo Ersek wrote: > > 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 <af...@apple.com> > > > Cc: Laszlo Ersek <ler...@redhat.com> > > > Cc: Leif Lindholm <leif.lindh...@linaro.org> > > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > > Cc: Dandan Bi <dandan...@intel.com> > > > Cc: Eric Dong <eric.d...@intel.com> > > > Cc: Liming Gao <liming....@intel.com> > > > Cc: Ray Ni <ray...@intel.com> > > > Cc: Star Zeng <star.z...@intel.com> > > > Cc: Zhichao Gao <zhichao....@intel.com> > > > Signed-off-by: Hao A Wu <hao.a...@intel.com> > > > --- > > > 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 <jian.j.w...@intel.com> > > > M: Hao A Wu <hao.a...@intel.com> > > > + > > > > (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 <dandan...@intel.com> > > > +R: Liming Gao <liming....@intel.com> > > > > 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 <zhichao....@intel.com> > > > +R: Ray Ni <ray...@intel.com> > > > + > > > +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 <zhichao....@intel.com> > > > +R: Ray Ni <ray...@intel.com> > > > + > > > +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 <dandan...@intel.com> > > > +R: Liming Gao <liming....@intel.com> > > > + > > > +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 <hao.a...@intel.com> > > > R: Ray Ni <ray...@intel.com> > > > - (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 <hao.a...@intel.com> > > > +R: Liming Gao <liming....@intel.com> > > > + > > > +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 <dandan...@intel.com> > > > +R: Eric Dong <eric.d...@intel.com> > > > + > > > +MdeModulePkg: Reset modules > > > +F: MdeModulePkg/*Reset*/ > > > +F: MdeModulePkg/Include/*Reset*.h > > > +R: Zhichao Gao <zhichao....@intel.com> > > > +R: Ray Ni <ray...@intel.com> > > > + > > > +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 <hao.a...@intel.com> > > > +R: Eric Dong <eric.d...@intel.com> > > > + > > > +MdeModulePkg: SMBIOS modules > > > +F: MdeModulePkg/Universal/Smbios*/ > > > +R: Dandan Bi <dandan...@intel.com> > > > R: Star Zeng <star.z...@intel.com> > > > > > > +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 <eric.d...@intel.com> > > > +R: Ray Ni <ray...@intel.com> > > > + > > > +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 <hao.a...@intel.com> > > > +R: Liming Gao <liming....@intel.com> > > > + > > > > (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 > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43889): https://edk2.groups.io/g/devel/message/43889 Mute This Topic: https://groups.io/mt/32498522/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
