Re: [edk2-devel] [PATCH v1 1/1] Maintainers.txt: Fine-grained review ownership for MdeModulePkg

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Wednesday, July 17, 2019 8:41 PM
> To: Wu, Hao A; devel@edk2.groups.io
> Cc: Andrew Fish; Leif Lindholm; Kinney, Michael D; Bi, Dandan; Dong, Eric;
> Gao, Liming; Ni, Ray; Zeng, Star; Gao, Zhichao
> Subject: Re: [edk2-devel] [PATCH v1 1/1] Maintainers.txt: Fine-grained
> review ownership for MdeModulePkg
> 
> 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 <afish@apple.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Dandan Bi <dandan.bi@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Zhichao Gao <zhichao.gao@intel.com>
> > Signed-off-by: Hao A Wu <hao.a.wu@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.wang@intel.com>
> >  M: Hao A Wu <hao.a.wu@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.


Agree. I will address this in the next patch series.


> 
> 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.bi@intel.com>
> > +R: Liming Gao <liming.gao@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.


I will handle this.

For the proposed patch, I deliberately split the 'F:' list into 2 parts,
folders and header files. Each part are sorted alphabetically. In my
opinion, it makes the list a little bit cleaner.

But I will follow the comment here to keep consistency within this file.


> 
> > +
> > +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.gao@intel.com>
> > +R: Ray Ni <ray.ni@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. :)


Agree. 'Graphics' is also used in the name of modules under MdeModulePkg.


> 
> (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.gao@intel.com>
> > +R: Ray Ni <ray.ni@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.bi@intel.com>
> > +R: Liming Gao <liming.gao@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.wu@intel.com>
> >  R: Ray Ni <ray.ni@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.wu@intel.com>
> > +R: Liming Gao <liming.gao@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.bi@intel.com>
> > +R: Eric Dong <eric.dong@intel.com>
> > +
> > +MdeModulePkg: Reset modules
> > +F: MdeModulePkg/*Reset*/
> > +F: MdeModulePkg/Include/*Reset*.h
> > +R: Zhichao Gao <zhichao.gao@intel.com>
> > +R: Ray Ni <ray.ni@intel.com>
> > +
> > +MdeModulePkg: S3 modules
> 
> (4) Can we say "ACPI S3 modules"? (Not insisting, just suggesting.)


I am fine with that. Will rename this one.


> 
> > +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.wu@intel.com>
> > +R: Eric Dong <eric.dong@intel.com>
> > +
> > +MdeModulePkg: SMBIOS modules
> > +F: MdeModulePkg/Universal/Smbios*/
> > +R: Dandan Bi <dandan.bi@intel.com>
> >  R: Star Zeng <star.zeng@intel.com>
> >
> > +MdeModulePkg: SMM modules
> 
> (5) Can we use the more generic term:
> 
>   Management Mode (MM, SMM) modules


Yes. The suggested name is better.


> 
> ?
> 
> > +F: MdeModulePkg/*Smi*/
> > +F: MdeModulePkg/*Smm*/
> > +F: MdeModulePkg/Include/*Smi*.h
> > +F: MdeModulePkg/Include/*Smm*.h
> > +R: Eric Dong <eric.dong@intel.com>
> > +R: Ray Ni <ray.ni@intel.com>
> > +
> > +MdeModulePkg: Variable modules
> 
> (6) Can we say: "UEFI Variable modules"?


Yes.

Best Regards,
Hao Wu


> 
> > +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.wu@intel.com>
> > +R: Liming Gao <liming.gao@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
> >
> 
> 
>