Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members

["Pedro Falcato" <pedro.falcato@gmail.com>]

On Sun, Oct 29, 2023 at 1:30 PM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 10/29/23 03:16, Pedro Falcato wrote:
> >> diff --git a/Maintainers.txt b/Maintainers.txt
> >> index 3f40cdeb5554..2b03ccbe54aa 100644
> >> --- a/Maintainers.txt
> >> +++ b/Maintainers.txt
> >> @@ -93,7 +93,7 @@ M: Sami Mujawar <sami.mujawar@arm.com> [samimujawar]
> >>  RISCV64
> >>  F: */RiscV64/
> >>  M: Sunil V L <sunilvl@ventanamicro.com> [vlsunil]
> >> -R: Daniel Schaefer <git@danielschaefer.me> [JohnAZoidberg]
> >> +R: Andrei Warkentin <andrei.warkentin@intel.com> [andreiw]
> >>
> >>  LOONGARCH64
> >>  F: */LoongArch64/
> >> @@ -157,16 +157,6 @@ R: Leif Lindholm <quic_llindhol@quicinc.com> [leiflindholm]
> >>  R: Sami Mujawar <sami.mujawar@arm.com> [samimujawar]
> >>  R: Gerd Hoffmann <kraxel@redhat.com> [kraxel]
> >>
> >> -ArmVirtPkg: modules used on Xen
> >> -F: ArmVirtPkg/ArmVirtXen.*
> >> -F: ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/
> >> -F: ArmVirtPkg/Library/XenVirtMemInfoLib/
> >> -F: ArmVirtPkg/PrePi/
> >> -F: ArmVirtPkg/XenAcpiPlatformDxe/
> >> -F: ArmVirtPkg/XenPlatformHasAcpiDtDxe/
> >> -F: ArmVirtPkg/XenioFdtDxe/
> >> -R: Julien Grall <julien@xen.org> [jgrall]
> >
> > ArmVirtPkg Xen modules seize to have a dedicated maintainer. Can the
> > generic ArmVirtPkg maintainers handle *more code* (particularly,
> > functionality that's not trivial to test, unless you actively use
> > Xen)?
>
> An alternative to removing this entire section is to replace Julien's
> line with the following status line:
>
> S: Orphan
>
> The definition in Maintainers.txt is:
>
>      Orphan:     No current maintainer [but maybe you could take the
>                  role as you write your new code].
>
> I think this might be clearer for all three of: contributors, consumers,
> and existent maintainers.
>
> - Contributors: An ArmVirtPkg maintainer may techincally merge your
> code, but you won't get substantive feedback
>
> - Consumers: you can build and run this code, but if it breaks, you get
> to keep both parts
>
> - Existent ArmVirtPkg maintainers: you can rest assured in the knowledge
> that you are not saddled with deep technical reviews for this subsystem
> that you can't even boot in your environment. You're only responsible
> for the technical act of merging patches.

I agree with this solution, but I do think there should be a "time
limit" for orphaned code. You don't want to keep orphaned code for too
long, this is not a practice we should support (which may lead to
corporate code dumps where corps just dump a bunch of patches on the
mailing list, fire and forget, and they're still "supported").

>
> >
> >>  BaseTools
> >>  F: BaseTools/
> >>  W: https://github.com/tianocore/tianocore.github.io/wiki/BaseTools
> >> @@ -187,8 +177,7 @@ F: CryptoPkg/
> >>  W: https://github.com/tianocore/tianocore.github.io/wiki/CryptoPkg
> >>  M: Jiewen Yao <jiewen.yao@intel.com> [jyao1]
> >>  M: Yi Li <yi1.li@intel.com> [liyi77]
> >> -R: Xiaoyu Lu <xiaoyu1.lu@intel.com> [xiaoyuxlu]
> >> -R: Guomin Jiang <guomin.jiang@intel.com> [guominjia]
> >> +R: Wenxing Hou <wenxing.hou@intel.com> [Wenxing-hou]
> >>
> >>  DynamicTablesPkg
> >>  F: DynamicTablesPkg/
> >> @@ -202,7 +191,6 @@ W: https://github.com/tianocore/tianocore.github.io/wiki/EmbeddedPkg
> >>  M: Leif Lindholm <quic_llindhol@quicinc.com> [leiflindholm]
> >>  M: Ard Biesheuvel <ardb+tianocore@kernel.org> [ardbiesheuvel]
> >>  M: Abner Chang <abner.chang@amd.com> [changab]
> >> -R: Daniel Schaefer <git@danielschaefer.me> [JohnAZoidberg]
> >>
> >>  EmulatorPkg
> >>  F: EmulatorPkg/
> >> @@ -228,7 +216,6 @@ F: FmpDevicePkg/
> >>  W: https://github.com/tianocore/tianocore.github.io/wiki/FmpDevicePkg
> >>  M: Liming Gao <gaoliming@byosoft.com.cn> [lgao4]
> >>  M: Michael D Kinney <michael.d.kinney@intel.com> [mdkinney]
> >> -R: Guomin Jiang <guomin.jiang@intel.com> [guominjia]
> >>  R: Wei6 Xu <wei6.xu@intel.com> [xuweiintel]
> >>
> >>  IntelFsp2Pkg
> >> @@ -237,7 +224,6 @@ W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2Pkg
> >>  M: Chasel Chiu <chasel.chiu@intel.com> [ChaselChiu]
> >>  M: Nate DeSimone <nathaniel.l.desimone@intel.com> [nate-desimone]
> >>  M: Duggapu Chinni B <chinni.b.duggapu@intel.com> [cbduggap]
> >> -M: Ray Han Lim Ng <ray.han.lim.ng@intel.com> [rayhanlimng]
> >>  R: Star Zeng <star.zeng@intel.com> [lzeng14]
> >>  R: Ted Kuo <ted.kuo@intel.com> [tedkuo1]
> >>  R: Ashraf Ali S <ashraf.ali.s@intel.com> [AshrafAliS]
> >> @@ -258,7 +244,6 @@ R: Susovan Mohapatra <susovan.mohapatra@intel.com> [susovanmohapatra]
> >>  MdeModulePkg
> >>  F: MdeModulePkg/
> >>  W: https://github.com/tianocore/tianocore.github.io/wiki/MdeModulePkg
> >> -M: Jian J Wang <jian.j.wang@intel.com> [jwang36]
> >>  M: Liming Gao <gaoliming@byosoft.com.cn> [lgao4]
> >
> > MdeModulePkg now only has a single maintainer (Liming, who also
> > handles a myriad of other tasks and packages)
>
> This leads me to my main point: it may be time for edk2 to adopt a
> leaner contribution process.
>
> We can insist on no patch going in without maintainer approval, but that
> -- i.e., maintainer authority -- only works as long as it goes hand in
> hand with maintainer responsibility: timely reviews. If the community
> cannot offer enough working hours for reviewing patches for a subsystem,
> then the requirements to contribute to that subsystem should be relaxed.
> The other alternative is that the subsystem goes into stasis, where it
> becomes effectively impossible to contribute to a subsystem.
>
> (NB this "relaxation of contribution rules" is entirely orthogonal to
> using a mailing list vs. github pull requests. I still strongly prefer
> the mailing list.)
>
> So maybe we could say, if there is no patch review for like 7 working
> days (approx. one and half calendar weeks), then the patch should be
> merged by default. Put differently, switch from "rejected by default" to
> "accepted by default".

I understand your idea. I do, however, see it going in 2 different ways:
1) This pressures maintainers/corporations to be faster at reviewing
patches, keeping a smooth flow of careful, reviewed patches. Things
continue to work smoothly.
2) Maintainers keep being unresponsive, patches get "auto merged" and
people need to deal with any ensuing breakage. Things /may/ regularly
break.

IMO, this solution does not solve anything. If maintainers are short
on time (or simply spread too thin), they will still be short on time
unless corps give them more time for FOSS work. This just adds a fear
factor ("Complete shite may be automerged if you don't have people on
it!!").

And frankly, I find it hard to find a solution for this problem, since
the vast majority of the firmware product sits *outside* of the
open-source repos, sometimes with gnarly forks in the way. How do we
convince more people to engage out in the open?

>
> By the way, I would like to assist with MdeModulePkg reviews. I'm not
> sure if I can *commit* to that, but right now, that is my intent. (As
> always, I see maintainership / reviewership as a service, not as a
> privilege.)

Nice, thanks! I'll defer making any promises on my end, but I may end
up helping out (as I sometimes do, anyway).

>
> >>
> >>  MdeModulePkg: ACPI modules
> >> @@ -268,15 +253,6 @@ R: Zhiguang Liu <zhiguang.liu@intel.com> [LiuZhiguang001]
> >>  R: Dandan Bi <dandan.bi@intel.com> [dandanbi]
> >>  R: Liming Gao <gaoliming@byosoft.com.cn> [lgao4]
> >>
> >> -MdeModulePkg: ACPI modules related to S3
> >> -F: MdeModulePkg/*LockBox*/
> >> -F: MdeModulePkg/Include/*BootScript*.h
> >> -F: MdeModulePkg/Include/*LockBox*.h
> >> -F: MdeModulePkg/Include/*S3*.h
> >> -F: MdeModulePkg/Library/*S3*/
> >> -R: Hao A Wu <hao.a.wu@intel.com> [hwu25]
> >> -R: Eric Dong <eric.dong@intel.com> [ydong10]
> >> -
> >>  MdeModulePkg: BDS modules
> >>  F: MdeModulePkg/*BootManager*/
> >>  F: MdeModulePkg/Include/Library/UefiBootManagerLib.h
>
> Same story could apply here -- we could orphan S3 stuff as well.
>
> However, I can't deny I'm quite cranky at the thought of S3 breaking, at
> least in my trusty old configurations, so I'd certainly like to keep an
> eye on the S3 modules -- even if that only consisted of me
> regression-testing patches under OVMF (and not providing "expert
> feedback" on patch contents).
>

I know Intel has abandoned S3 over the (equally cursed!) S0ix/S2idle.
I don't know how the situation is on AMD/ARM. Do QEMU/libvirt/whatever
use S3 for anything useful? I know they do roughly support it.


> >> @@ -326,7 +302,6 @@ F: MdeModulePkg/Library/DxeSecurityManagementLib/
> >>  F: MdeModulePkg/Universal/PCD/
> >>  F: MdeModulePkg/Universal/PlatformDriOverrideDxe/
> >>  F: MdeModulePkg/Universal/SecurityStubDxe/SecurityStub.c
> >> -R: Dandan Bi <dandan.bi@intel.com> [dandanbi]
> >>  R: Liming Gao <gaoliming@byosoft.com.cn> [lgao4]
> >
> > Down to one reviewer.
>
> I'll try to assist whenever I can, wherever I notice something
> interesting -- I'm quite sure I'm going to be overwhelmed incredibly
> quickly, but at least I have that intent right now.

I really appreciate your involvement in EDK2, but if the alternative
for "Liming 'owns' half of EDK2.git" is "Liming AND Laszlo 'own' half
of EDK2.git", I don't see how this can be sustainable without
maintainer burnout :/


> >> -OvmfPkg: CSM modules
> >> -F: OvmfPkg/Csm/
> >> -R: David Woodhouse <dwmw2@infradead.org> [dwmw2]
> >
> > 0 people dedicated to OVMF CSM (although relatively low maintenance
> > overhead, from what it seems)
>
> In my view, we should orphan the CSM now. Or maybe even better, mark it as
>
>      Obsolete:   Old code. Something tagged obsolete generally means
>                  it has been replaced by a better system and you
>                  should be using that.
>
> Mid-term, we should figure out a "feature deprecation process" for edk2,
> and then remove the CSM altogether. Other projects I'm somewhat familiar
> with have deprecation policies; they announce / document a subsystem
> deprecated in one release, and then a number of releases later, the
> subsystem is removed completely. This gives users notice ahead of time,
> and lets them migrate to different solutions gradually.
>
> Lots of edk2 code have been removed already (Itanium support, Intel
> Framework stuff, etc); we didn't observe any deprecation policy back
> then. I don't know if there was any backlash from that. I'd be OK with
> removing the CSM at once (well, not in edk2-stable202311, but in the
> release after), but that might not be perceived as overly polite.

How is CSM looking in the virtualization space? Individual QEMU users
probably wouldn't miss it much (since SeaBIOS exists and even is the
default), but I don't know what you folks are using for firmware.

>
> >> -
> >>  OvmfPkg: Confidential Computing
> >>  F: OvmfPkg/AmdSev/
> >>  F: OvmfPkg/AmdSevDxe/
> >> @@ -545,7 +505,6 @@ F: OvmfPkg/PlatformPei/AmdSev.c
> >>  F: OvmfPkg/ResetVector/
> >>  F: OvmfPkg/Sec/
> >>  R: Erdem Aktas <erdemaktas@google.com> [ruleof2]
> >> -R: James Bottomley <jejb@linux.ibm.com> [jejb]
> >>  R: Jiewen Yao <jiewen.yao@intel.com> [jyao1]
> >>  R: Min Xu <min.m.xu@intel.com> [mxu9]
> >>  R: Tom Lendacky <thomas.lendacky@amd.com> [tlendacky]
>
> It's good for the project that CoCo has several reviewers still.

Yes, OVMF in general still seems quite well staffed (probably because
the development is done upstream!)

> >>  ShellPkg
> >> @@ -648,12 +603,10 @@ M: Zhichao Gao <zhichao.gao@intel.com> [ZhichaoGao]
> >>  SignedCapsulePkg
> >>  F: SignedCapsulePkg/
> >>  W: https://github.com/tianocore/tianocore.github.io/wiki/SignedCapsulePkg
> >> -M: Jian J Wang <jian.j.wang@intel.com> [jwang36]
> >
> > Unmaintained
>
> Probably best to mark it as orphaned then!

Wasn't Intel big on capsules?

> >>  SourceLevelDebugPkg
> >>  F: SourceLevelDebugPkg/
> >>  W: https://github.com/tianocore/tianocore.github.io/wiki/SourceLevelDebugPkg
> >> -M: Hao A Wu <hao.a.wu@intel.com> [hwu25]
> >
> > Unmaintained
> >>
> >>  StandaloneMmPkg
> >>  F: StandaloneMmPkg/
>
> I'd orphan this one as well. For one, I've never gotten
> SOURCE_DEBUG_ENABLE to work in OVMF.
>
> (I'd not go as far as removing it, I'm sure this module has many
> downstream users!)
>
>
> >> @@ -664,7 +617,6 @@ M: Ray Ni <ray.ni@intel.com> [niruiyu]
> >>  UefiCpuPkg
> >>  F: UefiCpuPkg/
> >>  W: https://github.com/tianocore/tianocore.github.io/wiki/UefiCpuPkg
> >> -M: Eric Dong <eric.dong@intel.com> [ydong10]
> >>  M: Ray Ni <ray.ni@intel.com> [niruiyu]
> >>  R: Rahul Kumar <rahul1.kumar@intel.com> [rahul1-kumar]
> >>  R: Gerd Hoffmann <kraxel@redhat.com> [kraxel]
> >> @@ -672,7 +624,6 @@ R: Gerd Hoffmann <kraxel@redhat.com> [kraxel]
> >>  UefiCpuPkg: Sec related modules
> >>  F: UefiCpuPkg/SecCore/
> >>  F: UefiCpuPkg/ResetVector/
> >> -R: Debkumar De <debkumar.de@intel.com> [dde01]
> >>  R: Catharine West <catharine.west@intel.com> [catharine-intl]
> >
> > One reviewer.
>
> Not necessarily alarming IMO, UefiCpuPkg in general is not neglected
> (Gerd is listed, and I would like to keep an eye on it too). So I'd
> rather phrase this one as "we even have a dedicated reviewer for
> 'UefiCpuPkg: Sec'!" :)

Heh :) FWIW, when we tried to use it for QemuOpenBoardPkg we found
multiple problems and instantly had to write our own SEC (although it
may have been PEBKAC, I don't know if this SecCore is used in real
platforms).

>
> >
> > Some brief LoC (taking into account code, blank lines and comments)
> > stats over some of the affected packages/modules:
> > SignedCapsulePkg - 6,836 LoC
> > SourceLevelDebugPkg - 15,208 LoC
> > MdeModulePkg - 616,591 LoC (!!)
> >    Bus/ -                216,268 LoC (!!!)
>
> Yep, these two are heavy-weights.
>
> > (HII and UI was tough to actually measure, but I'm relatively sure
> > it's 100,000+ LoC!)
>
> HII is unfortunately terribly difficult. The documentation is very
> lacking IMO (in the spec). I tried to read Tim Lewis's blog posts on it:
>
>   https://uefi.blogspot.com/search/label/UEFI%20HII
>
> but I didn't get far. It feels like one of the most over-engineered (or
> at least most complex) parts of UEFI / edk2. I once authored
> OvmfPkg/PlatformDxe, because Jordan really wanted me to; ever since I've
> been steering as clear of it as I could :) At least Dandan continues as
> a designated reviewer for HII!

Overengineered and underreviewed? Yikes...


> > Taking everything into account, I have two questions:
> > 1) Should we go through these changes (that effectively reflect
> > reality, that much I understand) and see what needs to be cut from
> > EDK2 (i.e do we have an overabundance of features)?
>
> I'd say "yes". To reiterate,
>
> - I'd propose explicitly marking orphaned subsystems as such, rather
> than merging them silently into parent subsystems,
>
> - certainly removing some subsystems, but for that, a "staged"
> deprecation policy would be most polite.

Agreed.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110274): https://edk2.groups.io/g/devel/message/110274
Mute This Topic: https://groups.io/mt/102245264/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-