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

["Ni, Ray" <ray.ni@intel.com>]

Regarding the role assigned to me, Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Sent: Wednesday, July 17, 2019 10:37 PM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io; Andrew Fish
> <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Bi,
> Dandan <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Gao,
> Liming <liming.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Gao, Zhichao <zhichao.gao@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 <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.
> >
> > 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.
> >
> > > +
> > > +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. :)
> >
> > (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.)
> >
> > > +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
> >
> > ?
> >
> > > +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"?
> >
> > > +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
> > >
> >