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

["Laszlo Ersek" <lersek@redhat.com>]

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
>