At a recently concluded session at the Linux Plumbers Conference I
proposed a "Subsystem Profile" as a document that a maintainer can
provide to set contributor expectations and provide fodder for a
discussion between maintainers about the merits of different maintainer
policies.
For those that did not attend, the goal of the Subsystem Profile, and the
Maintainer Handbook more generally, is to provide a desk reference for
maintainers both new and experienced. The session introduction was:
The first rule of kernel maintenance is that there are no hard and
fast rules. That state of affairs is both a blessing and a curse. It
has served the community well to be adaptable to the different
people and different problem spaces that inhabit the kernel
community. However, that variability also leads to inconsistent
experiences for contributors, little to no guidance for new
contributors, and unnecessary stress on current maintainers. There
are quite a few of people who have been around long enough to make
enough mistakes that they have gained some hard earned proficiency.
However if the kernel community expects to keep growing it needs to
be able both scale the maintainers it has and ramp new ones without
necessarily let them make a decades worth of mistakes to learn the
ropes.
To be clear, the proposed document does not impose or suggest new
rules. Instead it provides an outlet to document the unwritten rules
and policies in effect for each subsystem, and that each subsystem
might decide differently for whatever reason.
---
Dan Williams (3):
MAINTAINERS: Reclaim the P: tag for Subsystem Profile
MAINTAINERS, Handbook: Subsystem Profile
libnvdimm, MAINTAINERS: Subsystem Profile
Documentation/maintainer/index.rst | 1
Documentation/maintainer/subsystem-profile.rst | 145 ++++++++++++++++++++++++
Documentation/nvdimm/subsystem-profile.rst | 86 ++++++++++++++
MAINTAINERS | 26 +++-
4 files changed, 249 insertions(+), 9 deletions(-)
create mode 100644 Documentation/maintainer/subsystem-profile.rst
create mode 100644 Documentation/nvdimm/subsystem-profile.rst
As presented at the 2018 Linux Plumbers conference [1], the Subsystem
Profile is proposed as a way to reduce friction between committers and
maintainers and perhaps encourage conversations amongst maintainers
about best practice policies.
The profile contains short answers to some of the common policy
questions a contributor might have, or that a maintainer might consider
formalizing. The current list of maintenance policies is:
Overview: General introduction to maintaining the subsystem
Core: List of source files considered core
Leaf: List of source files that consume core functionality
Patches or Pull requests: Simple statement of expected submission format
Last -rc for new feature submissions: Expected lead time for submissions
Last -rc to merge features: Deadline for merge decisions
Non-author Ack / Review Tags Required: Patch review economics
Test Suite: Pass this suite before requesting inclusion
Resubmit Cadence: When to ping the maintainer
Trusted Reviewers: Help for triaging patches
Time Zone / Office Hours: When might a maintainer be available
Checkpatch / Style Cleanups: Policy on pure cleanup patches
Off-list review: Request for review gates
TODO: Potential development tasks up for grabs, or active focus areas
The goal of the Subsystem Profile is to set expectations for
contributors and interim or replacement maintainers for a subsystem.
See Documentation/maintainer/subsystem-profile.rst for more details, and
a follow-on example profile for the libnvdimm subsystem.
[1]: https://linuxplumbersconf.org/event/2/contributions/59/
Cc: Jonathan Corbet <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Steve French <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Tobin C. Harding <[email protected]>
Cc: Olof Johansson <[email protected]>
Cc: Martin K. Petersen <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
Documentation/maintainer/index.rst | 1
Documentation/maintainer/subsystem-profile.rst | 145 ++++++++++++++++++++++++
MAINTAINERS | 4 +
3 files changed, 150 insertions(+)
create mode 100644 Documentation/maintainer/subsystem-profile.rst
diff --git a/Documentation/maintainer/index.rst b/Documentation/maintainer/index.rst
index 2a14916930cb..1e6b1aaa6024 100644
--- a/Documentation/maintainer/index.rst
+++ b/Documentation/maintainer/index.rst
@@ -11,4 +11,5 @@ additions to this manual.
configure-git
pull-requests
+ subsystem-profile
diff --git a/Documentation/maintainer/subsystem-profile.rst b/Documentation/maintainer/subsystem-profile.rst
new file mode 100644
index 000000000000..a74b624e0972
--- /dev/null
+++ b/Documentation/maintainer/subsystem-profile.rst
@@ -0,0 +1,145 @@
+.. _subsystemprofile:
+
+Subsystem Profile
+=================
+
+The Subsystem Profile is a collection of policy positions that a
+maintainer or maintainer team establishes for the their subsystem. While
+there is a wide range of technical nuance on maintaining disparate
+sections of the kernel, the Subsystem Profile documents a known set of
+major process policies that vary between subsystems. What follows is a
+list of policy questions a maintainer can answer and include a document
+in the kernel, or on an external website. It advertises to other
+maintainers and contributors the local policy of the subsystem. Some
+sections are optional like "Overview", "Off-list review", and "TODO".
+The others are recommended for all subsystems to address, but no section
+is mandatory. In addition there are no wrong answers, just document how
+a subsystem typically operates. Note that the profile follows the
+subsystem not the maintainer, i.e. there is no expectation that a
+maintainer of multiple subsystems deploys the same policy across those
+subsystems.
+
+
+Overview
+--------
+In this optional section of the profile provide a free form overview of
+the subsystem written as a hand-off document. In other words write a
+note to someone that would receive the “keys to the castle” in the event
+of extended or unexpected absence. “So, you have recently become the
+maintainer of the XYZ subsystem, condolences, it is a thankless job,
+here is the lay of the land.” Details to consider are the extended
+details that are not included in MAINTAINERS, and not addressed by the
+other profile questions below. For example details like, who has access
+to the git tree, branches that are pulled into -next, relevant
+specifications, issue trackers, and sensitive code areas. If available
+the Overview should link to other subsystem documentation that may
+clarify, re-iterate, emphasize / de-emphasize portions of the global
+process documentation for contributors (CodingStyle, SubmittingPatches,
+etc...).
+
+
+Core
+----
+A list of F: tags (as described by MAINTAINERS) listing what the
+maintainer considers to be core files. The review and lead time
+constraints for 'core' code may be stricter given the increased
+sensitivity and risk of change.
+
+
+Patches or Pull requests
+------------------------
+Some subsystems allow contributors to send pull requests, most require
+mailed patches. State “Patches only”, or “Pull requests accepted”.
+
+
+Last -rc for new feature submissions
+------------------------------------
+New feature submissions targeting the next merge window should have
+their first posting for consideration before this point. Patches that
+are submitted after this point should be clear that they are targeting
+the NEXT+1 merge window, or should come with sufficient justification
+why they should be considered on an expedited schedule. A general
+guideline is to set expectation with contributors that new feature
+submissions should appear before -rc5. The answer may be different for
+'Core:' files, include a second entry prefixed with 'Core:' if so.
+
+
+Last -rc to merge features
+--------------------------
+Indicate to contributors the point at which an as yet un-applied patch
+set will need to wait for the NEXT+1 merge window. Of course there is no
+obligation to ever except any given patchset, but if the review has not
+concluded by this point the expectation the contributor should wait and
+resubmit for the following merge window. The answer may be different for
+'Core:' files, include a second entry prefixed with 'Core:' if so.
+
+
+Non-author Ack / Review Tags Required
+-------------------------------------
+Let contributors and other maintainers know whether they can expect to
+see the maintainer self-commit patches without 3rd-party review. Some
+subsystem developer communities are so small as to make this requirement
+impractical. Others may have been bootstrapped by a submission of
+self-reviewed code at the outset, but have since moved to a
+non-author review-required stance. This section sets expectations on the
+code-review economics in the subsystem. For example, can a contributor
+trade review of a maintainer's, or other contributor's patches in
+exchange for consideration of their own.
+
+
+Test Suite
+----------
+Indicate the test suite all patches are expected to pass before being
+submitted for inclusion consideration.
+
+
+Resubmit Cadence
+----------------
+Define a rate at which a contributor should wait to resubmit a patchset
+that has not yet received comments. A general guideline is to try to
+meet a deadline of 1 - 2 weeks to acknowledge starting consideration for
+a patch set.
+
+
+Trusted Reviewers
+-----------------
+While a maintainer / maintainer-team is expected to be reviewer of last
+resort the review load is less onerous when distributed amongst
+contributors and or a trusted set of individuals. This section is
+distinct from the R: tag (Designated Reviewer). Whereas R: identifies
+reviewers that should always be copied on a patch submission, the
+trusted reviewers here are individuals contributors can reach out to if
+a few 'Resubmit Cadence' intervals have gone by without maintainer
+action, or to otherwise consult for advice.
+
+
+Time Zone / Office Hours
+------------------------
+Let contributors know the time of day when one or more maintainers are
+usually actively monitoring the mailing list.
+
+
+Checkpatch / Style Cleanups
+---------------------------
+For subsystems with long standing code bases it is reasonable to decline
+to accept pure coding-style fixup patches. This is where you can let
+contributors know “Standalone style-cleanups are welcome”,
+“Style-cleanups to existing code only welcome with other feature
+changes”, or “Standalone style-cleanups to existing code are not
+welcome”.
+
+
+Off-list review
+---------------
+A maintainer may optionally require that contributors seek prior review
+of patches before initial submission for upstream. For example,
+“Developers from organization X, please seek internal review before
+requesting upstream review”. This policy identifies occasions where a
+maintainer wants to reflect some of the review load back to an
+organization.
+
+
+TODO
+----
+In this optional section include a list of work items that might be
+suitable for onboarding a new developer to the subsystem.
diff --git a/MAINTAINERS b/MAINTAINERS
index 83b7b3943a12..bb4a83a7684d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -99,6 +99,10 @@ Descriptions of section entries:
Obsolete: Old code. Something tagged obsolete generally means
it has been replaced by a better system and you
should be using that.
+ P: Subsystem Profile document for the maintainer entry. This
+ is either an in-tree file or a URI to a document. The
+ contents of a Subsystem Profile are described in
+ Documentation/maintainer/subsystem-profile.rst.
F: Files and directories with wildcard patterns.
A trailing slash includes all files and subdirectory files.
F: drivers/net/ all files in and below drivers/net
Document the basic policies of the libnvdimm subsystem and provide a
first example of a Subsystem Profile for others to duplicate and edit.
Cc: Ross Zwisler <[email protected]>
Cc: Vishal Verma <[email protected]>
Cc: Dave Jiang <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
Documentation/nvdimm/subsystem-profile.rst | 86 ++++++++++++++++++++++++++++
MAINTAINERS | 4 +
2 files changed, 90 insertions(+)
create mode 100644 Documentation/nvdimm/subsystem-profile.rst
diff --git a/Documentation/nvdimm/subsystem-profile.rst b/Documentation/nvdimm/subsystem-profile.rst
new file mode 100644
index 000000000000..d3428be7528e
--- /dev/null
+++ b/Documentation/nvdimm/subsystem-profile.rst
@@ -0,0 +1,86 @@
+LIBNVDIMM Subsystem Profile
+===========================
+
+Overview
+--------
+So, you have recently become a maintainer of the LIBNVDIMM subsystem,
+condolences, it is a thankless job, here is the lay of the land. The git
+tree, git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/, is
+writable by all the individuals listed in LIBNVDIMM section of
+MAINTAINERS. Access is granted per the typical kernel.org account
+management policies. Two branches in that tree are regularly pulled into
+-next, libnvdimm-for-next, and libnvdimm-fixes. The submit rate of
+patches is low, usually enough for one person to handle. There is a
+patchwork instance at
+https://patchwork.kernel.org/project/linux-nvdimm/list/, and it
+historically is only used for ingesting patches and collecting
+ack/review tags, i.e. no expectation to update the patch state after it
+has been dispositioned, or merged.
+
+The most sensitive code area is the ACPI DSM (Device Specific Method)
+path. In addition to the general fragility of an ioctl() ABI the ACPI
+DSM scheme allows any vendor to implement any command without any prior
+review by the ACPI committee. For this reason the LIBNVDIMM system seeks
+to constrain the proliferation of vendor commands and at a minimum
+requires any command support to be publicly documented. Over time the
+submission rate of new vendor-specific commands is falling as more
+commands are defined with named methods in the official ACPI
+specification.
+
+LIBNVDIMM sits at the intersection of device-drivers, the block-layer,
+core memory-management, and filesystems. Be sure to re-route memory
+management patches to the -mm tree, and otherwise pull-in fs-devel for
+patches that touch anything related to DAX.
+
+Core
+----
+F: drivers/nvdimm/\*_devs.c
+F: drivers/acpi/nfit/\*.[ch]
+
+
+Patches or Pull requests
+------------------------
+Patches only
+
+
+Last day for new feature submissions
+------------------------------------
+Before -rc5
+
+
+Last day to merge features
+--------------------------
+End of last -rc
+
+
+Non-author Ack / Review Tags Required
+-------------------------------------
+Required
+
+
+Test Suite
+----------
+Run ‘make check’ from https://github.com/pmem/ndctl
+
+
+Trusted Reviewers
+-----------------
+Johannes Thumshirn
+Toshi Kani
+Jeff Moyer
+Robert Elliott
+
+
+Resubmit Cadence
+----------------
+8 business days
+
+
+Time Zone / Office Hours
+------------------------
+8:00am to 5:00pm Pacific Time Zone
+
+
+Checkpatch / Style cleanups
+---------------------------
+Standalone style-cleanups are welcome.
diff --git a/MAINTAINERS b/MAINTAINERS
index bb4a83a7684d..ba2beedd4605 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8439,6 +8439,7 @@ M: Dan Williams <[email protected]>
M: Vishal Verma <[email protected]>
M: Dave Jiang <[email protected]>
L: [email protected]
+P: Documentation/nvdimm/subsystem-profile.rst
Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
S: Supported
F: drivers/nvdimm/blk.c
@@ -8450,6 +8451,7 @@ M: Dan Williams <[email protected]>
M: Ross Zwisler <[email protected]>
M: Dave Jiang <[email protected]>
L: [email protected]
+P: Documentation/nvdimm/subsystem-profile.rst
Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
S: Supported
F: drivers/nvdimm/btt*
@@ -8460,6 +8462,7 @@ M: Dan Williams <[email protected]>
M: Vishal Verma <[email protected]>
M: Dave Jiang <[email protected]>
L: [email protected]
+P: Documentation/nvdimm/subsystem-profile.rst
Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
S: Supported
F: drivers/nvdimm/pmem*
@@ -8478,6 +8481,7 @@ M: Ross Zwisler <[email protected]>
M: Vishal Verma <[email protected]>
M: Dave Jiang <[email protected]>
L: [email protected]
+P: Documentation/nvdimm/subsystem-profile.rst
Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
T: git git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git
S: Supported
Fixup some P: entries to be M: and rename the remaining ones to 'E:' for
"entity". The P: tag will be used to indicate the location of a
Subsystem Profile for a given MAINTAINERS entry.
Cc: Joe Perches <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
MAINTAINERS | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 0abecc528dac..83b7b3943a12 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -76,7 +76,7 @@ trivial patch so apply some common sense.
Descriptions of section entries:
- P: Person (obsolete)
+ E: Entity (obsolete)
M: Mail patches to: FullName <address@domain>
R: Designated reviewer: FullName <address@domain>
These reviewers should be CCed on patches.
@@ -771,7 +771,7 @@ S: Orphan
F: drivers/usb/gadget/udc/amd5536udc.*
AMD GEODE PROCESSOR/CHIPSET SUPPORT
-P: Andres Salomon <[email protected]>
+M: Andres Salomon <[email protected]>
L: [email protected] (moderated for non-subscribers)
W: http://www.amd.com/us-en/ConnectivitySolutions/TechnicalResources/0,,50_2334_2452_11363,00.html
S: Supported
@@ -9271,7 +9271,7 @@ F: drivers/staging/media/tegra-vde/
MEDIA INPUT INFRASTRUCTURE (V4L/DVB)
M: Mauro Carvalho Chehab <[email protected]>
-P: LinuxTV.org Project
+E: LinuxTV.org Project
L: [email protected]
W: https://linuxtv.org
Q: http://patchwork.kernel.org/project/linux-media/list/
@@ -12465,7 +12465,7 @@ S: Maintained
F: arch/mips/ralink
RALINK RT2X00 WIRELESS LAN DRIVER
-P: rt2x00 project
+E: rt2x00 project
M: Stanislaw Gruszka <[email protected]>
M: Helmut Schaa <[email protected]>
L: [email protected]
@@ -12764,7 +12764,7 @@ S: Supported
F: drivers/net/ethernet/rocker/
ROCKETPORT DRIVER
-P: Comtrol Corp.
+E: Comtrol Corp.
W: http://www.comtrol.com
S: Maintained
F: Documentation/serial/rocket.txt
@@ -13577,15 +13577,15 @@ F: drivers/video/fbdev/simplefb.c
F: include/linux/platform_data/simplefb.h
SIMTEC EB110ATX (Chalice CATS)
-P: Ben Dooks
-P: Vincent Sanders <[email protected]>
+E: Ben Dooks
+M: Vincent Sanders <[email protected]>
M: Simtec Linux Team <[email protected]>
W: http://www.simtec.co.uk/products/EB110ATX/
S: Supported
SIMTEC EB2410ITX (BAST)
-P: Ben Dooks
-P: Vincent Sanders <[email protected]>
+E: Ben Dooks
+M: Vincent Sanders <[email protected]>
M: Simtec Linux Team <[email protected]>
W: http://www.simtec.co.uk/products/EB2410ITX/
S: Supported
Em Wed, 14 Nov 2018 20:53:19 -0800
Dan Williams <[email protected]> escreveu:
> Fixup some P: entries to be M: and rename the remaining ones to 'E:' for
> "entity". The P: tag will be used to indicate the location of a
> Subsystem Profile for a given MAINTAINERS entry.
>
> Cc: Joe Perches <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> MAINTAINERS | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0abecc528dac..83b7b3943a12 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -76,7 +76,7 @@ trivial patch so apply some common sense.
>
> Descriptions of section entries:
>
> - P: Person (obsolete)
> + E: Entity (obsolete)
I don't like very much the idea of renaming it, but that's just my 2 cents.
IMO, the best would be to replace them to a non-obsolete field on a patch
that would remove it, then another patch would re-add with a different
meaning.
On a quick look, I suspect we could get rid of all remaining entries.
See below.
> M: Mail patches to: FullName <address@domain>
> R: Designated reviewer: FullName <address@domain>
> These reviewers should be CCed on patches.
> @@ -771,7 +771,7 @@ S: Orphan
> F: drivers/usb/gadget/udc/amd5536udc.*
>
> AMD GEODE PROCESSOR/CHIPSET SUPPORT
> -P: Andres Salomon <[email protected]>
> +M: Andres Salomon <[email protected]>
> L: [email protected] (moderated for non-subscribers)
> W: http://www.amd.com/us-en/ConnectivitySolutions/TechnicalResources/0,,50_2334_2452_11363,00.html
> S: Supported
> @@ -9271,7 +9271,7 @@ F: drivers/staging/media/tegra-vde/
>
> MEDIA INPUT INFRASTRUCTURE (V4L/DVB)
> M: Mauro Carvalho Chehab <[email protected]>
> -P: LinuxTV.org Project
> +E: LinuxTV.org Project
LinuxTV is not really an entity. Just a name for a community-maintained site
where we store data about it.
That's already a "W:" tag pointing for it, so I would just remove it.
If you decide to remove, feel free to add my ack:
Acked-by: Mauro Carvalho Chehab <[email protected]>
> L: [email protected]
> W: https://linuxtv.org
> Q: http://patchwork.kernel.org/project/linux-media/list/
> @@ -12465,7 +12465,7 @@ S: Maintained
> F: arch/mips/ralink
>
> RALINK RT2X00 WIRELESS LAN DRIVER
> -P: rt2x00 project
> +E: rt2x00 project
I suspect that the above info is not really useful, and it is probably just
a left-over from the past history.
I suspect that, if removed, nobody will really miss it, as it is quite
obvious :-)
> M: Stanislaw Gruszka <[email protected]>
> M: Helmut Schaa <[email protected]>
> L: [email protected]
> @@ -12764,7 +12764,7 @@ S: Supported
> F: drivers/net/ethernet/rocker/
>
> ROCKETPORT DRIVER
> -P: Comtrol Corp.
> +E: Comtrol Corp.
There's already a W: field pointing to the company that maintains it.
So, IMHO this is simply duplicated stuff that can be removed.
> W: http://www.comtrol.com
> S: Maintained
> F: Documentation/serial/rocket.txt
> @@ -13577,15 +13577,15 @@ F: drivers/video/fbdev/simplefb.c
> F: include/linux/platform_data/simplefb.h
> SIMTEC EB110ATX (Chalice CATS)
> -P: Ben Dooks
> -P: Vincent Sanders <[email protected]>
> +E: Ben Dooks
> +M: Vincent Sanders <[email protected]>
> M: Simtec Linux Team <[email protected]>
> W: http://www.simtec.co.uk/products/EB110ATX/
> S: Supported
>
> SIMTEC EB2410ITX (BAST)
> -P: Ben Dooks
> -P: Vincent Sanders <[email protected]>
> +E: Ben Dooks
> +M: Vincent Sanders <[email protected]>
> M: Simtec Linux Team <[email protected]>
> W: http://www.simtec.co.uk/products/EB2410ITX/
> S: Supported
On the above two drivers, I've no idea why to keep it there. Ben Dooks
seems to be a past maintainer. He is already listed at CREDITS
for several things including Simtec.
So, I guess this information is duplicated/obsoleted and could just
be removed.
Cheers,
Mauro
Em Wed, 14 Nov 2018 20:53:25 -0800
Dan Williams <[email protected]> escreveu:
> As presented at the 2018 Linux Plumbers conference [1], the Subsystem
> Profile is proposed as a way to reduce friction between committers and
> maintainers and perhaps encourage conversations amongst maintainers
> about best practice policies.
>
> The profile contains short answers to some of the common policy
> questions a contributor might have, or that a maintainer might consider
> formalizing. The current list of maintenance policies is:
>
> Overview: General introduction to maintaining the subsystem
> Core: List of source files considered core
> Leaf: List of source files that consume core functionality
> Patches or Pull requests: Simple statement of expected submission format
> Last -rc for new feature submissions: Expected lead time for submissions
> Last -rc to merge features: Deadline for merge decisions
> Non-author Ack / Review Tags Required: Patch review economics
> Test Suite: Pass this suite before requesting inclusion
> Resubmit Cadence: When to ping the maintainer
> Trusted Reviewers: Help for triaging patches
There is one detail with regards to reviewing process that I'm not sure
how to express. There are some subsystems with co-maintainers, in the
sense that all co-maintainers are equally responsible for the subsystem.
There are other cases where there are sub-maintainers. That's, for example,
the model we use on media. There, we have different sub-maintainers for:
- V4L2 drivers
- Camera Sensors
- Remote Controllers
- HDMI CEC
- DVB
- Media Controller
The usual workflow is that they work as both reviewers and committers.
After they commit a certain amount of patches, they submit for me to
do a final review. This way, most of media patches have at least two
SOBs from non-authors.
On this model, a sub-maintainer is more than a trusted reviewer. Not sure
how to reflect it on this template.
I'll do a better review of the profile when I'll try to write a subsystem
profile for media.
Regards,
Mauro
> Time Zone / Office Hours: When might a maintainer be available
> Checkpatch / Style Cleanups: Policy on pure cleanup patches
> Off-list review: Request for review gates
> TODO: Potential development tasks up for grabs, or active focus areas
>
> The goal of the Subsystem Profile is to set expectations for
> contributors and interim or replacement maintainers for a subsystem.
>
> See Documentation/maintainer/subsystem-profile.rst for more details, and
> a follow-on example profile for the libnvdimm subsystem.
>
> [1]: https://linuxplumbersconf.org/event/2/contributions/59/
>
> Cc: Jonathan Corbet <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Steve French <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Cc: Tobin C. Harding <[email protected]>
> Cc: Olof Johansson <[email protected]>
> Cc: Martin K. Petersen <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: Joe Perches <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> Documentation/maintainer/index.rst | 1
> Documentation/maintainer/subsystem-profile.rst | 145 ++++++++++++++++++++++++
> MAINTAINERS | 4 +
> 3 files changed, 150 insertions(+)
> create mode 100644 Documentation/maintainer/subsystem-profile.rst
>
> diff --git a/Documentation/maintainer/index.rst b/Documentation/maintainer/index.rst
> index 2a14916930cb..1e6b1aaa6024 100644
> --- a/Documentation/maintainer/index.rst
> +++ b/Documentation/maintainer/index.rst
> @@ -11,4 +11,5 @@ additions to this manual.
>
> configure-git
> pull-requests
> + subsystem-profile
>
> diff --git a/Documentation/maintainer/subsystem-profile.rst b/Documentation/maintainer/subsystem-profile.rst
> new file mode 100644
> index 000000000000..a74b624e0972
> --- /dev/null
> +++ b/Documentation/maintainer/subsystem-profile.rst
> @@ -0,0 +1,145 @@
> +.. _subsystemprofile:
> +
> +Subsystem Profile
> +=================
> +
> +The Subsystem Profile is a collection of policy positions that a
> +maintainer or maintainer team establishes for the their subsystem. While
> +there is a wide range of technical nuance on maintaining disparate
> +sections of the kernel, the Subsystem Profile documents a known set of
> +major process policies that vary between subsystems. What follows is a
> +list of policy questions a maintainer can answer and include a document
> +in the kernel, or on an external website. It advertises to other
> +maintainers and contributors the local policy of the subsystem. Some
> +sections are optional like "Overview", "Off-list review", and "TODO".
> +The others are recommended for all subsystems to address, but no section
> +is mandatory. In addition there are no wrong answers, just document how
> +a subsystem typically operates. Note that the profile follows the
> +subsystem not the maintainer, i.e. there is no expectation that a
> +maintainer of multiple subsystems deploys the same policy across those
> +subsystems.
> +
> +
> +Overview
> +--------
> +In this optional section of the profile provide a free form overview of
> +the subsystem written as a hand-off document. In other words write a
> +note to someone that would receive the “keys to the castle” in the event
> +of extended or unexpected absence. “So, you have recently become the
> +maintainer of the XYZ subsystem, condolences, it is a thankless job,
> +here is the lay of the land.” Details to consider are the extended
> +details that are not included in MAINTAINERS, and not addressed by the
> +other profile questions below. For example details like, who has access
> +to the git tree, branches that are pulled into -next, relevant
> +specifications, issue trackers, and sensitive code areas. If available
> +the Overview should link to other subsystem documentation that may
> +clarify, re-iterate, emphasize / de-emphasize portions of the global
> +process documentation for contributors (CodingStyle, SubmittingPatches,
> +etc...).
> +
> +
> +Core
> +----
> +A list of F: tags (as described by MAINTAINERS) listing what the
> +maintainer considers to be core files. The review and lead time
> +constraints for 'core' code may be stricter given the increased
> +sensitivity and risk of change.
> +
> +
> +Patches or Pull requests
> +------------------------
> +Some subsystems allow contributors to send pull requests, most require
> +mailed patches. State “Patches only”, or “Pull requests accepted”.
> +
> +
> +Last -rc for new feature submissions
> +------------------------------------
> +New feature submissions targeting the next merge window should have
> +their first posting for consideration before this point. Patches that
> +are submitted after this point should be clear that they are targeting
> +the NEXT+1 merge window, or should come with sufficient justification
> +why they should be considered on an expedited schedule. A general
> +guideline is to set expectation with contributors that new feature
> +submissions should appear before -rc5. The answer may be different for
> +'Core:' files, include a second entry prefixed with 'Core:' if so.
> +
> +
> +Last -rc to merge features
> +--------------------------
> +Indicate to contributors the point at which an as yet un-applied patch
> +set will need to wait for the NEXT+1 merge window. Of course there is no
> +obligation to ever except any given patchset, but if the review has not
> +concluded by this point the expectation the contributor should wait and
> +resubmit for the following merge window. The answer may be different for
> +'Core:' files, include a second entry prefixed with 'Core:' if so.
> +
> +
> +Non-author Ack / Review Tags Required
> +-------------------------------------
> +Let contributors and other maintainers know whether they can expect to
> +see the maintainer self-commit patches without 3rd-party review. Some
> +subsystem developer communities are so small as to make this requirement
> +impractical. Others may have been bootstrapped by a submission of
> +self-reviewed code at the outset, but have since moved to a
> +non-author review-required stance. This section sets expectations on the
> +code-review economics in the subsystem. For example, can a contributor
> +trade review of a maintainer's, or other contributor's patches in
> +exchange for consideration of their own.
> +
> +
> +Test Suite
> +----------
> +Indicate the test suite all patches are expected to pass before being
> +submitted for inclusion consideration.
> +
> +
> +Resubmit Cadence
> +----------------
> +Define a rate at which a contributor should wait to resubmit a patchset
> +that has not yet received comments. A general guideline is to try to
> +meet a deadline of 1 - 2 weeks to acknowledge starting consideration for
> +a patch set.
> +
> +
> +Trusted Reviewers
> +-----------------
> +While a maintainer / maintainer-team is expected to be reviewer of last
> +resort the review load is less onerous when distributed amongst
> +contributors and or a trusted set of individuals. This section is
> +distinct from the R: tag (Designated Reviewer). Whereas R: identifies
> +reviewers that should always be copied on a patch submission, the
> +trusted reviewers here are individuals contributors can reach out to if
> +a few 'Resubmit Cadence' intervals have gone by without maintainer
> +action, or to otherwise consult for advice.
> +
> +
> +Time Zone / Office Hours
> +------------------------
> +Let contributors know the time of day when one or more maintainers are
> +usually actively monitoring the mailing list.
> +
> +
> +Checkpatch / Style Cleanups
> +---------------------------
> +For subsystems with long standing code bases it is reasonable to decline
> +to accept pure coding-style fixup patches. This is where you can let
> +contributors know “Standalone style-cleanups are welcome”,
> +“Style-cleanups to existing code only welcome with other feature
> +changes”, or “Standalone style-cleanups to existing code are not
> +welcome”.
> +
> +
> +Off-list review
> +---------------
> +A maintainer may optionally require that contributors seek prior review
> +of patches before initial submission for upstream. For example,
> +“Developers from organization X, please seek internal review before
> +requesting upstream review”. This policy identifies occasions where a
> +maintainer wants to reflect some of the review load back to an
> +organization.
> +
> +
> +TODO
> +----
> +In this optional section include a list of work items that might be
> +suitable for onboarding a new developer to the subsystem.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 83b7b3943a12..bb4a83a7684d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -99,6 +99,10 @@ Descriptions of section entries:
> Obsolete: Old code. Something tagged obsolete generally means
> it has been replaced by a better system and you
> should be using that.
> + P: Subsystem Profile document for the maintainer entry. This
> + is either an in-tree file or a URI to a document. The
> + contents of a Subsystem Profile are described in
> + Documentation/maintainer/subsystem-profile.rst.
> F: Files and directories with wildcard patterns.
> A trailing slash includes all files and subdirectory files.
> F: drivers/net/ all files in and below drivers/net
>
> _______________________________________________
> Ksummit-discuss mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss
Cheers,
Mauro
On Wed, 14 Nov 2018, Dan Williams wrote:
> As presented at the 2018 Linux Plumbers conference [1], the Subsystem
> Profile is proposed as a way to reduce friction between committers and
> maintainers and perhaps encourage conversations amongst maintainers
> about best practice policies.
>
> The profile contains short answers to some of the common policy
> questions a contributor might have, or that a maintainer might consider
> formalizing. The current list of maintenance policies is:
>
> Overview: General introduction to maintaining the subsystem
> Core: List of source files considered core
> Leaf: List of source files that consume core functionality
> Patches or Pull requests: Simple statement of expected submission format
> Last -rc for new feature submissions: Expected lead time for submissions
> Last -rc to merge features: Deadline for merge decisions
> Non-author Ack / Review Tags Required: Patch review economics
> Test Suite: Pass this suite before requesting inclusion
> Resubmit Cadence: When to ping the maintainer
> Trusted Reviewers: Help for triaging patches
> Time Zone / Office Hours: When might a maintainer be available
> Checkpatch / Style Cleanups: Policy on pure cleanup patches
> Off-list review: Request for review gates
> TODO: Potential development tasks up for grabs, or active focus areas
How about patch subject lines? What is the formula that should be used to
transform the name(s) of the affected file(s) into an appropriate suject
line?
thanks,
julia
>
> The goal of the Subsystem Profile is to set expectations for
> contributors and interim or replacement maintainers for a subsystem.
>
> See Documentation/maintainer/subsystem-profile.rst for more details, and
> a follow-on example profile for the libnvdimm subsystem.
>
> [1]: https://linuxplumbersconf.org/event/2/contributions/59/
>
> Cc: Jonathan Corbet <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Steve French <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Cc: Tobin C. Harding <[email protected]>
> Cc: Olof Johansson <[email protected]>
> Cc: Martin K. Petersen <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: Joe Perches <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> Documentation/maintainer/index.rst | 1
> Documentation/maintainer/subsystem-profile.rst | 145 ++++++++++++++++++++++++
> MAINTAINERS | 4 +
> 3 files changed, 150 insertions(+)
> create mode 100644 Documentation/maintainer/subsystem-profile.rst
>
> diff --git a/Documentation/maintainer/index.rst b/Documentation/maintainer/index.rst
> index 2a14916930cb..1e6b1aaa6024 100644
> --- a/Documentation/maintainer/index.rst
> +++ b/Documentation/maintainer/index.rst
> @@ -11,4 +11,5 @@ additions to this manual.
>
> configure-git
> pull-requests
> + subsystem-profile
>
> diff --git a/Documentation/maintainer/subsystem-profile.rst b/Documentation/maintainer/subsystem-profile.rst
> new file mode 100644
> index 000000000000..a74b624e0972
> --- /dev/null
> +++ b/Documentation/maintainer/subsystem-profile.rst
> @@ -0,0 +1,145 @@
> +.. _subsystemprofile:
> +
> +Subsystem Profile
> +=================
> +
> +The Subsystem Profile is a collection of policy positions that a
> +maintainer or maintainer team establishes for the their subsystem. While
> +there is a wide range of technical nuance on maintaining disparate
> +sections of the kernel, the Subsystem Profile documents a known set of
> +major process policies that vary between subsystems. What follows is a
> +list of policy questions a maintainer can answer and include a document
> +in the kernel, or on an external website. It advertises to other
> +maintainers and contributors the local policy of the subsystem. Some
> +sections are optional like "Overview", "Off-list review", and "TODO".
> +The others are recommended for all subsystems to address, but no section
> +is mandatory. In addition there are no wrong answers, just document how
> +a subsystem typically operates. Note that the profile follows the
> +subsystem not the maintainer, i.e. there is no expectation that a
> +maintainer of multiple subsystems deploys the same policy across those
> +subsystems.
> +
> +
> +Overview
> +--------
> +In this optional section of the profile provide a free form overview of
> +the subsystem written as a hand-off document. In other words write a
> +note to someone that would receive the “keys to the castle” in the event
> +of extended or unexpected absence. “So, you have recently become the
> +maintainer of the XYZ subsystem, condolences, it is a thankless job,
> +here is the lay of the land.” Details to consider are the extended
> +details that are not included in MAINTAINERS, and not addressed by the
> +other profile questions below. For example details like, who has access
> +to the git tree, branches that are pulled into -next, relevant
> +specifications, issue trackers, and sensitive code areas. If available
> +the Overview should link to other subsystem documentation that may
> +clarify, re-iterate, emphasize / de-emphasize portions of the global
> +process documentation for contributors (CodingStyle, SubmittingPatches,
> +etc...).
> +
> +
> +Core
> +----
> +A list of F: tags (as described by MAINTAINERS) listing what the
> +maintainer considers to be core files. The review and lead time
> +constraints for 'core' code may be stricter given the increased
> +sensitivity and risk of change.
> +
> +
> +Patches or Pull requests
> +------------------------
> +Some subsystems allow contributors to send pull requests, most require
> +mailed patches. State “Patches only”, or “Pull requests accepted”.
> +
> +
> +Last -rc for new feature submissions
> +------------------------------------
> +New feature submissions targeting the next merge window should have
> +their first posting for consideration before this point. Patches that
> +are submitted after this point should be clear that they are targeting
> +the NEXT+1 merge window, or should come with sufficient justification
> +why they should be considered on an expedited schedule. A general
> +guideline is to set expectation with contributors that new feature
> +submissions should appear before -rc5. The answer may be different for
> +'Core:' files, include a second entry prefixed with 'Core:' if so.
> +
> +
> +Last -rc to merge features
> +--------------------------
> +Indicate to contributors the point at which an as yet un-applied patch
> +set will need to wait for the NEXT+1 merge window. Of course there is no
> +obligation to ever except any given patchset, but if the review has not
> +concluded by this point the expectation the contributor should wait and
> +resubmit for the following merge window. The answer may be different for
> +'Core:' files, include a second entry prefixed with 'Core:' if so.
> +
> +
> +Non-author Ack / Review Tags Required
> +-------------------------------------
> +Let contributors and other maintainers know whether they can expect to
> +see the maintainer self-commit patches without 3rd-party review. Some
> +subsystem developer communities are so small as to make this requirement
> +impractical. Others may have been bootstrapped by a submission of
> +self-reviewed code at the outset, but have since moved to a
> +non-author review-required stance. This section sets expectations on the
> +code-review economics in the subsystem. For example, can a contributor
> +trade review of a maintainer's, or other contributor's patches in
> +exchange for consideration of their own.
> +
> +
> +Test Suite
> +----------
> +Indicate the test suite all patches are expected to pass before being
> +submitted for inclusion consideration.
> +
> +
> +Resubmit Cadence
> +----------------
> +Define a rate at which a contributor should wait to resubmit a patchset
> +that has not yet received comments. A general guideline is to try to
> +meet a deadline of 1 - 2 weeks to acknowledge starting consideration for
> +a patch set.
> +
> +
> +Trusted Reviewers
> +-----------------
> +While a maintainer / maintainer-team is expected to be reviewer of last
> +resort the review load is less onerous when distributed amongst
> +contributors and or a trusted set of individuals. This section is
> +distinct from the R: tag (Designated Reviewer). Whereas R: identifies
> +reviewers that should always be copied on a patch submission, the
> +trusted reviewers here are individuals contributors can reach out to if
> +a few 'Resubmit Cadence' intervals have gone by without maintainer
> +action, or to otherwise consult for advice.
> +
> +
> +Time Zone / Office Hours
> +------------------------
> +Let contributors know the time of day when one or more maintainers are
> +usually actively monitoring the mailing list.
> +
> +
> +Checkpatch / Style Cleanups
> +---------------------------
> +For subsystems with long standing code bases it is reasonable to decline
> +to accept pure coding-style fixup patches. This is where you can let
> +contributors know “Standalone style-cleanups are welcome”,
> +“Style-cleanups to existing code only welcome with other feature
> +changes”, or “Standalone style-cleanups to existing code are not
> +welcome”.
> +
> +
> +Off-list review
> +---------------
> +A maintainer may optionally require that contributors seek prior review
> +of patches before initial submission for upstream. For example,
> +“Developers from organization X, please seek internal review before
> +requesting upstream review”. This policy identifies occasions where a
> +maintainer wants to reflect some of the review load back to an
> +organization.
> +
> +
> +TODO
> +----
> +In this optional section include a list of work items that might be
> +suitable for onboarding a new developer to the subsystem.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 83b7b3943a12..bb4a83a7684d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -99,6 +99,10 @@ Descriptions of section entries:
> Obsolete: Old code. Something tagged obsolete generally means
> it has been replaced by a better system and you
> should be using that.
> + P: Subsystem Profile document for the maintainer entry. This
> + is either an in-tree file or a URI to a document. The
> + contents of a Subsystem Profile are described in
> + Documentation/maintainer/subsystem-profile.rst.
> F: Files and directories with wildcard patterns.
> A trailing slash includes all files and subdirectory files.
> F: drivers/net/ all files in and below drivers/net
>
> _______________________________________________
> Ksummit-discuss mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss
>
Em Wed, 14 Nov 2018 20:53:13 -0800
Dan Williams <[email protected]> escreveu:
> At a recently concluded session at the Linux Plumbers Conference I
> proposed a "Subsystem Profile" as a document that a maintainer can
> provide to set contributor expectations and provide fodder for a
> discussion between maintainers about the merits of different maintainer
> policies.
As discussed at LPC, please don't forget to add a patch teaching
get_maintainers.pl to print the subsystem rules tag (if any) at the
final version of this series.
Cheers,
Mauro
Hi Dan,
On Thu, Nov 15, 2018 at 6:05 AM Dan Williams <[email protected]> wrote:
> As presented at the 2018 Linux Plumbers conference [1], the Subsystem
> Profile is proposed as a way to reduce friction between committers and
> maintainers and perhaps encourage conversations amongst maintainers
> about best practice policies.
>
> The profile contains short answers to some of the common policy
> questions a contributor might have, or that a maintainer might consider
> formalizing. The current list of maintenance policies is:
>
> Overview: General introduction to maintaining the subsystem
> Core: List of source files considered core
> Leaf: List of source files that consume core functionality
> Patches or Pull requests: Simple statement of expected submission format
> Last -rc for new feature submissions: Expected lead time for submissions
> Last -rc to merge features: Deadline for merge decisions
> Non-author Ack / Review Tags Required: Patch review economics
> Test Suite: Pass this suite before requesting inclusion
> Resubmit Cadence: When to ping the maintainer
> Trusted Reviewers: Help for triaging patches
> Time Zone / Office Hours: When might a maintainer be available
> Checkpatch / Style Cleanups: Policy on pure cleanup patches
> Off-list review: Request for review gates
> TODO: Potential development tasks up for grabs, or active focus areas
>
> The goal of the Subsystem Profile is to set expectations for
> contributors and interim or replacement maintainers for a subsystem.
>
> See Documentation/maintainer/subsystem-profile.rst for more details, and
> a follow-on example profile for the libnvdimm subsystem.
> Signed-off-by: Dan Williams <[email protected]>
Thanks for your patch!
> --- /dev/null
> +++ b/Documentation/maintainer/subsystem-profile.rst
> +Last -rc to merge features
> +--------------------------
> +Indicate to contributors the point at which an as yet un-applied patch
> +set will need to wait for the NEXT+1 merge window. Of course there is no
> +obligation to ever except any given patchset, but if the review has not
s/except/accept/
> +concluded by this point the expectation the contributor should wait and
expectation is (that)
> +resubmit for the following merge window. The answer may be different for
> +'Core:' files, include a second entry prefixed with 'Core:' if so.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Julia,
On Thu, Nov 15, 2018 at 6:48 AM Julia Lawall <[email protected]> wrote:
> On Wed, 14 Nov 2018, Dan Williams wrote:
> > As presented at the 2018 Linux Plumbers conference [1], the Subsystem
> > Profile is proposed as a way to reduce friction between committers and
> > maintainers and perhaps encourage conversations amongst maintainers
> > about best practice policies.
> >
> > The profile contains short answers to some of the common policy
> > questions a contributor might have, or that a maintainer might consider
> > formalizing. The current list of maintenance policies is:
> >
> > Overview: General introduction to maintaining the subsystem
> > Core: List of source files considered core
> > Leaf: List of source files that consume core functionality
> > Patches or Pull requests: Simple statement of expected submission format
> > Last -rc for new feature submissions: Expected lead time for submissions
> > Last -rc to merge features: Deadline for merge decisions
> > Non-author Ack / Review Tags Required: Patch review economics
> > Test Suite: Pass this suite before requesting inclusion
> > Resubmit Cadence: When to ping the maintainer
> > Trusted Reviewers: Help for triaging patches
> > Time Zone / Office Hours: When might a maintainer be available
> > Checkpatch / Style Cleanups: Policy on pure cleanup patches
> > Off-list review: Request for review gates
> > TODO: Potential development tasks up for grabs, or active focus areas
>
> How about patch subject lines? What is the formula that should be used to
> transform the name(s) of the affected file(s) into an appropriate suject
> line?
Automating that may be difficult.
I always use "git log --oneline", and try to derive something sane
from its output.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Dan,
On Thu, Nov 15, 2018 at 6:06 AM Dan Williams <[email protected]> wrote:
> Document the basic policies of the libnvdimm subsystem and provide a
> first example of a Subsystem Profile for others to duplicate and edit.
>
> Cc: Ross Zwisler <[email protected]>
> Cc: Vishal Verma <[email protected]>
> Cc: Dave Jiang <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
Thanks for your patch!
> --- /dev/null
> +++ b/Documentation/nvdimm/subsystem-profile.rst
> +Trusted Reviewers
> +-----------------
> +Johannes Thumshirn
> +Toshi Kani
> +Jeff Moyer
> +Robert Elliott
Don't you want to add email addresses?
Only the first one is listed in MAINTAINERS.
> +Time Zone / Office Hours
> +------------------------
> +8:00am to 5:00pm Pacific Time Zone
UTC-???
People are not familiar with all time zones.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Cc: linux-doc
On Wed, 14 Nov 2018, Dan Williams <[email protected]> wrote:
> As presented at the 2018 Linux Plumbers conference [1], the Subsystem
> Profile is proposed as a way to reduce friction between committers and
> maintainers and perhaps encourage conversations amongst maintainers
> about best practice policies.
>
> The profile contains short answers to some of the common policy
> questions a contributor might have, or that a maintainer might consider
> formalizing. The current list of maintenance policies is:
>
> Overview: General introduction to maintaining the subsystem
> Core: List of source files considered core
> Leaf: List of source files that consume core functionality
> Patches or Pull requests: Simple statement of expected submission format
> Last -rc for new feature submissions: Expected lead time for submissions
> Last -rc to merge features: Deadline for merge decisions
> Non-author Ack / Review Tags Required: Patch review economics
> Test Suite: Pass this suite before requesting inclusion
> Resubmit Cadence: When to ping the maintainer
> Trusted Reviewers: Help for triaging patches
> Time Zone / Office Hours: When might a maintainer be available
> Checkpatch / Style Cleanups: Policy on pure cleanup patches
> Off-list review: Request for review gates
> TODO: Potential development tasks up for grabs, or active focus areas
>
> The goal of the Subsystem Profile is to set expectations for
> contributors and interim or replacement maintainers for a subsystem.
First of all, I welcome documentation efforts like this.
The cover letter mainly focuses on the maintainer aspect, and the
documentation is added to the maintainer handbook. However, here you set
the goal as setting expectations for contributors. The example nvdimm
profile in patch 3/3 addresses the reader as a new maintainer, yet goes
on to set expectations also for contributors, not just the maintainer.
I do think the documentation for contributors and maintainers/committers
should be kept separate. Most contributors will never care about the
documentation for the latter. We have Documentation/process for
contributors, and I think the audience of Documentation/maintainer
should be strictly maintainers.
In summary, I do think we need all of the documentation you propose, and
I appreciate you taking this on, but I think this should be split by
audience.
BR,
Jani.
>
> See Documentation/maintainer/subsystem-profile.rst for more details, and
> a follow-on example profile for the libnvdimm subsystem.
>
> [1]: https://linuxplumbersconf.org/event/2/contributions/59/
>
> Cc: Jonathan Corbet <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Steve French <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Cc: Tobin C. Harding <[email protected]>
> Cc: Olof Johansson <[email protected]>
> Cc: Martin K. Petersen <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: Joe Perches <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> Documentation/maintainer/index.rst | 1
> Documentation/maintainer/subsystem-profile.rst | 145 ++++++++++++++++++++++++
> MAINTAINERS | 4 +
> 3 files changed, 150 insertions(+)
> create mode 100644 Documentation/maintainer/subsystem-profile.rst
>
> diff --git a/Documentation/maintainer/index.rst b/Documentation/maintainer/index.rst
> index 2a14916930cb..1e6b1aaa6024 100644
> --- a/Documentation/maintainer/index.rst
> +++ b/Documentation/maintainer/index.rst
> @@ -11,4 +11,5 @@ additions to this manual.
>
> configure-git
> pull-requests
> + subsystem-profile
>
> diff --git a/Documentation/maintainer/subsystem-profile.rst b/Documentation/maintainer/subsystem-profile.rst
> new file mode 100644
> index 000000000000..a74b624e0972
> --- /dev/null
> +++ b/Documentation/maintainer/subsystem-profile.rst
> @@ -0,0 +1,145 @@
> +.. _subsystemprofile:
> +
> +Subsystem Profile
> +=================
> +
> +The Subsystem Profile is a collection of policy positions that a
> +maintainer or maintainer team establishes for the their subsystem. While
> +there is a wide range of technical nuance on maintaining disparate
> +sections of the kernel, the Subsystem Profile documents a known set of
> +major process policies that vary between subsystems. What follows is a
> +list of policy questions a maintainer can answer and include a document
> +in the kernel, or on an external website. It advertises to other
> +maintainers and contributors the local policy of the subsystem. Some
> +sections are optional like "Overview", "Off-list review", and "TODO".
> +The others are recommended for all subsystems to address, but no section
> +is mandatory. In addition there are no wrong answers, just document how
> +a subsystem typically operates. Note that the profile follows the
> +subsystem not the maintainer, i.e. there is no expectation that a
> +maintainer of multiple subsystems deploys the same policy across those
> +subsystems.
> +
> +
> +Overview
> +--------
> +In this optional section of the profile provide a free form overview of
> +the subsystem written as a hand-off document. In other words write a
> +note to someone that would receive the “keys to the castle” in the event
> +of extended or unexpected absence. “So, you have recently become the
> +maintainer of the XYZ subsystem, condolences, it is a thankless job,
> +here is the lay of the land.” Details to consider are the extended
> +details that are not included in MAINTAINERS, and not addressed by the
> +other profile questions below. For example details like, who has access
> +to the git tree, branches that are pulled into -next, relevant
> +specifications, issue trackers, and sensitive code areas. If available
> +the Overview should link to other subsystem documentation that may
> +clarify, re-iterate, emphasize / de-emphasize portions of the global
> +process documentation for contributors (CodingStyle, SubmittingPatches,
> +etc...).
> +
> +
> +Core
> +----
> +A list of F: tags (as described by MAINTAINERS) listing what the
> +maintainer considers to be core files. The review and lead time
> +constraints for 'core' code may be stricter given the increased
> +sensitivity and risk of change.
> +
> +
> +Patches or Pull requests
> +------------------------
> +Some subsystems allow contributors to send pull requests, most require
> +mailed patches. State “Patches only”, or “Pull requests accepted”.
> +
> +
> +Last -rc for new feature submissions
> +------------------------------------
> +New feature submissions targeting the next merge window should have
> +their first posting for consideration before this point. Patches that
> +are submitted after this point should be clear that they are targeting
> +the NEXT+1 merge window, or should come with sufficient justification
> +why they should be considered on an expedited schedule. A general
> +guideline is to set expectation with contributors that new feature
> +submissions should appear before -rc5. The answer may be different for
> +'Core:' files, include a second entry prefixed with 'Core:' if so.
> +
> +
> +Last -rc to merge features
> +--------------------------
> +Indicate to contributors the point at which an as yet un-applied patch
> +set will need to wait for the NEXT+1 merge window. Of course there is no
> +obligation to ever except any given patchset, but if the review has not
> +concluded by this point the expectation the contributor should wait and
> +resubmit for the following merge window. The answer may be different for
> +'Core:' files, include a second entry prefixed with 'Core:' if so.
> +
> +
> +Non-author Ack / Review Tags Required
> +-------------------------------------
> +Let contributors and other maintainers know whether they can expect to
> +see the maintainer self-commit patches without 3rd-party review. Some
> +subsystem developer communities are so small as to make this requirement
> +impractical. Others may have been bootstrapped by a submission of
> +self-reviewed code at the outset, but have since moved to a
> +non-author review-required stance. This section sets expectations on the
> +code-review economics in the subsystem. For example, can a contributor
> +trade review of a maintainer's, or other contributor's patches in
> +exchange for consideration of their own.
> +
> +
> +Test Suite
> +----------
> +Indicate the test suite all patches are expected to pass before being
> +submitted for inclusion consideration.
> +
> +
> +Resubmit Cadence
> +----------------
> +Define a rate at which a contributor should wait to resubmit a patchset
> +that has not yet received comments. A general guideline is to try to
> +meet a deadline of 1 - 2 weeks to acknowledge starting consideration for
> +a patch set.
> +
> +
> +Trusted Reviewers
> +-----------------
> +While a maintainer / maintainer-team is expected to be reviewer of last
> +resort the review load is less onerous when distributed amongst
> +contributors and or a trusted set of individuals. This section is
> +distinct from the R: tag (Designated Reviewer). Whereas R: identifies
> +reviewers that should always be copied on a patch submission, the
> +trusted reviewers here are individuals contributors can reach out to if
> +a few 'Resubmit Cadence' intervals have gone by without maintainer
> +action, or to otherwise consult for advice.
> +
> +
> +Time Zone / Office Hours
> +------------------------
> +Let contributors know the time of day when one or more maintainers are
> +usually actively monitoring the mailing list.
> +
> +
> +Checkpatch / Style Cleanups
> +---------------------------
> +For subsystems with long standing code bases it is reasonable to decline
> +to accept pure coding-style fixup patches. This is where you can let
> +contributors know “Standalone style-cleanups are welcome”,
> +“Style-cleanups to existing code only welcome with other feature
> +changes”, or “Standalone style-cleanups to existing code are not
> +welcome”.
> +
> +
> +Off-list review
> +---------------
> +A maintainer may optionally require that contributors seek prior review
> +of patches before initial submission for upstream. For example,
> +“Developers from organization X, please seek internal review before
> +requesting upstream review”. This policy identifies occasions where a
> +maintainer wants to reflect some of the review load back to an
> +organization.
> +
> +
> +TODO
> +----
> +In this optional section include a list of work items that might be
> +suitable for onboarding a new developer to the subsystem.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 83b7b3943a12..bb4a83a7684d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -99,6 +99,10 @@ Descriptions of section entries:
> Obsolete: Old code. Something tagged obsolete generally means
> it has been replaced by a better system and you
> should be using that.
> + P: Subsystem Profile document for the maintainer entry. This
> + is either an in-tree file or a URI to a document. The
> + contents of a Subsystem Profile are described in
> + Documentation/maintainer/subsystem-profile.rst.
> F: Files and directories with wildcard patterns.
> A trailing slash includes all files and subdirectory files.
> F: drivers/net/ all files in and below drivers/net
>
> _______________________________________________
> Ksummit-discuss mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss
--
Jani Nikula, Intel Open Source Graphics Center
On Thu, 15 Nov 2018, Geert Uytterhoeven wrote:
> Hi Julia,
>
> On Thu, Nov 15, 2018 at 6:48 AM Julia Lawall <[email protected]> wrote:
> > On Wed, 14 Nov 2018, Dan Williams wrote:
> > > As presented at the 2018 Linux Plumbers conference [1], the Subsystem
> > > Profile is proposed as a way to reduce friction between committers and
> > > maintainers and perhaps encourage conversations amongst maintainers
> > > about best practice policies.
> > >
> > > The profile contains short answers to some of the common policy
> > > questions a contributor might have, or that a maintainer might consider
> > > formalizing. The current list of maintenance policies is:
> > >
> > > Overview: General introduction to maintaining the subsystem
> > > Core: List of source files considered core
> > > Leaf: List of source files that consume core functionality
> > > Patches or Pull requests: Simple statement of expected submission format
> > > Last -rc for new feature submissions: Expected lead time for submissions
> > > Last -rc to merge features: Deadline for merge decisions
> > > Non-author Ack / Review Tags Required: Patch review economics
> > > Test Suite: Pass this suite before requesting inclusion
> > > Resubmit Cadence: When to ping the maintainer
> > > Trusted Reviewers: Help for triaging patches
> > > Time Zone / Office Hours: When might a maintainer be available
> > > Checkpatch / Style Cleanups: Policy on pure cleanup patches
> > > Off-list review: Request for review gates
> > > TODO: Potential development tasks up for grabs, or active focus areas
> >
> > How about patch subject lines? What is the formula that should be used to
> > transform the name(s) of the affected file(s) into an appropriate suject
> > line?
>
> Automating that may be difficult.
> I always use "git log --oneline", and try to derive something sane
> from its output.
Yes, I do likewise. But there may be some subsystems for which it would
be possible to come up with a more specific policy. The advantage of what
is proposed here is that it is not necessary to come up with a single
formula that works everywhere. Even a description in English could be
helpful.
Other subsystem specific properties such as what tree to work on, whether
to CC stable, ordering of variables or header files, etc could also be
useful to mention. Anything that will cause a maintainer to immediately
reject a patch for syntactic reasons.
julia
Em Thu, 15 Nov 2018 09:03:11 +0100
Geert Uytterhoeven <[email protected]> escreveu:
> Hi Dan,
>
> On Thu, Nov 15, 2018 at 6:06 AM Dan Williams <[email protected]> wrote:
> > Document the basic policies of the libnvdimm subsystem and provide a
> > first example of a Subsystem Profile for others to duplicate and edit.
> >
> > Cc: Ross Zwisler <[email protected]>
> > Cc: Vishal Verma <[email protected]>
> > Cc: Dave Jiang <[email protected]>
> > Signed-off-by: Dan Williams <[email protected]>
>
> Thanks for your patch!
>
> > --- /dev/null
> > +++ b/Documentation/nvdimm/subsystem-profile.rst
>
> > +Trusted Reviewers
> > +-----------------
> > +Johannes Thumshirn
> > +Toshi Kani
> > +Jeff Moyer
> > +Robert Elliott
>
> Don't you want to add email addresses?
> Only the first one is listed in MAINTAINERS.
IMO, it makes sense to have their e-mails here, in a way that it could
easily be parsed by get_maintainers.pl.
That's said, IMO a given subsystem should either use the R: tag at
MAINTAINERS or have a list of reviewers or sub-maintainers here, as
maintaining duplicated information will be painful.
If we go to this path, we should probably document it at MAINTAINERS
file.
>
> > +Time Zone / Office Hours
> > +------------------------
> > +8:00am to 5:00pm Pacific Time Zone
>
> UTC-???
>
> People are not familiar with all time zones.
>
> Gr{oetje,eeting}s,
>
> Geert
>
Cheers,
Mauro
Em Wed, 14 Nov 2018 20:53:30 -0800
Dan Williams <[email protected]> escreveu:
> Document the basic policies of the libnvdimm subsystem and provide a
> first example of a Subsystem Profile for others to duplicate and edit.
>
> Cc: Ross Zwisler <[email protected]>
> Cc: Vishal Verma <[email protected]>
> Cc: Dave Jiang <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> Documentation/nvdimm/subsystem-profile.rst | 86 ++++++++++++++++++++++++++++
> MAINTAINERS | 4 +
> 2 files changed, 90 insertions(+)
> create mode 100644 Documentation/nvdimm/subsystem-profile.rst
>
> diff --git a/Documentation/nvdimm/subsystem-profile.rst b/Documentation/nvdimm/subsystem-profile.rst
> new file mode 100644
> index 000000000000..d3428be7528e
> --- /dev/null
> +++ b/Documentation/nvdimm/subsystem-profile.rst
Hmm... would it make sense to add a pointer at maintainer/index.rst (or to some
other .rst file) for those profiles too?
> @@ -0,0 +1,86 @@
> +LIBNVDIMM Subsystem Profile
> +===========================
> +
> +Overview
> +--------
A minor nitpick here: I would add a blank line after each topic/subtopic.
On some cases, Sphinx will do wrong without that blank line, and having
some places with that extra line and others without it sounds unbalanced
on my eyes ;-)
> +So, you have recently become a maintainer of the LIBNVDIMM subsystem,
> +condolences, it is a thankless job, here is the lay of the land. The git
My understanding that the main focus of this document is to help people to
submit patches to the subsystem.
With that in mind, I would never start the doc talking only to maintainers,
as developers will likely just stop reading it at the above paragraph.
> +tree, git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/, is
> +writable by all the individuals listed in LIBNVDIMM section of
> +MAINTAINERS. Access is granted per the typical kernel.org account
> +management policies. Two branches in that tree are regularly pulled into
> +-next, libnvdimm-for-next, and libnvdimm-fixes. The submit rate of
> +patches is low, usually enough for one person to handle. There is a
> +patchwork instance at
> +https://patchwork.kernel.org/project/linux-nvdimm/list/, and it
> +historically is only used for ingesting patches and collecting
> +ack/review tags, i.e. no expectation to update the patch state after it
> +has been dispositioned, or merged.
> +
> +The most sensitive code area is the ACPI DSM (Device Specific Method)
> +path. In addition to the general fragility of an ioctl() ABI the ACPI
> +DSM scheme allows any vendor to implement any command without any prior
> +review by the ACPI committee. For this reason the LIBNVDIMM system seeks
> +to constrain the proliferation of vendor commands and at a minimum
> +requires any command support to be publicly documented. Over time the
> +submission rate of new vendor-specific commands is falling as more
> +commands are defined with named methods in the official ACPI
> +specification.
As Jani pointed, all the above stuff is for maintainers, but several other
stuff on this document are for developers. The best would likely to have
two separate files.
However, maintaining it on two separate files could be painful. Maybe
we could have an specific section, at the end of the document, with
maintainers-specific instructions.
> +
> +LIBNVDIMM sits at the intersection of device-drivers, the block-layer,
> +core memory-management, and filesystems. Be sure to re-route memory
> +management patches to the -mm tree, and otherwise pull-in fs-devel for
> +patches that touch anything related to DAX.
This is for developers, so it sounds OK!
> +
> +Core
> +----
> +F: drivers/nvdimm/\*_devs.c
> +F: drivers/acpi/nfit/\*.[ch]
> +
> +
> +Patches or Pull requests
> +------------------------
> +Patches only
> +
> +
> +Last day for new feature submissions
> +------------------------------------
> +Before -rc5
> +
> +
> +Last day to merge features
> +--------------------------
> +End of last -rc
> +
> +
> +Non-author Ack / Review Tags Required
> +-------------------------------------
> +Required
> +
> +
> +Test Suite
> +----------
> +Run ‘make check’ from https://github.com/pmem/ndctl
> +
> +
> +Trusted Reviewers
> +-----------------
> +Johannes Thumshirn
> +Toshi Kani
> +Jeff Moyer
> +Robert Elliott
See my other email commenting about that.
> +
> +
> +Resubmit Cadence
> +----------------
> +8 business days
> +
> +
> +Time Zone / Office Hours
> +------------------------
> +8:00am to 5:00pm Pacific Time Zone
> +
> +
> +Checkpatch / Style cleanups
> +---------------------------
> +Standalone style-cleanups are welcome.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bb4a83a7684d..ba2beedd4605 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8439,6 +8439,7 @@ M: Dan Williams <[email protected]>
> M: Vishal Verma <[email protected]>
> M: Dave Jiang <[email protected]>
> L: [email protected]
> +P: Documentation/nvdimm/subsystem-profile.rst
> Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
> S: Supported
> F: drivers/nvdimm/blk.c
> @@ -8450,6 +8451,7 @@ M: Dan Williams <[email protected]>
> M: Ross Zwisler <[email protected]>
> M: Dave Jiang <[email protected]>
> L: [email protected]
> +P: Documentation/nvdimm/subsystem-profile.rst
> Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
> S: Supported
> F: drivers/nvdimm/btt*
> @@ -8460,6 +8462,7 @@ M: Dan Williams <[email protected]>
> M: Vishal Verma <[email protected]>
> M: Dave Jiang <[email protected]>
> L: [email protected]
> +P: Documentation/nvdimm/subsystem-profile.rst
> Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
> S: Supported
> F: drivers/nvdimm/pmem*
> @@ -8478,6 +8481,7 @@ M: Ross Zwisler <[email protected]>
> M: Vishal Verma <[email protected]>
> M: Dave Jiang <[email protected]>
> L: [email protected]
> +P: Documentation/nvdimm/subsystem-profile.rst
> Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
> T: git git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git
> S: Supported
>
> _______________________________________________
> Ksummit-discuss mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss
Cheers,
Mauro
On Thu, 15 Nov 2018, Mauro Carvalho Chehab wrote:
> Em Wed, 14 Nov 2018 20:53:30 -0800
> Dan Williams <[email protected]> escreveu:
>
> > Document the basic policies of the libnvdimm subsystem and provide a
> > first example of a Subsystem Profile for others to duplicate and edit.
> >
> > Cc: Ross Zwisler <[email protected]>
> > Cc: Vishal Verma <[email protected]>
> > Cc: Dave Jiang <[email protected]>
> > Signed-off-by: Dan Williams <[email protected]>
> > ---
> > Documentation/nvdimm/subsystem-profile.rst | 86 ++++++++++++++++++++++++++++
> > MAINTAINERS | 4 +
> > 2 files changed, 90 insertions(+)
> > create mode 100644 Documentation/nvdimm/subsystem-profile.rst
> >
> > diff --git a/Documentation/nvdimm/subsystem-profile.rst b/Documentation/nvdimm/subsystem-profile.rst
> > new file mode 100644
> > index 000000000000..d3428be7528e
> > --- /dev/null
> > +++ b/Documentation/nvdimm/subsystem-profile.rst
>
> Hmm... would it make sense to add a pointer at maintainer/index.rst (or to some
> other .rst file) for those profiles too?
>
> > @@ -0,0 +1,86 @@
> > +LIBNVDIMM Subsystem Profile
> > +===========================
> > +
> > +Overview
> > +--------
>
> A minor nitpick here: I would add a blank line after each topic/subtopic.
>
> On some cases, Sphinx will do wrong without that blank line, and having
> some places with that extra line and others without it sounds unbalanced
> on my eyes ;-)
>
> > +So, you have recently become a maintainer of the LIBNVDIMM subsystem,
> > +condolences, it is a thankless job, here is the lay of the land. The git
>
> My understanding that the main focus of this document is to help people to
> submit patches to the subsystem.
>
> With that in mind, I would never start the doc talking only to maintainers,
> as developers will likely just stop reading it at the above paragraph.
This seems like a good idea. New maintainers will probably be directed to
this document by existing maintainers, so they will already have some
context. On the other hand, developers may interact with it on their own,
so it is good that they know immediately that they are in the right place.
julia
>
> > +tree, git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/, is
> > +writable by all the individuals listed in LIBNVDIMM section of
> > +MAINTAINERS. Access is granted per the typical kernel.org account
> > +management policies. Two branches in that tree are regularly pulled into
> > +-next, libnvdimm-for-next, and libnvdimm-fixes. The submit rate of
> > +patches is low, usually enough for one person to handle. There is a
> > +patchwork instance at
> > +https://patchwork.kernel.org/project/linux-nvdimm/list/, and it
> > +historically is only used for ingesting patches and collecting
> > +ack/review tags, i.e. no expectation to update the patch state after it
> > +has been dispositioned, or merged.
> > +
> > +The most sensitive code area is the ACPI DSM (Device Specific Method)
> > +path. In addition to the general fragility of an ioctl() ABI the ACPI
> > +DSM scheme allows any vendor to implement any command without any prior
> > +review by the ACPI committee. For this reason the LIBNVDIMM system seeks
> > +to constrain the proliferation of vendor commands and at a minimum
> > +requires any command support to be publicly documented. Over time the
> > +submission rate of new vendor-specific commands is falling as more
> > +commands are defined with named methods in the official ACPI
> > +specification.
>
> As Jani pointed, all the above stuff is for maintainers, but several other
> stuff on this document are for developers. The best would likely to have
> two separate files.
>
> However, maintaining it on two separate files could be painful. Maybe
> we could have an specific section, at the end of the document, with
> maintainers-specific instructions.
>
> > +
> > +LIBNVDIMM sits at the intersection of device-drivers, the block-layer,
> > +core memory-management, and filesystems. Be sure to re-route memory
> > +management patches to the -mm tree, and otherwise pull-in fs-devel for
> > +patches that touch anything related to DAX.
>
> This is for developers, so it sounds OK!
>
> > +
> > +Core
> > +----
> > +F: drivers/nvdimm/\*_devs.c
> > +F: drivers/acpi/nfit/\*.[ch]
> > +
> > +
> > +Patches or Pull requests
> > +------------------------
> > +Patches only
> > +
> > +
> > +Last day for new feature submissions
> > +------------------------------------
> > +Before -rc5
> > +
> > +
> > +Last day to merge features
> > +--------------------------
> > +End of last -rc
> > +
> > +
> > +Non-author Ack / Review Tags Required
> > +-------------------------------------
> > +Required
> > +
> > +
> > +Test Suite
> > +----------
> > +Run ‘make check’ from https://github.com/pmem/ndctl
> > +
> > +
> > +Trusted Reviewers
> > +-----------------
> > +Johannes Thumshirn
> > +Toshi Kani
> > +Jeff Moyer
> > +Robert Elliott
>
> See my other email commenting about that.
>
> > +
> > +
> > +Resubmit Cadence
> > +----------------
> > +8 business days
> > +
> > +
> > +Time Zone / Office Hours
> > +------------------------
> > +8:00am to 5:00pm Pacific Time Zone
> > +
> > +
> > +Checkpatch / Style cleanups
> > +---------------------------
> > +Standalone style-cleanups are welcome.
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index bb4a83a7684d..ba2beedd4605 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -8439,6 +8439,7 @@ M: Dan Williams <[email protected]>
> > M: Vishal Verma <[email protected]>
> > M: Dave Jiang <[email protected]>
> > L: [email protected]
> > +P: Documentation/nvdimm/subsystem-profile.rst
> > Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
> > S: Supported
> > F: drivers/nvdimm/blk.c
> > @@ -8450,6 +8451,7 @@ M: Dan Williams <[email protected]>
> > M: Ross Zwisler <[email protected]>
> > M: Dave Jiang <[email protected]>
> > L: [email protected]
> > +P: Documentation/nvdimm/subsystem-profile.rst
> > Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
> > S: Supported
> > F: drivers/nvdimm/btt*
> > @@ -8460,6 +8462,7 @@ M: Dan Williams <[email protected]>
> > M: Vishal Verma <[email protected]>
> > M: Dave Jiang <[email protected]>
> > L: [email protected]
> > +P: Documentation/nvdimm/subsystem-profile.rst
> > Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
> > S: Supported
> > F: drivers/nvdimm/pmem*
> > @@ -8478,6 +8481,7 @@ M: Ross Zwisler <[email protected]>
> > M: Vishal Verma <[email protected]>
> > M: Dave Jiang <[email protected]>
> > L: [email protected]
> > +P: Documentation/nvdimm/subsystem-profile.rst
> > Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
> > T: git git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git
> > S: Supported
> >
> > _______________________________________________
> > Ksummit-discuss mailing list
> > [email protected]
> > https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss
>
>
>
>
> Cheers,
> Mauro
> _______________________________________________
> Ksummit-discuss mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss
>
Em Wed, 14 Nov 2018 20:53:25 -0800
Dan Williams <[email protected]> escreveu:
> As presented at the 2018 Linux Plumbers conference [1], the Subsystem
> Profile is proposed as a way to reduce friction between committers and
> maintainers and perhaps encourage conversations amongst maintainers
> about best practice policies.
>
> The profile contains short answers to some of the common policy
> questions a contributor might have, or that a maintainer might consider
> formalizing. The current list of maintenance policies is:
>
> Overview: General introduction to maintaining the subsystem
> Core: List of source files considered core
> Leaf: List of source files that consume core functionality
> Patches or Pull requests: Simple statement of expected submission format
> Last -rc for new feature submissions: Expected lead time for submissions
> Last -rc to merge features: Deadline for merge decisions
> Non-author Ack / Review Tags Required: Patch review economics
> Test Suite: Pass this suite before requesting inclusion
> Resubmit Cadence: When to ping the maintainer
> Trusted Reviewers: Help for triaging patches
> Time Zone / Office Hours: When might a maintainer be available
> Checkpatch / Style Cleanups: Policy on pure cleanup patches
> Off-list review: Request for review gates
> TODO: Potential development tasks up for grabs, or active focus areas
>
> The goal of the Subsystem Profile is to set expectations for
> contributors and interim or replacement maintainers for a subsystem.
>
> See Documentation/maintainer/subsystem-profile.rst for more details, and
> a follow-on example profile for the libnvdimm subsystem.
>
> [1]: https://linuxplumbersconf.org/event/2/contributions/59/
>
> Cc: Jonathan Corbet <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Steve French <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Cc: Tobin C. Harding <[email protected]>
> Cc: Olof Johansson <[email protected]>
> Cc: Martin K. Petersen <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: Joe Perches <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
Ok, I sort of followed the proposed model, adding a documentation for
the media subsystem using the template.
I'm pretty sure this is incomplete and more work would be needed.
So, for now, this is just an example.
Also, I didn't test building it on Sphinx (nor added any reference for it
at MAINTAINERS), as my main goal here was to see how the model would fit
for us.
I noticed a few issues there:
1) Describing the "core" files. The media subsystem is complex: depending on
the device type, we have different APIs and different cores. So, our core
is actually a set of different cores. I ended by adding sub-sections.
Also, several things were not described there. For example, we store
DVB frontend drivers on an specific directory. We have drivers there organized
by bus type. So, one directory for I2C, one for USB, one for PCI, ...
It should probably make sense to be able to tell about that.
So, I would actually prefer to have, at the profile, a "files" section,
instead of "core".
2) As I said before, on media, we have sub-maintainers. Not sure how to
describe them on this template.
3) I noticed one big missing thing there: in the case of media, we are
responsible also to maintaining media staging drivers, that goes at
drivers/staging/media. IMO, it would be important to be able to describe
who maintains staging stuff - e. g. if the subsystem maintainers prefer
to let staging up to staging maintainers or if they'll maintain themselves.
In the latter, it probably makes sense to describe any specific thing
related to it (where staging will be at the tree, are there any special
rules for them?)
Anyway, RFC patch follows.
-
[PATCH] [RFC] Add a system profile description for media subsystem
This RFC aligns with current Dan's proposal for having subsystem
specific ruleset stored at the Kernel tree.
On this initial RFC, I opted to not add the reviewers e-mail
(adding just a "<>") as a boilerplate. If we decide keeping emails
there, I'll add them.
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
diff --git a/Documentation/media/subsystem-profile.rst b/Documentation/media/subsystem-profile.rst
new file mode 100644
index 000000000000..7a5d6f691d05
--- /dev/null
+++ b/Documentation/media/subsystem-profile.rst
@@ -0,0 +1,186 @@
+Media Subsystem Profile
+=======================
+
+Overview
+--------
+
+The media subsystem cover support for a variety of devices: stream
+capture, analog and digital TV, cameras, remote controllers, HDMI CEC
+and media pipeline control.
+
+Both our userspace and Kernel APIs are documented and should be kept in
+sync with the API changes. It means that all patches that add new
+features to the subsystem should also bring changes to the corresponding
+API files.
+
+Also, patches for device drivers that changes the Open Firmware/Device
+Tree bindings should be reviewed by the Device Tree maintainers.
+
+Due to the size and wide scope of the media subsystem, our
+maintainership model is to have sub-maintainers that have a broad
+knowledge of an specific aspect of the subsystem. It is a
+sub-maintainers task to review the patches, providing feedback to users
+if the patches are following the subsystem rules and are properly using
+the media internal and external APIs.
+
+We have a set of compliance tools at https://git.linuxtv.org/v4l-utils.git/
+that should be used in order to check if the drivers are properly
+implementing the media APIs.
+
+Patches for the media subsystem should be sent to the media mailing list
+at [email protected] as plain text only e-mail. emails with
+HTML will be automacially rejected by the mail server.
+
+Our workflow is heavily based on Patchwork, meaning that, once a patch
+is submitted, it should appear at:
+
+ - https://patchwork.linuxtv.org/project/linux-media/list/
+
+If it doesn't automatically appear there after a few minutes, then
+probably something got wrong on your submission. Please check if the
+email is in plain text only and if your emailer is not mangling with
+whitespaces before complaining or submit it again.
+
+Core
+----
+
+Documentation
++++++++++++++
+
+F: Documentation/media
+
+Kernelspace API headers
++++++++++++++++++++++++
+
+F: include/media/*.h
+
+Digital TV Core
++++++++++++++++
+
+F: drivers/media/dvb-core
+
+HDMI CEC Core
++++++++++++++
+
+F: drivers/media/cec
+
+Media Controller Core
++++++++++++++++++++++
+
+F: drivers/media/media-\*.[ch]
+
+Remote Controller Core
+++++++++++++++++++++++
+
+F: drivers/media/rc/rc-core-priv.h
+F: drivers/media/rc/rc-ir-raw.c
+F: drivers/media/rc/rc-main.c
+F: drivers/media/rc/ir\*-decoder.c
+F: drivers/media/rc/lirc_dev.c
+
+Video4linux Core
+++++++++++++++++
+
+F: drivers/media/v4l2-core
+
+Patches or Pull requests
+------------------------
+
+All patches should be submitted via e-mail for review. We use
+pull requests on our workflow between sub-maintainers and the
+maintainer.
+
+
+Last day for new feature submissions
+------------------------------------
+
+Before -rc5
+
+
+Last day to merge features
+--------------------------
+
+Before -rc7
+
+
+Non-author Ack / Review Tags Required
+-------------------------------------
+
+Not required, but desirable
+
+
+Test Suite
+----------
+
+Use the several *-compliance tools that are part of the v4l-utils
+package.
+
+
+Trusted Reviewers
+-----------------
+
+Sub-maintainers
++++++++++++++++
+
+At the media subsystem, we have a group of senior developers that are
+responsible for doing the code reviews at the drivers (called
+sub-maintainers), and another senior developer responsible for the
+subsystem as a hole. For core changes, whenever possible, multiple
+media (sub-)maintainers do the review.
+
+The sub-maintainers work on specific areas of the subsystem, as
+described below:
+
+- Sensor drivers
+
+ R: Sakari Ailus <>
+
+- V4L2 drivers
+
+ R: Hans Verkuil <>
+
+- Media controller drivers
+
+ R: Laurent Pinchart <>
+
+- HDMI CEC
+
+- Remote Controllers
+
+ R: Sean Young <>
+
+- Digital TV
+
+ R: Michael Krufky <>
+ R: Sean Young <>
+
+
+Resubmit Cadence
+----------------
+
+Provided that your patch is at https://patchwork.linuxtv.org, it should
+be sooner or later handled, so you don't need to re-submit a patch.
+
+Please notice that the media subsystem is a high traffic one, so it
+could take a while for us to be able to review your patches. Feel free
+to ping if you don't get a feedback on a couple of weeks.
+
+Time Zone / Office Hours
+------------------------
+
+Media developers are distributed all around the globe. So, don't assume
+that we're on your time zone. We usually don't work on local holidays or
+at weekends. Please also notice that, during the Kernel merge window,
+we're usually busy ensuring that everything goes smoothly, meaning that
+we usually have a lot of patches waiting for review just after that. So
+you should expect a higher delay during the merge window and one week
+before/after it.
+
+
+Checkpatch / Style cleanups
+---------------------------
+
+Standalone style-cleanups are welcome, but they should be grouped per
+directory. So, for example, if you're doing a cleanup at drivers
+under drivers/media, please send a single patch for all drivers under
+drivers/media/pci, another one for drivers/media/usb and so on.
Cheers,
Mauro
On Thu, Nov 15, 2018 at 06:10:36AM -0800, Mauro Carvalho Chehab wrote:
> Em Thu, 15 Nov 2018 09:03:11 +0100
> Geert Uytterhoeven <[email protected]> escreveu:
>
> > Hi Dan,
> >
> > On Thu, Nov 15, 2018 at 6:06 AM Dan Williams <[email protected]> wrote:
> > > Document the basic policies of the libnvdimm subsystem and provide a
> > > first example of a Subsystem Profile for others to duplicate and edit.
> > >
> > > Cc: Ross Zwisler <[email protected]>
> > > Cc: Vishal Verma <[email protected]>
> > > Cc: Dave Jiang <[email protected]>
> > > Signed-off-by: Dan Williams <[email protected]>
> >
> > Thanks for your patch!
> >
> > > --- /dev/null
> > > +++ b/Documentation/nvdimm/subsystem-profile.rst
> >
> > > +Trusted Reviewers
> > > +-----------------
> > > +Johannes Thumshirn
> > > +Toshi Kani
> > > +Jeff Moyer
> > > +Robert Elliott
> >
> > Don't you want to add email addresses?
> > Only the first one is listed in MAINTAINERS.
>
> IMO, it makes sense to have their e-mails here, in a way that it could
> easily be parsed by get_maintainers.pl.
I personally think that list of "trusted reviewers" makes more harm than
good. It creates unneeded negative feelings to those who wanted to be in
this list, but for any reason they don't. Those reviewers will feel
"untrusted".
Thanks
> -----Original Message-----
> From: Jani Nikula on Thursday, November 15, 2018 12:39 AM
>
> Cc: linux-doc
>
> On Wed, 14 Nov 2018, Dan Williams <[email protected]> wrote:
> > As presented at the 2018 Linux Plumbers conference [1], the Subsystem
> > Profile is proposed as a way to reduce friction between committers and
> > maintainers and perhaps encourage conversations amongst maintainers
> > about best practice policies.
> >
> > The profile contains short answers to some of the common policy
> > questions a contributor might have, or that a maintainer might consider
> > formalizing. The current list of maintenance policies is:
> >
> > Overview: General introduction to maintaining the subsystem
> > Core: List of source files considered core
> > Leaf: List of source files that consume core functionality
> > Patches or Pull requests: Simple statement of expected submission format
> > Last -rc for new feature submissions: Expected lead time for submissions
> > Last -rc to merge features: Deadline for merge decisions
> > Non-author Ack / Review Tags Required: Patch review economics
> > Test Suite: Pass this suite before requesting inclusion
> > Resubmit Cadence: When to ping the maintainer
> > Trusted Reviewers: Help for triaging patches
> > Time Zone / Office Hours: When might a maintainer be available
> > Checkpatch / Style Cleanups: Policy on pure cleanup patches
> > Off-list review: Request for review gates
> > TODO: Potential development tasks up for grabs, or active focus areas
> >
> > The goal of the Subsystem Profile is to set expectations for
> > contributors and interim or replacement maintainers for a subsystem.
>
> First of all, I welcome documentation efforts like this.
>
> The cover letter mainly focuses on the maintainer aspect, and the
> documentation is added to the maintainer handbook. However, here you set
> the goal as setting expectations for contributors. The example nvdimm
> profile in patch 3/3 addresses the reader as a new maintainer, yet goes
> on to set expectations also for contributors, not just the maintainer.
>
> I do think the documentation for contributors and maintainers/committers
> should be kept separate. Most contributors will never care about the
> documentation for the latter. We have Documentation/process for
> contributors, and I think the audience of Documentation/maintainer
> should be strictly maintainers.
>
> In summary, I do think we need all of the documentation you propose, and
> I appreciate you taking this on, but I think this should be split by
> audience.
I think there is a spectrum of audiences for this document, including contributors,
reviewers, and maintainers. Some of this information documents the "social API" between
these groups. So, I think it's handy to have it in one place, viewable by all parties.
However, designating items that are of special interest to maintainers, or reviewers
or contributors is worthwhile. Maybe having sections in a single document would
serve that purpose?
Put another way, there are pieces of information that are an agreement between
maintainers and contributors, that both should probably be reminded of. Maintaining
such information in 2 places would be a pain, and prone to getting out-of-sync.
Also,
I think over time we want to contributors to feel comfortable becoming reviewers
and maintainers, so it's good for them to learn about the processes that the
maintainers are using. I fear if we kept the information
in role-specific documents, it wouldn't promote this progression.
-- Tim
Em Thu, 15 Nov 2018 18:20:08 +0200
Leon Romanovsky <[email protected]> escreveu:
> On Thu, Nov 15, 2018 at 06:10:36AM -0800, Mauro Carvalho Chehab wrote:
> > Em Thu, 15 Nov 2018 09:03:11 +0100
> > Geert Uytterhoeven <[email protected]> escreveu:
> >
> > > Hi Dan,
> > >
> > > On Thu, Nov 15, 2018 at 6:06 AM Dan Williams <[email protected]> wrote:
> > > > Document the basic policies of the libnvdimm subsystem and provide a
> > > > first example of a Subsystem Profile for others to duplicate and edit.
> > > >
> > > > Cc: Ross Zwisler <[email protected]>
> > > > Cc: Vishal Verma <[email protected]>
> > > > Cc: Dave Jiang <[email protected]>
> > > > Signed-off-by: Dan Williams <[email protected]>
> > >
> > > Thanks for your patch!
> > >
> > > > --- /dev/null
> > > > +++ b/Documentation/nvdimm/subsystem-profile.rst
> > >
> > > > +Trusted Reviewers
> > > > +-----------------
> > > > +Johannes Thumshirn
> > > > +Toshi Kani
> > > > +Jeff Moyer
> > > > +Robert Elliott
> > >
> > > Don't you want to add email addresses?
> > > Only the first one is listed in MAINTAINERS.
> >
> > IMO, it makes sense to have their e-mails here, in a way that it could
> > easily be parsed by get_maintainers.pl.
>
> I personally think that list of "trusted reviewers" makes more harm than
> good. It creates unneeded negative feelings to those who wanted to be in
> this list, but for any reason they don't. Those reviewers will feel
> "untrusted".
Yeah, perhaps something like "most active reviewers" would sound
better.
Cheers,
Mauro
On Thu, Nov 15, 2018 at 11:09:34AM -0800, Mauro Carvalho Chehab wrote:
> Em Thu, 15 Nov 2018 18:20:08 +0200
> Leon Romanovsky <[email protected]> escreveu:
>
> > On Thu, Nov 15, 2018 at 06:10:36AM -0800, Mauro Carvalho Chehab wrote:
> > > Em Thu, 15 Nov 2018 09:03:11 +0100
> > > Geert Uytterhoeven <[email protected]> escreveu:
> > >
> > > > Hi Dan,
> > > >
> > > > On Thu, Nov 15, 2018 at 6:06 AM Dan Williams <[email protected]> wrote:
> > > > > Document the basic policies of the libnvdimm subsystem and provide a
> > > > > first example of a Subsystem Profile for others to duplicate and edit.
> > > > >
> > > > > Cc: Ross Zwisler <[email protected]>
> > > > > Cc: Vishal Verma <[email protected]>
> > > > > Cc: Dave Jiang <[email protected]>
> > > > > Signed-off-by: Dan Williams <[email protected]>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > --- /dev/null
> > > > > +++ b/Documentation/nvdimm/subsystem-profile.rst
> > > >
> > > > > +Trusted Reviewers
> > > > > +-----------------
> > > > > +Johannes Thumshirn
> > > > > +Toshi Kani
> > > > > +Jeff Moyer
> > > > > +Robert Elliott
> > > >
> > > > Don't you want to add email addresses?
> > > > Only the first one is listed in MAINTAINERS.
> > >
> > > IMO, it makes sense to have their e-mails here, in a way that it could
> > > easily be parsed by get_maintainers.pl.
> >
> > I personally think that list of "trusted reviewers" makes more harm than
> > good. It creates unneeded negative feelings to those who wanted to be in
> > this list, but for any reason they don't. Those reviewers will feel
> > "untrusted".
>
> Yeah, perhaps something like "most active reviewers" would sound
> better.
I would recommend to remove this section at all.
New maintainers won't come out of blue, but will be come
from existing community and such individuals for sure will see
and judge by themselves to whom they trust and to whom not.
Thanks
>
> Cheers,
> Mauro
> I would recommend to remove this section at all.
> New maintainers won't come out of blue, but will be come
> from existing community and such individuals for sure will see
> and judge by themselves to whom they trust and to whom not.
Perhaps this is more of a hint to contributors than to maintainers
(see earlier discussion on who is the target audience for these documents).
It would help contributors know some names of useful reviewers (and
thus this list should be picked up by scripts/get_maintainer.pl to help
the user compose Cc: lists for e-mail patches).
-Tony
On Thu, 2018-11-15 at 19:40 +0000, Luck, Tony wrote:
> > I would recommend to remove this section at all.
> > New maintainers won't come out of blue, but will be come
> > from existing community and such individuals for sure will see
> > and judge by themselves to whom they trust and to whom not.
>
> Perhaps this is more of a hint to contributors than to maintainers
> (see earlier discussion on who is the target audience for these documents).
>
> It would help contributors know some names of useful reviewers (and
> thus this list should be picked up by scripts/get_maintainer.pl to help
> the user compose Cc: lists for e-mail patches).
Trusted reviewers should be specifically listed
in the MAINTAINERS file with an "R:" entry.
get_maintainers should not look anywhere else.
On Wed, 2018-11-14 at 21:39 -0800, Mauro Carvalho Chehab wrote:
> Em Wed, 14 Nov 2018 20:53:19 -0800
> Dan Williams <[email protected]> escreveu:
>
> > Fixup some P: entries to be M: and rename the remaining ones to 'E:' for
> > "entity". The P: tag will be used to indicate the location of a
> > Subsystem Profile for a given MAINTAINERS entry.
> >
> > Cc: Joe Perches <[email protected]>
> > Signed-off-by: Dan Williams <[email protected]>
> > ---
> > MAINTAINERS | 18 +++++++++---------
> > 1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 0abecc528dac..83b7b3943a12 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -76,7 +76,7 @@ trivial patch so apply some common sense.
> >
> > Descriptions of section entries:
> >
> > - P: Person (obsolete)
> > + E: Entity (obsolete)
>
> I don't like very much the idea of renaming it, but that's just my 2 cents.
I'd prefer just removing all the "P:" entries.
[]
> > @@ -13577,15 +13577,15 @@ F: drivers/video/fbdev/simplefb.c
> > F: include/linux/platform_data/simplefb.h
> > SIMTEC EB110ATX (Chalice CATS)
> > -P: Ben Dooks
> > -P: Vincent Sanders <[email protected]>
> > +E: Ben Dooks
> > +M: Vincent Sanders <[email protected]>
> > M: Simtec Linux Team <[email protected]>
> > W: http://www.simtec.co.uk/products/EB110ATX/
> > S: Supported
This is an ancient StrongARM based board that is shown as
discontinued on Simtec's website.
This is very obsolete and it's probably better to remove it.
> > SIMTEC EB2410ITX (BAST)
> > -P: Ben Dooks
> > -P: Vincent Sanders <[email protected]>
> > +E: Ben Dooks
> > +M: Vincent Sanders <[email protected]>
> > M: Simtec Linux Team <[email protected]>
> > W: http://www.simtec.co.uk/products/EB2410ITX/
> > S: Supported
[]
> So, I guess this information is duplicated/obsoleted and could just
> be removed.
This one is still listed as a production board on Simtec's website.
http://www.simtec.co.uk/products/EB2410ITX/
But I expect these 2410 boards aren't sold in any volume.
On Thu, Nov 15, 2018 at 10:38:44AM +0200, Jani Nikula wrote:
>
> Cc: linux-doc
>
> On Wed, 14 Nov 2018, Dan Williams <[email protected]> wrote:
> > As presented at the 2018 Linux Plumbers conference [1], the Subsystem
> > Profile is proposed as a way to reduce friction between committers and
> > maintainers and perhaps encourage conversations amongst maintainers
> > about best practice policies.
> >
> > The profile contains short answers to some of the common policy
> > questions a contributor might have, or that a maintainer might consider
> > formalizing. The current list of maintenance policies is:
> >
> > Overview: General introduction to maintaining the subsystem
> > Core: List of source files considered core
> > Leaf: List of source files that consume core functionality
> > Patches or Pull requests: Simple statement of expected submission format
> > Last -rc for new feature submissions: Expected lead time for submissions
> > Last -rc to merge features: Deadline for merge decisions
> > Non-author Ack / Review Tags Required: Patch review economics
> > Test Suite: Pass this suite before requesting inclusion
> > Resubmit Cadence: When to ping the maintainer
> > Trusted Reviewers: Help for triaging patches
> > Time Zone / Office Hours: When might a maintainer be available
> > Checkpatch / Style Cleanups: Policy on pure cleanup patches
> > Off-list review: Request for review gates
> > TODO: Potential development tasks up for grabs, or active focus areas
> >
> > The goal of the Subsystem Profile is to set expectations for
> > contributors and interim or replacement maintainers for a subsystem.
>
> First of all, I welcome documentation efforts like this.
>
> The cover letter mainly focuses on the maintainer aspect, and the
> documentation is added to the maintainer handbook. However, here you set
> the goal as setting expectations for contributors. The example nvdimm
> profile in patch 3/3 addresses the reader as a new maintainer, yet goes
> on to set expectations also for contributors, not just the maintainer.
>
> I do think the documentation for contributors and maintainers/committers
> should be kept separate. Most contributors will never care about the
> documentation for the latter. We have Documentation/process for
> contributors, and I think the audience of Documentation/maintainer
> should be strictly maintainers.
>
> In summary, I do think we need all of the documentation you propose, and
> I appreciate you taking this on, but I think this should be split by
> audience.
I got confused by this at first also Jani. This document is a template
for use by maintainers. The files created from the template (by a
subsystem maintainer) are for contributors. So I believe this document
is in the correct place.
Hope this helps to clarify.
Tobin
On 11/14/18 8:53 PM, Dan Williams wrote:
> As presented at the 2018 Linux Plumbers conference [1], the Subsystem
> Profile is proposed as a way to reduce friction between committers and
> maintainers and perhaps encourage conversations amongst maintainers
> about best practice policies.
Thanks for working on this.
> The profile contains short answers to some of the common policy
> questions a contributor might have, or that a maintainer might consider
> formalizing. The current list of maintenance policies is:
>
> Overview: General introduction to maintaining the subsystem
> Core: List of source files considered core
> Leaf: List of source files that consume core functionality
> Patches or Pull requests: Simple statement of expected submission format
> Last -rc for new feature submissions: Expected lead time for submissions
> Last -rc to merge features: Deadline for merge decisions
> Non-author Ack / Review Tags Required: Patch review economics
> Test Suite: Pass this suite before requesting inclusion
> Resubmit Cadence: When to ping the maintainer
> Trusted Reviewers: Help for triaging patches
> Time Zone / Office Hours: When might a maintainer be available
> Checkpatch / Style Cleanups: Policy on pure cleanup patches
> Off-list review: Request for review gates
> TODO: Potential development tasks up for grabs, or active focus areas
>
> The goal of the Subsystem Profile is to set expectations for
> contributors and interim or replacement maintainers for a subsystem.
>
> See Documentation/maintainer/subsystem-profile.rst for more details, and
> a follow-on example profile for the libnvdimm subsystem.
>
> [1]: https://linuxplumbersconf.org/event/2/contributions/59/
>
> Cc: Jonathan Corbet <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Steve French <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Cc: Tobin C. Harding <[email protected]>
> Cc: Olof Johansson <[email protected]>
> Cc: Martin K. Petersen <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: Joe Perches <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> Documentation/maintainer/index.rst | 1
> Documentation/maintainer/subsystem-profile.rst | 145 ++++++++++++++++++++++++
> MAINTAINERS | 4 +
> 3 files changed, 150 insertions(+)
> create mode 100644 Documentation/maintainer/subsystem-profile.rst
>
> diff --git a/Documentation/maintainer/index.rst b/Documentation/maintainer/index.rst
> index 2a14916930cb..1e6b1aaa6024 100644
> --- a/Documentation/maintainer/index.rst
> +++ b/Documentation/maintainer/index.rst
> @@ -11,4 +11,5 @@ additions to this manual.
>
> configure-git
> pull-requests
> + subsystem-profile
>
> diff --git a/Documentation/maintainer/subsystem-profile.rst b/Documentation/maintainer/subsystem-profile.rst
> new file mode 100644
> index 000000000000..a74b624e0972
> --- /dev/null
> +++ b/Documentation/maintainer/subsystem-profile.rst
> @@ -0,0 +1,145 @@
> +.. _subsystemprofile:
> +
> +Subsystem Profile
> +=================
> +
> +The Subsystem Profile is a collection of policy positions that a
> +maintainer or maintainer team establishes for the their subsystem. While
> +there is a wide range of technical nuance on maintaining disparate
> +sections of the kernel, the Subsystem Profile documents a known set of
> +major process policies that vary between subsystems. What follows is a
> +list of policy questions a maintainer can answer and include a document
> +in the kernel, or on an external website. It advertises to other
> +maintainers and contributors the local policy of the subsystem. Some
> +sections are optional like "Overview", "Off-list review", and "TODO".
> +The others are recommended for all subsystems to address, but no section
> +is mandatory. In addition there are no wrong answers, just document how
> +a subsystem typically operates. Note that the profile follows the
> +subsystem not the maintainer, i.e. there is no expectation that a
> +maintainer of multiple subsystems deploys the same policy across those
> +subsystems.
> +
> +
> +Overview
> +--------
> +In this optional section of the profile provide a free form overview of
> +the subsystem written as a hand-off document. In other words write a
> +note to someone that would receive the “keys to the castle” in the event
> +of extended or unexpected absence. “So, you have recently become the
> +maintainer of the XYZ subsystem, condolences, it is a thankless job,
> +here is the lay of the land.” Details to consider are the extended
> +details that are not included in MAINTAINERS, and not addressed by the
> +other profile questions below. For example details like, who has access
> +to the git tree, branches that are pulled into -next, relevant
> +specifications, issue trackers, and sensitive code areas. If available
> +the Overview should link to other subsystem documentation that may
> +clarify, re-iterate, emphasize / de-emphasize portions of the global
> +process documentation for contributors (CodingStyle, SubmittingPatches,
> +etc...).
> +
> +
> +Core
> +----
> +A list of F: tags (as described by MAINTAINERS) listing what the
> +maintainer considers to be core files. The review and lead time
> +constraints for 'core' code may be stricter given the increased
> +sensitivity and risk of change.
> +
> +
> +Patches or Pull requests
> +------------------------
> +Some subsystems allow contributors to send pull requests, most require
> +mailed patches. State “Patches only”, or “Pull requests accepted”.
This may create some confusion if only "Pull requests accepted" is the
contents of this section. My understanding has been that all changes
should be available on a mail list for review before being accepted
by the maintainer, even if eventually the final version of the series
that is accepted is applied by the maintainer as a pull instead of
via applying patches.
Is my "must appear on a mail list" understanding correct?
> +
> +
> +Last -rc for new feature submissions
> +------------------------------------
> +New feature submissions targeting the next merge window should have
> +their first posting for consideration before this point. Patches that
> +are submitted after this point should be clear that they are targeting
> +the NEXT+1 merge window, or should come with sufficient justification
> +why they should be considered on an expedited schedule. A general
> +guideline is to set expectation with contributors that new feature
> +submissions should appear before -rc5. The answer may be different for
> +'Core:' files, include a second entry prefixed with 'Core:' if so.
I would expect the -rc to vary based on the patch series size, complexity,
risk, number of revisions that will occur, how controversial, number of
-rc releases before Linus chooses to release, etc. You would need a
crystal ball to predict much of this. I could see being able to provide
a good estimate of this value for a relatively simple patch.
> +
> +
> +Last -rc to merge features
> +--------------------------
> +Indicate to contributors the point at which an as yet un-applied patch
> +set will need to wait for the NEXT+1 merge window. Of course there is no
> +obligation to ever except any given patchset, but if the review has not
> +concluded by this point the expectation the contributor should wait and
> +resubmit for the following merge window. The answer may be different for
> +'Core:' files, include a second entry prefixed with 'Core:' if so.
Same comment as for the previous section, with the additional complexity
that each unique patch series should bake in -next.
> +
> +
> +Non-author Ack / Review Tags Required
> +-------------------------------------
The title of this section seems a bit different than the description
below. A more aligned title would be something like "Maintainer
self-commit review requirements".
> +Let contributors and other maintainers know whether they can expect to
> +see the maintainer self-commit patches without 3rd-party review. Some
> +subsystem developer communities are so small as to make this requirement
> +impractical. Others may have been bootstrapped by a submission of
> +self-reviewed code at the outset, but have since moved to a
> +non-author review-required stance. This section sets expectations on the
> +code-review economics in the subsystem. For example, can a contributor
> +trade review of a maintainer's, or other contributor's patches in
> +exchange for consideration of their own.
Then another section (which is the one I expected when I slightly
mis-read the section title) would be: Review Tags Requirements",
which would discuss what type of review is expected, encouraged,
or required.
> +
> +
> +Test Suite
> +----------
> +Indicate the test suite all patches are expected to pass before being
> +submitted for inclusion consideration.
> +
> +
> +Resubmit Cadence
> +----------------
> +Define a rate at which a contributor should wait to resubmit a patchset
> +that has not yet received comments. A general guideline is to try to
> +meet a deadline of 1 - 2 weeks to acknowledge starting consideration for
> +a patch set.
Or ping instead of resubmitting? If you resubmit the same series with
an unchanged version then comments could be split across multiple
email threads. If you resubmit the same series with a new version
number, the same comment split can occur plus it can be confusing
that two versions have the same contents. I suspect that there may be
some maintainers who do prefer a resubmit, but that is just wild
conjecture on my part.
> +
> +
> +Trusted Reviewers
> +-----------------
> +While a maintainer / maintainer-team is expected to be reviewer of last
> +resort the review load is less onerous when distributed amongst
> +contributors and or a trusted set of individuals. This section is
> +distinct from the R: tag (Designated Reviewer). Whereas R: identifies
> +reviewers that should always be copied on a patch submission, the
> +trusted reviewers here are individuals contributors can reach out to if
> +a few 'Resubmit Cadence' intervals have gone by without maintainer
> +action, or to otherwise consult for advice.
This seems redundant with the MAINTAINERS reviewers list. It seems like
the role specified in this section is more of an ombudsman or developer
advocate who can assist with the review and/or accept flow if the
maintainer is being slow to respond.
> +
> +
> +Time Zone / Office Hours
> +------------------------
> +Let contributors know the time of day when one or more maintainers are
> +usually actively monitoring the mailing list.
I would strike "actively monitoring the mailing list". To me, it should
be what are the hours of the day that the maintainer might happen to poll
(or might receive an interrupt) from the appropriate communications
channels (could be IRC, could be email, etc).
For my area, I would want to say something like: I tend to be active
between 17:00 UTC (18:00 UTC when daylight savings) and 25:00 (26:00),
but often will check for urgent or brief items up until 07:00 (08:00).
I interact with email via a poll model. I interact with IRC via a
pull model and often overlook IRC activity for multiple days).
-Frank
> +
> +
> +Checkpatch / Style Cleanups
> +---------------------------
> +For subsystems with long standing code bases it is reasonable to decline
> +to accept pure coding-style fixup patches. This is where you can let
> +contributors know “Standalone style-cleanups are welcome”,
> +“Style-cleanups to existing code only welcome with other feature
> +changes”, or “Standalone style-cleanups to existing code are not
> +welcome”.
> +
> +
> +Off-list review
> +---------------
> +A maintainer may optionally require that contributors seek prior review
> +of patches before initial submission for upstream. For example,
> +“Developers from organization X, please seek internal review before
> +requesting upstream review”. This policy identifies occasions where a
> +maintainer wants to reflect some of the review load back to an
> +organization.
> +
> +
> +TODO
> +----
> +In this optional section include a list of work items that might be
> +suitable for onboarding a new developer to the subsystem.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 83b7b3943a12..bb4a83a7684d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -99,6 +99,10 @@ Descriptions of section entries:
> Obsolete: Old code. Something tagged obsolete generally means
> it has been replaced by a better system and you
> should be using that.
> + P: Subsystem Profile document for the maintainer entry. This
> + is either an in-tree file or a URI to a document. The
> + contents of a Subsystem Profile are described in
> + Documentation/maintainer/subsystem-profile.rst.
> F: Files and directories with wildcard patterns.
> A trailing slash includes all files and subdirectory files.
> F: drivers/net/ all files in and below drivers/net
>
> _______________________________________________
> Ksummit-discuss mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss
>
Hi, Dan-san,
Thank you for your patch. I have one comment.
> ---
> Documentation/nvdimm/subsystem-profile.rst | 86 ++++++++++++++++++++++++++++
> MAINTAINERS | 4 +
> 2 files changed, 90 insertions(+)
> create mode 100644 Documentation/nvdimm/subsystem-profile.rst
>
> diff --git a/Documentation/nvdimm/subsystem-profile.rst b/Documentation/nvdimm/subsystem-profile.rst
> new file mode 100644
> index 000000000000..d3428be7528e
> --- /dev/null
> +++ b/Documentation/nvdimm/subsystem-profile.rst
> @@ -0,0 +1,86 @@
> +LIBNVDIMM Subsystem Profile
> +===========================
> +
> +Overview
> +--------
> +So, you have recently become a maintainer of the LIBNVDIMM subsystem,
> +condolences, it is a thankless job, here is the lay of the land. The git
Here, I think that more positive sentence is desirable to encourage new comer.
Certainly, I can imagine that maintainer is hard and tough work,
but I believe it is great work.
Even if this is irony or joke, then it may be cause of miss-understanding
(especially, non-native English speaker like me.)
In addition, new coming developer is sometimes nervous
to post new mail/patch. If this text will be changed for new comer,
then I think this text will be good to encourage them.
So, I think more positive expression is desirable.
Thanks,
Em Thu, 15 Nov 2018 21:35:20 +0200
Leon Romanovsky <[email protected]> escreveu:
> On Thu, Nov 15, 2018 at 11:09:34AM -0800, Mauro Carvalho Chehab wrote:
> > Em Thu, 15 Nov 2018 18:20:08 +0200
> > Leon Romanovsky <[email protected]> escreveu:
> >
> > > On Thu, Nov 15, 2018 at 06:10:36AM -0800, Mauro Carvalho Chehab wrote:
> > > > Em Thu, 15 Nov 2018 09:03:11 +0100
> > > > Geert Uytterhoeven <[email protected]> escreveu:
> > > >
> > > > > Hi Dan,
> > > > >
> > > > > On Thu, Nov 15, 2018 at 6:06 AM Dan Williams <[email protected]> wrote:
> > > > > > Document the basic policies of the libnvdimm subsystem and provide a
> > > > > > first example of a Subsystem Profile for others to duplicate and edit.
> > > > > >
> > > > > > Cc: Ross Zwisler <[email protected]>
> > > > > > Cc: Vishal Verma <[email protected]>
> > > > > > Cc: Dave Jiang <[email protected]>
> > > > > > Signed-off-by: Dan Williams <[email protected]>
> > > > >
> > > > > Thanks for your patch!
> > > > >
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/nvdimm/subsystem-profile.rst
> > > > >
> > > > > > +Trusted Reviewers
> > > > > > +-----------------
> > > > > > +Johannes Thumshirn
> > > > > > +Toshi Kani
> > > > > > +Jeff Moyer
> > > > > > +Robert Elliott
> > > > >
> > > > > Don't you want to add email addresses?
> > > > > Only the first one is listed in MAINTAINERS.
> > > >
> > > > IMO, it makes sense to have their e-mails here, in a way that it could
> > > > easily be parsed by get_maintainers.pl.
> > >
> > > I personally think that list of "trusted reviewers" makes more harm than
> > > good. It creates unneeded negative feelings to those who wanted to be in
> > > this list, but for any reason they don't. Those reviewers will feel
> > > "untrusted".
> >
> > Yeah, perhaps something like "most active reviewers" would sound
> > better.
>
> I would recommend to remove this section at all.
> New maintainers won't come out of blue, but will be come
> from existing community and such individuals for sure will see
> and judge by themselves to whom they trust and to whom not.
I see your point, but, on the other hand, having a list with the ones
that are actively doing reviews helps newcomers.
I would keep, but perhaps it makes sense to add some notice about a
criteria about how to be included at the "active reviewers" list,
(the criteria probably belongs to the subsystem profile), e. g.
something like:
"Active reviewers are developers that contribute with more than 25
patches per year and do more than 50 reviews per year on
on patches written for drivers that they're not usual contributors"
Cheers,
Mauro
Em Thu, 15 Nov 2018 11:43:51 -0800
Joe Perches <[email protected]> escreveu:
> On Thu, 2018-11-15 at 19:40 +0000, Luck, Tony wrote:
> > > I would recommend to remove this section at all.
> > > New maintainers won't come out of blue, but will be come
> > > from existing community and such individuals for sure will see
> > > and judge by themselves to whom they trust and to whom not.
> >
> > Perhaps this is more of a hint to contributors than to maintainers
> > (see earlier discussion on who is the target audience for these documents).
> >
> > It would help contributors know some names of useful reviewers (and
> > thus this list should be picked up by scripts/get_maintainer.pl to help
> > the user compose Cc: lists for e-mail patches).
>
> Trusted reviewers should be specifically listed
> in the MAINTAINERS file with an "R:" entry.
>
> get_maintainers should not look anywhere else.
I know that making get_maintainers to look elsewhere can make it more
complex and slower, but IMHO, by having a per-subsystem profile, this is
unavoidable.
The thing is that touching at a single MAINTAINERS file every time a new
reviewer comes is painful. Also, MAINTAINERS file format doesn't allow
adding free text explaining the criteria for someone to become a
reviewer.
IMO, having reviewers on a per-subsystem file, where one could explain
the criteria for being added there will make easier to attract more
reviewers.
Cheers,
Mauro
On Fri 16-11-18 03:33:17, Mauro Carvalho Chehab wrote:
> Em Thu, 15 Nov 2018 21:35:20 +0200
> Leon Romanovsky <[email protected]> escreveu:
>
> > On Thu, Nov 15, 2018 at 11:09:34AM -0800, Mauro Carvalho Chehab wrote:
> > > Em Thu, 15 Nov 2018 18:20:08 +0200
> > > Leon Romanovsky <[email protected]> escreveu:
> > >
> > > > On Thu, Nov 15, 2018 at 06:10:36AM -0800, Mauro Carvalho Chehab wrote:
> > > > > Em Thu, 15 Nov 2018 09:03:11 +0100
> > > > > Geert Uytterhoeven <[email protected]> escreveu:
> > > > >
> > > > > > Hi Dan,
> > > > > >
> > > > > > On Thu, Nov 15, 2018 at 6:06 AM Dan Williams <[email protected]> wrote:
> > > > > > > Document the basic policies of the libnvdimm subsystem and provide a
> > > > > > > first example of a Subsystem Profile for others to duplicate and edit.
> > > > > > >
> > > > > > > Cc: Ross Zwisler <[email protected]>
> > > > > > > Cc: Vishal Verma <[email protected]>
> > > > > > > Cc: Dave Jiang <[email protected]>
> > > > > > > Signed-off-by: Dan Williams <[email protected]>
> > > > > >
> > > > > > Thanks for your patch!
> > > > > >
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/nvdimm/subsystem-profile.rst
> > > > > >
> > > > > > > +Trusted Reviewers
> > > > > > > +-----------------
> > > > > > > +Johannes Thumshirn
> > > > > > > +Toshi Kani
> > > > > > > +Jeff Moyer
> > > > > > > +Robert Elliott
> > > > > >
> > > > > > Don't you want to add email addresses?
> > > > > > Only the first one is listed in MAINTAINERS.
> > > > >
> > > > > IMO, it makes sense to have their e-mails here, in a way that it could
> > > > > easily be parsed by get_maintainers.pl.
> > > >
> > > > I personally think that list of "trusted reviewers" makes more harm than
> > > > good. It creates unneeded negative feelings to those who wanted to be in
> > > > this list, but for any reason they don't. Those reviewers will feel
> > > > "untrusted".
> > >
> > > Yeah, perhaps something like "most active reviewers" would sound
> > > better.
> >
> > I would recommend to remove this section at all.
> > New maintainers won't come out of blue, but will be come
> > from existing community and such individuals for sure will see
> > and judge by themselves to whom they trust and to whom not.
>
> I see your point, but, on the other hand, having a list with the ones
> that are actively doing reviews helps newcomers.
How exactly? Do you expect people to CC patches to these directly? And if
yes, why is not picking patches from the mailing list good enough?
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
Em Thu, 15 Nov 2018 16:11:59 -0800
Frank Rowand <[email protected]> escreveu:
> On 11/14/18 8:53 PM, Dan Williams wrote:
> > As presented at the 2018 Linux Plumbers conference [1], the Subsystem
> > Profile is proposed as a way to reduce friction between committers and
> > maintainers and perhaps encourage conversations amongst maintainers
> > about best practice policies.
>
> Thanks for working on this.
>
>
> > The profile contains short answers to some of the common policy
> > questions a contributor might have, or that a maintainer might consider
> > formalizing. The current list of maintenance policies is:
> >
> > Overview: General introduction to maintaining the subsystem
> > Core: List of source files considered core
> > Leaf: List of source files that consume core functionality
> > Patches or Pull requests: Simple statement of expected submission format
> > Last -rc for new feature submissions: Expected lead time for submissions
> > Last -rc to merge features: Deadline for merge decisions
> > Non-author Ack / Review Tags Required: Patch review economics
> > Test Suite: Pass this suite before requesting inclusion
> > Resubmit Cadence: When to ping the maintainer
> > Trusted Reviewers: Help for triaging patches
> > Time Zone / Office Hours: When might a maintainer be available
> > Checkpatch / Style Cleanups: Policy on pure cleanup patches
> > Off-list review: Request for review gates
> > TODO: Potential development tasks up for grabs, or active focus areas
> >
> > The goal of the Subsystem Profile is to set expectations for
> > contributors and interim or replacement maintainers for a subsystem.
> >
> > See Documentation/maintainer/subsystem-profile.rst for more details, and
> > a follow-on example profile for the libnvdimm subsystem.
> >
> > [1]: https://linuxplumbersconf.org/event/2/contributions/59/
> >
> > Cc: Jonathan Corbet <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Mauro Carvalho Chehab <[email protected]>
> > Cc: Steve French <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: Linus Torvalds <[email protected]>
> > Cc: Tobin C. Harding <[email protected]>
> > Cc: Olof Johansson <[email protected]>
> > Cc: Martin K. Petersen <[email protected]>
> > Cc: Daniel Vetter <[email protected]>
> > Cc: Joe Perches <[email protected]>
> > Cc: Dmitry Vyukov <[email protected]>
> > Signed-off-by: Dan Williams <[email protected]>
> > ---
> > Documentation/maintainer/index.rst | 1
> > Documentation/maintainer/subsystem-profile.rst | 145 ++++++++++++++++++++++++
> > MAINTAINERS | 4 +
> > 3 files changed, 150 insertions(+)
> > create mode 100644 Documentation/maintainer/subsystem-profile.rst
> >
> > diff --git a/Documentation/maintainer/index.rst b/Documentation/maintainer/index.rst
> > index 2a14916930cb..1e6b1aaa6024 100644
> > --- a/Documentation/maintainer/index.rst
> > +++ b/Documentation/maintainer/index.rst
> > @@ -11,4 +11,5 @@ additions to this manual.
> >
> > configure-git
> > pull-requests
> > + subsystem-profile
> >
> > diff --git a/Documentation/maintainer/subsystem-profile.rst b/Documentation/maintainer/subsystem-profile.rst
> > new file mode 100644
> > index 000000000000..a74b624e0972
> > --- /dev/null
> > +++ b/Documentation/maintainer/subsystem-profile.rst
> > @@ -0,0 +1,145 @@
> > +.. _subsystemprofile:
> > +
> > +Subsystem Profile
> > +=================
> > +
> > +The Subsystem Profile is a collection of policy positions that a
> > +maintainer or maintainer team establishes for the their subsystem. While
> > +there is a wide range of technical nuance on maintaining disparate
> > +sections of the kernel, the Subsystem Profile documents a known set of
> > +major process policies that vary between subsystems. What follows is a
> > +list of policy questions a maintainer can answer and include a document
> > +in the kernel, or on an external website. It advertises to other
> > +maintainers and contributors the local policy of the subsystem. Some
> > +sections are optional like "Overview", "Off-list review", and "TODO".
> > +The others are recommended for all subsystems to address, but no section
> > +is mandatory. In addition there are no wrong answers, just document how
> > +a subsystem typically operates. Note that the profile follows the
> > +subsystem not the maintainer, i.e. there is no expectation that a
> > +maintainer of multiple subsystems deploys the same policy across those
> > +subsystems.
> > +
> > +
> > +Overview
> > +--------
> > +In this optional section of the profile provide a free form overview of
> > +the subsystem written as a hand-off document. In other words write a
> > +note to someone that would receive the “keys to the castle” in the event
> > +of extended or unexpected absence. “So, you have recently become the
> > +maintainer of the XYZ subsystem, condolences, it is a thankless job,
> > +here is the lay of the land.” Details to consider are the extended
> > +details that are not included in MAINTAINERS, and not addressed by the
> > +other profile questions below. For example details like, who has access
> > +to the git tree, branches that are pulled into -next, relevant
> > +specifications, issue trackers, and sensitive code areas. If available
> > +the Overview should link to other subsystem documentation that may
> > +clarify, re-iterate, emphasize / de-emphasize portions of the global
> > +process documentation for contributors (CodingStyle, SubmittingPatches,
> > +etc...).
> > +
> > +
> > +Core
> > +----
> > +A list of F: tags (as described by MAINTAINERS) listing what the
> > +maintainer considers to be core files. The review and lead time
> > +constraints for 'core' code may be stricter given the increased
> > +sensitivity and risk of change.
> > +
> > +
> > +Patches or Pull requests
> > +------------------------
> > +Some subsystems allow contributors to send pull requests, most require
> > +mailed patches. State “Patches only”, or “Pull requests accepted”.
>
> This may create some confusion if only "Pull requests accepted" is the
> contents of this section. My understanding has been that all changes
> should be available on a mail list for review before being accepted
> by the maintainer, even if eventually the final version of the series
> that is accepted is applied by the maintainer as a pull instead of
> via applying patches.
>
> Is my "must appear on a mail list" understanding correct?
I guess this is true on almost all subsystems that accept pull requests.
It could not be true if some subsystem moves to Github/Gitlab.
> > +
> > +
> > +Last -rc for new feature submissions
> > +------------------------------------
> > +New feature submissions targeting the next merge window should have
> > +their first posting for consideration before this point. Patches that
> > +are submitted after this point should be clear that they are targeting
> > +the NEXT+1 merge window, or should come with sufficient justification
> > +why they should be considered on an expedited schedule. A general
> > +guideline is to set expectation with contributors that new feature
> > +submissions should appear before -rc5. The answer may be different for
> > +'Core:' files, include a second entry prefixed with 'Core:' if so.
>
> I would expect the -rc to vary based on the patch series size, complexity,
> risk, number of revisions that will occur, how controversial, number of
> -rc releases before Linus chooses to release, etc. You would need a
> crystal ball to predict much of this. I could see being able to provide
> a good estimate of this value for a relatively simple patch.
That's only partially true. I mean, the usual flux is to go up to -rc7.
After that, the final version is usually merged (except when something
goes wrong).
Anyway, I guess nobody would complain if the maintainers do an exception
here and accept more patches when they know that the last rc version
won't be -rc7, but, instead, -rc8 or -rc9.
This is a general ruleset that describes the usual behavior, telling the
developers the expected behavior. If the maintainers can do more on some
particular development cycle, it should be fine.
>
> > +
> > +
> > +Last -rc to merge features
> > +--------------------------
> > +Indicate to contributors the point at which an as yet un-applied patch
> > +set will need to wait for the NEXT+1 merge window. Of course there is no
> > +obligation to ever except any given patchset, but if the review has not
> > +concluded by this point the expectation the contributor should wait and
> > +resubmit for the following merge window. The answer may be different for
> > +'Core:' files, include a second entry prefixed with 'Core:' if so.
>
> Same comment as for the previous section, with the additional complexity
> that each unique patch series should bake in -next.
>
>
> > +
> > +
> > +Non-author Ack / Review Tags Required
> > +-------------------------------------
>
> The title of this section seems a bit different than the description
> below. A more aligned title would be something like "Maintainer
> self-commit review requirements".
>
>
> > +Let contributors and other maintainers know whether they can expect to
> > +see the maintainer self-commit patches without 3rd-party review. Some
> > +subsystem developer communities are so small as to make this requirement
> > +impractical. Others may have been bootstrapped by a submission of
> > +self-reviewed code at the outset, but have since moved to a
> > +non-author review-required stance. This section sets expectations on the
> > +code-review economics in the subsystem. For example, can a contributor
> > +trade review of a maintainer's, or other contributor's patches in
> > +exchange for consideration of their own.
>
> Then another section (which is the one I expected when I slightly
> mis-read the section title) would be: Review Tags Requirements",
> which would discuss what type of review is expected, encouraged,
> or required.
>
>
> > +
> > +
> > +Test Suite
> > +----------
> > +Indicate the test suite all patches are expected to pass before being
> > +submitted for inclusion consideration.
> > +
> > +
> > +Resubmit Cadence
> > +----------------
> > +Define a rate at which a contributor should wait to resubmit a patchset
> > +that has not yet received comments. A general guideline is to try to
> > +meet a deadline of 1 - 2 weeks to acknowledge starting consideration for
> > +a patch set.
>
> Or ping instead of resubmitting? If you resubmit the same series with
> an unchanged version then comments could be split across multiple
> email threads. If you resubmit the same series with a new version
> number, the same comment split can occur plus it can be confusing
> that two versions have the same contents. I suspect that there may be
> some maintainers who do prefer a resubmit, but that is just wild
> conjecture on my part.
That depends on how maintainers handle patches. Those subsystems that
use patchwork (like media) don't really require anyone to resubmit
patches. They're all stored there at patchwork database.
My workflow is that, if I receive two patches from the same person with
the same subject, I simply mark the first one as obsoleted, as usually
the second one is a new version, and reviewers should need some
time to handle it.
In other words, re-sending a patch may actually delay its handling, as
the second version will be later on my input queue, and I'll grant
additional time for people to review the second version at the community.
>
>
> > +
> > +
> > +Trusted Reviewers
> > +-----------------
> > +While a maintainer / maintainer-team is expected to be reviewer of last
> > +resort the review load is less onerous when distributed amongst
> > +contributors and or a trusted set of individuals. This section is
> > +distinct from the R: tag (Designated Reviewer). Whereas R: identifies
> > +reviewers that should always be copied on a patch submission, the
> > +trusted reviewers here are individuals contributors can reach out to if
> > +a few 'Resubmit Cadence' intervals have gone by without maintainer
> > +action, or to otherwise consult for advice.
>
> This seems redundant with the MAINTAINERS reviewers list. It seems like
> the role specified in this section is more of an ombudsman or developer
> advocate who can assist with the review and/or accept flow if the
> maintainer is being slow to respond.
Well, on subsystems that have sub-maintainers, there's no way to point to
it at MAINTAINERS file.
Also, not sure about others, but I usually avoid touching at existing
MAINTAINERS file entries. This is a file that everyone touches, so it
has higher chances of conflicts.
Also, at least on media, we have 5 different API sets (digital TV, V4L2, CEC,
media controller, remote controller). Yet, all drivers are stored at the
same place (as a single driver may use multiple APIs).
The reviewers for each API set are different. There isn't a good way
to explain that inside a MAINTANERS file.
>
>
> > +
> > +
> > +Time Zone / Office Hours
> > +------------------------
> > +Let contributors know the time of day when one or more maintainers are
> > +usually actively monitoring the mailing list.
>
> I would strike "actively monitoring the mailing list". To me, it should
> be what are the hours of the day that the maintainer might happen to poll
> (or might receive an interrupt) from the appropriate communications
> channels (could be IRC, could be email, etc).
>
> For my area, I would want to say something like: I tend to be active
> between 17:00 UTC (18:00 UTC when daylight savings) and 25:00 (26:00),
> but often will check for urgent or brief items up until 07:00 (08:00).
> I interact with email via a poll model. I interact with IRC via a
> pull model and often overlook IRC activity for multiple days).
Frankly, for media, I don't think that working hours makes sense. Media
(sub-)maintainers are spread around the globe, on different time zones
(in US, Brazil and Europe). We also have several active developers in
Japan, so we may end by having some day reviewers/sub-maintainers from
there.
At max, we can say that we won't warrant to patches on weekends or holidays.
Cheers,
Mauro
On Thu, 15 Nov 2018, Julia Lawall <[email protected]> wrote:
> On Thu, 15 Nov 2018, Geert Uytterhoeven wrote:
>
>> Hi Julia,
>>
>> On Thu, Nov 15, 2018 at 6:48 AM Julia Lawall <[email protected]> wrote:
>> > How about patch subject lines? What is the formula that should be used to
>> > transform the name(s) of the affected file(s) into an appropriate suject
>> > line?
>>
>> Automating that may be difficult.
>> I always use "git log --oneline", and try to derive something sane
>> from its output.
>
> Yes, I do likewise. But there may be some subsystems for which it would
> be possible to come up with a more specific policy. The advantage of what
> is proposed here is that it is not necessary to come up with a single
> formula that works everywhere. Even a description in English could be
> helpful.
I quickly cooked up this script to produce the top-5 commit prefixes for
the given files over the arbitrary last 200 commits. It'll give you a
pretty good idea if you're even close.
---
#!/bin/sh
# usage: subject-prefix FILE [...]
# show top 5 subject prefixes for FILEs
git log --format=%s -n 200 -- "$@" |\
grep -v "^Merge " |\
sed 's/\(.*\):.*/\1/' |\
sort | uniq -c | sort -nr | sed 's/ *[0-9]\+ //' |\
head -n 5
---
Someone who knows perl could turn that into a checkpatch check: See if
the patch subject prefix is one of the top-5 for all files changed by
the patch, and ask the user to double check if it isn't. Or some
heuristics thereof.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
On 11/15/18 4:11 PM, Frank Rowand wrote:
> On 11/14/18 8:53 PM, Dan Williams wrote:
< snip >
>> +
>> +
>> +Time Zone / Office Hours
>> +------------------------
>> +Let contributors know the time of day when one or more maintainers are
>> +usually actively monitoring the mailing list.
>
> I would strike "actively monitoring the mailing list". To me, it should
> be what are the hours of the day that the maintainer might happen to poll
> (or might receive an interrupt) from the appropriate communications
> channels (could be IRC, could be email, etc).
>
> For my area, I would want to say something like: I tend to be active
> between 17:00 UTC (18:00 UTC when daylight savings) and 25:00 (26:00),
> but often will check for urgent or brief items up until 07:00 (08:00).
> I interact with email via a poll model. I interact with IRC via a
> pull model and often overlook IRC activity for multiple days).
^^^^ typo, that should be "poll", not "pull"
>
> -Frank
>
>
On Fri, 2018-11-16 at 14:44 +0200, Jani Nikula wrote:
> I quickly cooked up this script to produce the top-5 commit prefixes for
> the given files over the arbitrary last 200 commits. It'll give you a
> pretty good idea if you're even close.
>
> ---
> #!/bin/sh
> # usage: subject-prefix FILE [...]
> # show top 5 subject prefixes for FILEs
>
> git log --format=%s -n 200 -- "$@" |\
> grep -v "^Merge " |\
> sed 's/\(.*\):.*/\1/' |\
> sort | uniq -c | sort -nr | sed 's/ *[0-9]\+ //' |\
> head -n 5
> ---
>
> Someone who knows perl could turn that into a checkpatch check: See if
> the patch subject prefix is one of the top-5 for all files changed by
> the patch, and ask the user to double check if it isn't. Or some
> heuristics thereof.
This won't work when a patch contains multiple files
from different paths, or even multiple files from a
single driver.
Perhaps it's better to use a generic mechanism like
basename $(dirname $filename):
with some exceptions and add an override patch subject
grammar to appropriate various sections of MAINTAINERS.
I also think it's better to use a separate script like
scripts/spdxcheck.py and tie any necessary checkpatch
use to that script.
On Fri, Nov 16, 2018 at 4:04 AM Mauro Carvalho Chehab
<[email protected]> wrote:
>
> Em Thu, 15 Nov 2018 16:11:59 -0800
> Frank Rowand <[email protected]> escreveu:
[..]
> > > +Patches or Pull requests
> > > +------------------------
> > > +Some subsystems allow contributors to send pull requests, most require
> > > +mailed patches. State “Patches only”, or “Pull requests accepted”.
> >
> > This may create some confusion if only "Pull requests accepted" is the
> > contents of this section. My understanding has been that all changes
> > should be available on a mail list for review before being accepted
> > by the maintainer, even if eventually the final version of the series
> > that is accepted is applied by the maintainer as a pull instead of
> > via applying patches.
> >
> > Is my "must appear on a mail list" understanding correct?
>
> I guess this is true on almost all subsystems that accept pull requests.
>
> It could not be true if some subsystem moves to Github/Gitlab.
Yes. Maybe a "Review Forum" section for subsystems that have
transitioned from email to a web-based tool? There's also the
exception of security disclosures, but the expectations for those
patches are already documented.
> > > +Last -rc for new feature submissions
> > > +------------------------------------
> > > +New feature submissions targeting the next merge window should have
> > > +their first posting for consideration before this point. Patches that
> > > +are submitted after this point should be clear that they are targeting
> > > +the NEXT+1 merge window, or should come with sufficient justification
> > > +why they should be considered on an expedited schedule. A general
> > > +guideline is to set expectation with contributors that new feature
> > > +submissions should appear before -rc5. The answer may be different for
> > > +'Core:' files, include a second entry prefixed with 'Core:' if so.
> >
> > I would expect the -rc to vary based on the patch series size, complexity,
> > risk, number of revisions that will occur, how controversial, number of
> > -rc releases before Linus chooses to release, etc. You would need a
> > crystal ball to predict much of this. I could see being able to provide
> > a good estimate of this value for a relatively simple patch.
>
> That's only partially true. I mean, the usual flux is to go up to -rc7.
> After that, the final version is usually merged (except when something
> goes wrong).
>
> Anyway, I guess nobody would complain if the maintainers do an exception
> here and accept more patches when they know that the last rc version
> won't be -rc7, but, instead, -rc8 or -rc9.
>
> This is a general ruleset that describes the usual behavior, telling the
> developers the expected behavior. If the maintainers can do more on some
> particular development cycle, it should be fine.
Yes, and perhaps I should clarify that this is the point at which a
maintainer will start to push back in the typical case, and indicate
to a contributor that they are standing in exceptional territory.
Similar to how later in the -rc series patches get increasing
scrutiny.
> > > +Last -rc to merge features
> > > +--------------------------
> > > +Indicate to contributors the point at which an as yet un-applied patch
> > > +set will need to wait for the NEXT+1 merge window. Of course there is no
> > > +obligation to ever except any given patchset, but if the review has not
> > > +concluded by this point the expectation the contributor should wait and
> > > +resubmit for the following merge window. The answer may be different for
> > > +'Core:' files, include a second entry prefixed with 'Core:' if so.
> >
> > Same comment as for the previous section, with the additional complexity
> > that each unique patch series should bake in -next.
The intent is to try to leave all "should" or prescriptive statements
out of the profile. I'm looking to target other parts of the handbook
to carry advice for integrating with -next and suggested soak times.
> > > +Non-author Ack / Review Tags Required
> > > +-------------------------------------
> >
> > The title of this section seems a bit different than the description
> > below. A more aligned title would be something like "Maintainer
> > self-commit review requirements".
This is intended to go beyond maintainer self-commits. Consider a pull
request that contains commits without non-author tags.
> > > +Let contributors and other maintainers know whether they can expect to
> > > +see the maintainer self-commit patches without 3rd-party review. Some
> > > +subsystem developer communities are so small as to make this requirement
> > > +impractical. Others may have been bootstrapped by a submission of
> > > +self-reviewed code at the outset, but have since moved to a
> > > +non-author review-required stance. This section sets expectations on the
> > > +code-review economics in the subsystem. For example, can a contributor
> > > +trade review of a maintainer's, or other contributor's patches in
> > > +exchange for consideration of their own.
> >
> > Then another section (which is the one I expected when I slightly
> > mis-read the section title) would be: Review Tags Requirements",
> > which would discuss what type of review is expected, encouraged,
> > or required.
Per the clarification above, I think the whole thing should be called
"Review Tags Requirement".
> > > +Test Suite
> > > +----------
> > > +Indicate the test suite all patches are expected to pass before being
> > > +submitted for inclusion consideration.
> > > +
> > > +
> > > +Resubmit Cadence
> > > +----------------
> > > +Define a rate at which a contributor should wait to resubmit a patchset
> > > +that has not yet received comments. A general guideline is to try to
> > > +meet a deadline of 1 - 2 weeks to acknowledge starting consideration for
> > > +a patch set.
> >
> > Or ping instead of resubmitting? If you resubmit the same series with
> > an unchanged version then comments could be split across multiple
> > email threads. If you resubmit the same series with a new version
> > number, the same comment split can occur plus it can be confusing
> > that two versions have the same contents. I suspect that there may be
> > some maintainers who do prefer a resubmit, but that is just wild
> > conjecture on my part.
>
> That depends on how maintainers handle patches. Those subsystems that
> use patchwork (like media) don't really require anyone to resubmit
> patches. They're all stored there at patchwork database.
>
> My workflow is that, if I receive two patches from the same person with
> the same subject, I simply mark the first one as obsoleted, as usually
> the second one is a new version, and reviewers should need some
> time to handle it.
>
> In other words, re-sending a patch may actually delay its handling, as
> the second version will be later on my input queue, and I'll grant
> additional time for people to review the second version at the community.
Ok, this sounds like a separate policy. How often to follow-up
separated from resend the full series vs send a ping mail.
> > > +Trusted Reviewers
> > > +-----------------
> > > +While a maintainer / maintainer-team is expected to be reviewer of last
> > > +resort the review load is less onerous when distributed amongst
> > > +contributors and or a trusted set of individuals. This section is
> > > +distinct from the R: tag (Designated Reviewer). Whereas R: identifies
> > > +reviewers that should always be copied on a patch submission, the
> > > +trusted reviewers here are individuals contributors can reach out to if
> > > +a few 'Resubmit Cadence' intervals have gone by without maintainer
> > > +action, or to otherwise consult for advice.
> >
> > This seems redundant with the MAINTAINERS reviewers list. It seems like
> > the role specified in this section is more of an ombudsman or developer
> > advocate who can assist with the review and/or accept flow if the
> > maintainer is being slow to respond.
>
> Well, on subsystems that have sub-maintainers, there's no way to point to
> it at MAINTAINERS file.
>
> Also, not sure about others, but I usually avoid touching at existing
> MAINTAINERS file entries. This is a file that everyone touches, so it
> has higher chances of conflicts.
>
> Also, at least on media, we have 5 different API sets (digital TV, V4L2, CEC,
> media controller, remote controller). Yet, all drivers are stored at the
> same place (as a single driver may use multiple APIs).
>
> The reviewers for each API set are different. There isn't a good way
> to explain that inside a MAINTANERS file.
Would it be worthwhile to have separate Subsystem Profiles for those
API reviewers? If they end up merging patches and sending them
upstream might we need a hierarchy of profiles for each hop along the
upstream merge path?
> > > +Time Zone / Office Hours
> > > +------------------------
> > > +Let contributors know the time of day when one or more maintainers are
> > > +usually actively monitoring the mailing list.
> >
> > I would strike "actively monitoring the mailing list". To me, it should
> > be what are the hours of the day that the maintainer might happen to poll
> > (or might receive an interrupt) from the appropriate communications
> > channels (could be IRC, could be email, etc).
Yes, makes sense.
> > For my area, I would want to say something like: I tend to be active
> > between 17:00 UTC (18:00 UTC when daylight savings) and 25:00 (26:00),
> > but often will check for urgent or brief items up until 07:00 (08:00).
> > I interact with email via a poll model. I interact with IRC via a
> > pull model and often overlook IRC activity for multiple days).
>
> Frankly, for media, I don't think that working hours makes sense. Media
> (sub-)maintainers are spread around the globe, on different time zones
> (in US, Brazil and Europe). We also have several active developers in
> Japan, so we may end by having some day reviewers/sub-maintainers from
> there.
For that case just say:
"the sun never sets on the media subsystem" ;-)
> At max, we can say that we won't warrant to patches on weekends or holidays.
Yeah, maybe:
"outside of weekends or holidays there's usually a maintainer or
reviewer monitoring the mailing list"
On Thu, Nov 15, 2018 at 12:03 AM Geert Uytterhoeven
<[email protected]> wrote:
>
> Hi Dan,
>
> On Thu, Nov 15, 2018 at 6:06 AM Dan Williams <[email protected]> wrote:
> > Document the basic policies of the libnvdimm subsystem and provide a
> > first example of a Subsystem Profile for others to duplicate and edit.
> >
> > Cc: Ross Zwisler <[email protected]>
> > Cc: Vishal Verma <[email protected]>
> > Cc: Dave Jiang <[email protected]>
> > Signed-off-by: Dan Williams <[email protected]>
>
> Thanks for your patch!
>
> > --- /dev/null
> > +++ b/Documentation/nvdimm/subsystem-profile.rst
>
> > +Trusted Reviewers
> > +-----------------
> > +Johannes Thumshirn
> > +Toshi Kani
> > +Jeff Moyer
> > +Robert Elliott
>
> Don't you want to add email addresses?
I do, but I had not cleared this designation with these folks ahead of
time. Will resolve that for v2.
> Only the first one is listed in MAINTAINERS.
Correct, the intent is that these individuals can be sought out for
advice when the maintainer may not have the bandwidth to engage with a
contributor on a given timeframe. These are individuals that can
provide a second opinion.
> > +Time Zone / Office Hours
> > +------------------------
> > +8:00am to 5:00pm Pacific Time Zone
>
> UTC-???
>
> People are not familiar with all time zones.
Yes, will fix.
On Thu, Nov 15, 2018 at 6:31 AM Mauro Carvalho Chehab
<[email protected]> wrote:
>
> Em Wed, 14 Nov 2018 20:53:30 -0800
> Dan Williams <[email protected]> escreveu:
>
> > Document the basic policies of the libnvdimm subsystem and provide a
> > first example of a Subsystem Profile for others to duplicate and edit.
> >
> > Cc: Ross Zwisler <[email protected]>
> > Cc: Vishal Verma <[email protected]>
> > Cc: Dave Jiang <[email protected]>
> > Signed-off-by: Dan Williams <[email protected]>
> > ---
> > Documentation/nvdimm/subsystem-profile.rst | 86 ++++++++++++++++++++++++++++
> > MAINTAINERS | 4 +
> > 2 files changed, 90 insertions(+)
> > create mode 100644 Documentation/nvdimm/subsystem-profile.rst
> >
> > diff --git a/Documentation/nvdimm/subsystem-profile.rst b/Documentation/nvdimm/subsystem-profile.rst
> > new file mode 100644
> > index 000000000000..d3428be7528e
> > --- /dev/null
> > +++ b/Documentation/nvdimm/subsystem-profile.rst
>
> Hmm... would it make sense to add a pointer at maintainer/index.rst (or to some
> other .rst file) for those profiles too?
>
> > @@ -0,0 +1,86 @@
> > +LIBNVDIMM Subsystem Profile
> > +===========================
> > +
> > +Overview
> > +--------
>
> A minor nitpick here: I would add a blank line after each topic/subtopic.
>
> On some cases, Sphinx will do wrong without that blank line, and having
> some places with that extra line and others without it sounds unbalanced
> on my eyes ;-)
>
> > +So, you have recently become a maintainer of the LIBNVDIMM subsystem,
> > +condolences, it is a thankless job, here is the lay of the land. The git
>
> My understanding that the main focus of this document is to help people to
> submit patches to the subsystem.
>
> With that in mind, I would never start the doc talking only to maintainers,
> as developers will likely just stop reading it at the above paragraph.
Yes, I see this is confusing now. I'll fix up the Subsystem Profile
description text to be written with maintainers as the audience and
clarify that the per-subsystem instance is written with contributors
as the audience.
>
> > +tree, git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/, is
> > +writable by all the individuals listed in LIBNVDIMM section of
> > +MAINTAINERS. Access is granted per the typical kernel.org account
> > +management policies. Two branches in that tree are regularly pulled into
> > +-next, libnvdimm-for-next, and libnvdimm-fixes. The submit rate of
> > +patches is low, usually enough for one person to handle. There is a
> > +patchwork instance at
> > +https://patchwork.kernel.org/project/linux-nvdimm/list/, and it
> > +historically is only used for ingesting patches and collecting
> > +ack/review tags, i.e. no expectation to update the patch state after it
> > +has been dispositioned, or merged.
> > +
> > +The most sensitive code area is the ACPI DSM (Device Specific Method)
> > +path. In addition to the general fragility of an ioctl() ABI the ACPI
> > +DSM scheme allows any vendor to implement any command without any prior
> > +review by the ACPI committee. For this reason the LIBNVDIMM system seeks
> > +to constrain the proliferation of vendor commands and at a minimum
> > +requires any command support to be publicly documented. Over time the
> > +submission rate of new vendor-specific commands is falling as more
> > +commands are defined with named methods in the official ACPI
> > +specification.
>
> As Jani pointed, all the above stuff is for maintainers, but several other
> stuff on this document are for developers. The best would likely to have
> two separate files.
>
> However, maintaining it on two separate files could be painful. Maybe
> we could have an specific section, at the end of the document, with
> maintainers-specific instructions.
Yes, good idea, will incorporate.
On Thu, Nov 15, 2018 at 8:38 AM Leon Romanovsky <[email protected]> wrote:
>
> On Thu, Nov 15, 2018 at 06:10:36AM -0800, Mauro Carvalho Chehab wrote:
> > Em Thu, 15 Nov 2018 09:03:11 +0100
> > Geert Uytterhoeven <[email protected]> escreveu:
> >
> > > Hi Dan,
> > >
> > > On Thu, Nov 15, 2018 at 6:06 AM Dan Williams <[email protected]> wrote:
> > > > Document the basic policies of the libnvdimm subsystem and provide a
> > > > first example of a Subsystem Profile for others to duplicate and edit.
> > > >
> > > > Cc: Ross Zwisler <[email protected]>
> > > > Cc: Vishal Verma <[email protected]>
> > > > Cc: Dave Jiang <[email protected]>
> > > > Signed-off-by: Dan Williams <[email protected]>
> > >
> > > Thanks for your patch!
> > >
> > > > --- /dev/null
> > > > +++ b/Documentation/nvdimm/subsystem-profile.rst
> > >
> > > > +Trusted Reviewers
> > > > +-----------------
> > > > +Johannes Thumshirn
> > > > +Toshi Kani
> > > > +Jeff Moyer
> > > > +Robert Elliott
> > >
> > > Don't you want to add email addresses?
> > > Only the first one is listed in MAINTAINERS.
> >
> > IMO, it makes sense to have their e-mails here, in a way that it could
> > easily be parsed by get_maintainers.pl.
>
> I personally think that list of "trusted reviewers" makes more harm than
> good. It creates unneeded negative feelings to those who wanted to be in
> this list, but for any reason they don't. Those reviewers will feel
> "untrusted".
I'd like to +1 on this concern here. Besides leaving all the other
people demotivated.
A small group of trusted reviewers doesn't scale. People will get overloaded.
Or you won't be able to enforce that all patches need to get Reviews.
Reviews should be coming from everywhere and commiters and maintainers
deciding on what to trust or re-review.
Also the list is hard to maintain and keep the lists updated.
>
> Thanks
> _______________________________________________
> Ksummit-discuss mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
On 11/15/18 7:44 AM, Mauro Carvalho Chehab wrote:
>
> Anyway, RFC patch follows.
>
> -
>
> [PATCH] [RFC] Add a system profile description for media subsystem
>
> This RFC aligns with current Dan's proposal for having subsystem
> specific ruleset stored at the Kernel tree.
>
> On this initial RFC, I opted to not add the reviewers e-mail
> (adding just a "<>") as a boilerplate. If we decide keeping emails
> there, I'll add them.
Hi-
Here are my comments.
Hopefully the email addresses will be added. Just having names is a
half-answer for contact info.
>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
>
> diff --git a/Documentation/media/subsystem-profile.rst b/Documentation/media/subsystem-profile.rst
> new file mode 100644
> index 000000000000..7a5d6f691d05
> --- /dev/null
> +++ b/Documentation/media/subsystem-profile.rst
> @@ -0,0 +1,186 @@
> +Media Subsystem Profile
> +=======================
> +
> +Overview
> +--------
> +
> +The media subsystem cover support for a variety of devices: stream
covers
> +capture, analog and digital TV, cameras, remote controllers, HDMI CEC
> +and media pipeline control.
> +
> +Both our userspace and Kernel APIs are documented and should be kept in
> +sync with the API changes. It means that all patches that add new
> +features to the subsystem should also bring changes to the corresponding
> +API files.
> +
> +Also, patches for device drivers that changes the Open Firmware/Device
> +Tree bindings should be reviewed by the Device Tree maintainers.
> +
> +Due to the size and wide scope of the media subsystem, our
> +maintainership model is to have sub-maintainers that have a broad
> +knowledge of an specific aspect of the subsystem. It is a
of a specific
> +sub-maintainers task to review the patches, providing feedback to users
> +if the patches are following the subsystem rules and are properly using
> +the media internal and external APIs.
> +
> +We have a set of compliance tools at https://git.linuxtv.org/v4l-utils.git/
> +that should be used in order to check if the drivers are properly
> +implementing the media APIs.
> +
> +Patches for the media subsystem should be sent to the media mailing list
> +at [email protected] as plain text only e-mail. emails with
e-mail or email? Be consistent. (more below)
> +HTML will be automacially rejected by the mail server.
automatically
> +
> +Our workflow is heavily based on Patchwork, meaning that, once a patch
> +is submitted, it should appear at:
> +
> + - https://patchwork.linuxtv.org/project/linux-media/list/
> +
> +If it doesn't automatically appear there after a few minutes, then
> +probably something got wrong on your submission. Please check if the
> +email is in plain text only and if your emailer is not mangling with
email
> +whitespaces before complaining or submit it again.
> +
> +Core
> +----
> +
> +Documentation
> ++++++++++++++
> +
> +F: Documentation/media
> +
> +Kernelspace API headers
> ++++++++++++++++++++++++
> +
> +F: include/media/*.h
> +
> +Digital TV Core
> ++++++++++++++++
> +
> +F: drivers/media/dvb-core
> +
> +HDMI CEC Core
> ++++++++++++++
> +
> +F: drivers/media/cec
> +
> +Media Controller Core
> ++++++++++++++++++++++
> +
> +F: drivers/media/media-\*.[ch]
> +
> +Remote Controller Core
> +++++++++++++++++++++++
> +
> +F: drivers/media/rc/rc-core-priv.h
> +F: drivers/media/rc/rc-ir-raw.c
> +F: drivers/media/rc/rc-main.c
> +F: drivers/media/rc/ir\*-decoder.c
> +F: drivers/media/rc/lirc_dev.c
> +
> +Video4linux Core
> +++++++++++++++++
> +
> +F: drivers/media/v4l2-core
> +
> +Patches or Pull requests
> +------------------------
> +
> +All patches should be submitted via e-mail for review. We use
and e-mail
> +pull requests on our workflow between sub-maintainers and the
> +maintainer.
> +
> +
> +Last day for new feature submissions
> +------------------------------------
> +
> +Before -rc5
> +
> +
> +Last day to merge features
> +--------------------------
> +
> +Before -rc7
> +
> +
> +Non-author Ack / Review Tags Required
> +-------------------------------------
> +
> +Not required, but desirable
> +
> +
> +Test Suite
> +----------
> +
> +Use the several *-compliance tools that are part of the v4l-utils
> +package.
> +
> +
> +Trusted Reviewers
> +-----------------
> +
> +Sub-maintainers
> ++++++++++++++++
> +
> +At the media subsystem, we have a group of senior developers that are
> +responsible for doing the code reviews at the drivers (called
> +sub-maintainers), and another senior developer responsible for the
> +subsystem as a hole. For core changes, whenever possible, multiple
as a whole.
> +media (sub-)maintainers do the review.
> +
> +The sub-maintainers work on specific areas of the subsystem, as
> +described below:
> +
> +- Sensor drivers
> +
> + R: Sakari Ailus <>
> +
> +- V4L2 drivers
> +
> + R: Hans Verkuil <>
> +
> +- Media controller drivers
> +
> + R: Laurent Pinchart <>
> +
> +- HDMI CEC
> +
> +- Remote Controllers
> +
> + R: Sean Young <>
> +
> +- Digital TV
> +
> + R: Michael Krufky <>
> + R: Sean Young <>
> +
> +
> +Resubmit Cadence
> +----------------
> +
> +Provided that your patch is at https://patchwork.linuxtv.org, it should
> +be sooner or later handled, so you don't need to re-submit a patch.
Resubmit or re-submit? Be consistent.
> +
> +Please notice that the media subsystem is a high traffic one, so it
> +could take a while for us to be able to review your patches. Feel free
> +to ping if you don't get a feedback on a couple of weeks.
in a
> +
> +Time Zone / Office Hours
> +------------------------
> +
> +Media developers are distributed all around the globe. So, don't assume
> +that we're on your time zone. We usually don't work on local holidays or
> +at weekends. Please also notice that, during the Kernel merge window,
> +we're usually busy ensuring that everything goes smoothly, meaning that
> +we usually have a lot of patches waiting for review just after that. So
> +you should expect a higher delay during the merge window and one week
> +before/after it.
> +
> +
> +Checkpatch / Style cleanups
> +---------------------------
> +
> +Standalone style-cleanups are welcome, but they should be grouped per
> +directory. So, for example, if you're doing a cleanup at drivers
> +under drivers/media, please send a single patch for all drivers under
> +drivers/media/pci, another one for drivers/media/usb and so on.
> Cheers.-
~Randy
On Fri, Nov 16, 2018 at 12:37 PM Rodrigo Vivi <[email protected]> wrote:
>
> On Thu, Nov 15, 2018 at 8:38 AM Leon Romanovsky <[email protected]> wrote:
> >
> > On Thu, Nov 15, 2018 at 06:10:36AM -0800, Mauro Carvalho Chehab wrote:
> > > Em Thu, 15 Nov 2018 09:03:11 +0100
> > > Geert Uytterhoeven <[email protected]> escreveu:
> > >
> > > > Hi Dan,
> > > >
> > > > On Thu, Nov 15, 2018 at 6:06 AM Dan Williams <[email protected]> wrote:
> > > > > Document the basic policies of the libnvdimm subsystem and provide a
> > > > > first example of a Subsystem Profile for others to duplicate and edit.
> > > > >
> > > > > Cc: Ross Zwisler <[email protected]>
> > > > > Cc: Vishal Verma <[email protected]>
> > > > > Cc: Dave Jiang <[email protected]>
> > > > > Signed-off-by: Dan Williams <[email protected]>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > --- /dev/null
> > > > > +++ b/Documentation/nvdimm/subsystem-profile.rst
> > > >
> > > > > +Trusted Reviewers
> > > > > +-----------------
> > > > > +Johannes Thumshirn
> > > > > +Toshi Kani
> > > > > +Jeff Moyer
> > > > > +Robert Elliott
> > > >
> > > > Don't you want to add email addresses?
> > > > Only the first one is listed in MAINTAINERS.
> > >
> > > IMO, it makes sense to have their e-mails here, in a way that it could
> > > easily be parsed by get_maintainers.pl.
> >
> > I personally think that list of "trusted reviewers" makes more harm than
> > good. It creates unneeded negative feelings to those who wanted to be in
> > this list, but for any reason they don't. Those reviewers will feel
> > "untrusted".
>
> I'd like to +1 on this concern here. Besides leaving all the other
> people demotivated.
Yes, that's a valid concern, I overlooked that unfortunate interpretation.
>
> A small group of trusted reviewers doesn't scale. People will get overloaded.
> Or you won't be able to enforce that all patches need to get Reviews.
>
> Reviews should be coming from everywhere and commiters and maintainers
> deciding on what to trust or re-review.
>
> Also the list is hard to maintain and keep the lists updated.
I understand the concern, and as I saw feedback come in I realized
there were more people that I would add to that reviewer list for
libnvdimm.
Stepping back the end goal is to have an initial list of recommended
people to follow up with directly to seek a second opinion, or help in
cases where a contributor otherwise needs some direction / engagement
that they are not readily receiving from the maintainer. Typically
someone just lurks on the mailing list for a few weeks to get a feel
for who the usual suspects are in the subsystem, but for a new
contributor identifying those individuals may be difficult.
One of the contributing factors of lack of response to a patchset is
that they are sent with the implicit expectation that the maintainer
will get to eventually, and typically other people feel content to sit
back and watch. If instead a contributor sent a direct mail to a
"trusted reviewer" saying, "Hey, Alice, Bob seems busy can you help me
out?" that seems more likely to rope in additional review help.
On Wed, 2018-11-14 at 20:53 -0800, Dan Williams wrote:
> +Time Zone / Office Hours
> +------------------------
> +8:00am to 5:00pm Pacific Time Zone
Use Olson time zone names here please, if we must have them at all.
On Fri, Nov 16 2018, Dan Williams wrote:
> On Fri, Nov 16, 2018 at 12:37 PM Rodrigo Vivi <[email protected]> wrote:
>>
>> On Thu, Nov 15, 2018 at 8:38 AM Leon Romanovsky <[email protected]> wrote:
>> >
>> > On Thu, Nov 15, 2018 at 06:10:36AM -0800, Mauro Carvalho Chehab wrote:
>> > > Em Thu, 15 Nov 2018 09:03:11 +0100
>> > > Geert Uytterhoeven <[email protected]> escreveu:
>> > >
>> > > > Hi Dan,
>> > > >
>> > > > On Thu, Nov 15, 2018 at 6:06 AM Dan Williams <[email protected]> wrote:
>> > > > > Document the basic policies of the libnvdimm subsystem and provide a
>> > > > > first example of a Subsystem Profile for others to duplicate and edit.
>> > > > >
>> > > > > Cc: Ross Zwisler <[email protected]>
>> > > > > Cc: Vishal Verma <[email protected]>
>> > > > > Cc: Dave Jiang <[email protected]>
>> > > > > Signed-off-by: Dan Williams <[email protected]>
>> > > >
>> > > > Thanks for your patch!
>> > > >
>> > > > > --- /dev/null
>> > > > > +++ b/Documentation/nvdimm/subsystem-profile.rst
>> > > >
>> > > > > +Trusted Reviewers
>> > > > > +-----------------
>> > > > > +Johannes Thumshirn
>> > > > > +Toshi Kani
>> > > > > +Jeff Moyer
>> > > > > +Robert Elliott
>> > > >
>> > > > Don't you want to add email addresses?
>> > > > Only the first one is listed in MAINTAINERS.
>> > >
>> > > IMO, it makes sense to have their e-mails here, in a way that it could
>> > > easily be parsed by get_maintainers.pl.
>> >
>> > I personally think that list of "trusted reviewers" makes more harm than
>> > good. It creates unneeded negative feelings to those who wanted to be in
>> > this list, but for any reason they don't. Those reviewers will feel
>> > "untrusted".
>>
>> I'd like to +1 on this concern here. Besides leaving all the other
>> people demotivated.
>
> Yes, that's a valid concern, I overlooked that unfortunate interpretation.
>
>>
>> A small group of trusted reviewers doesn't scale. People will get overloaded.
>> Or you won't be able to enforce that all patches need to get Reviews.
>>
>> Reviews should be coming from everywhere and commiters and maintainers
>> deciding on what to trust or re-review.
>>
>> Also the list is hard to maintain and keep the lists updated.
>
> I understand the concern, and as I saw feedback come in I realized
> there were more people that I would add to that reviewer list for
> libnvdimm.
>
> Stepping back the end goal is to have an initial list of recommended
> people to follow up with directly to seek a second opinion, or help in
> cases where a contributor otherwise needs some direction / engagement
> that they are not readily receiving from the maintainer. Typically
> someone just lurks on the mailing list for a few weeks to get a feel
> for who the usual suspects are in the subsystem, but for a new
> contributor identifying those individuals may be difficult.
>
> One of the contributing factors of lack of response to a patchset is
> that they are sent with the implicit expectation that the maintainer
> will get to eventually, and typically other people feel content to sit
> back and watch. If instead a contributor sent a direct mail to a
> "trusted reviewer" saying, "Hey, Alice, Bob seems busy can you help me
> out?" that seems more likely to rope in additional review help.
In here is, I think, a real issue that listing "trusted reviewers" might
help address.
As you say, people don't feel the need to comment - particularly if they
don't see anything wrong (often best to insert a bug to encourage
responses!).
Maybe if we list people, it will make them feel that their opinion is
valuable (trusted!) and that will encourage them to Ack or Review a
patch.
I have found that being given a title of responsibility can change my
thinking from "someone should" to "I should".
NeilBrown
On 11/15/2018 04:44 PM, Mauro Carvalho Chehab wrote:
> Em Wed, 14 Nov 2018 20:53:25 -0800
> Dan Williams <[email protected]> escreveu:
>
>> As presented at the 2018 Linux Plumbers conference [1], the Subsystem
>> Profile is proposed as a way to reduce friction between committers and
>> maintainers and perhaps encourage conversations amongst maintainers
>> about best practice policies.
>>
>> The profile contains short answers to some of the common policy
>> questions a contributor might have, or that a maintainer might consider
>> formalizing. The current list of maintenance policies is:
>>
>> Overview: General introduction to maintaining the subsystem
>> Core: List of source files considered core
>> Leaf: List of source files that consume core functionality
>> Patches or Pull requests: Simple statement of expected submission format
>> Last -rc for new feature submissions: Expected lead time for submissions
>> Last -rc to merge features: Deadline for merge decisions
>> Non-author Ack / Review Tags Required: Patch review economics
>> Test Suite: Pass this suite before requesting inclusion
>> Resubmit Cadence: When to ping the maintainer
>> Trusted Reviewers: Help for triaging patches
>> Time Zone / Office Hours: When might a maintainer be available
>> Checkpatch / Style Cleanups: Policy on pure cleanup patches
>> Off-list review: Request for review gates
>> TODO: Potential development tasks up for grabs, or active focus areas
>>
>> The goal of the Subsystem Profile is to set expectations for
>> contributors and interim or replacement maintainers for a subsystem.
>>
>> See Documentation/maintainer/subsystem-profile.rst for more details, and
>> a follow-on example profile for the libnvdimm subsystem.
>>
>> [1]: https://linuxplumbersconf.org/event/2/contributions/59/
>>
>> Cc: Jonathan Corbet <[email protected]>
>> Cc: Thomas Gleixner <[email protected]>
>> Cc: Mauro Carvalho Chehab <[email protected]>
>> Cc: Steve French <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: Linus Torvalds <[email protected]>
>> Cc: Tobin C. Harding <[email protected]>
>> Cc: Olof Johansson <[email protected]>
>> Cc: Martin K. Petersen <[email protected]>
>> Cc: Daniel Vetter <[email protected]>
>> Cc: Joe Perches <[email protected]>
>> Cc: Dmitry Vyukov <[email protected]>
>> Signed-off-by: Dan Williams <[email protected]>
>
> Ok, I sort of followed the proposed model, adding a documentation for
> the media subsystem using the template.
>
> I'm pretty sure this is incomplete and more work would be needed.
> So, for now, this is just an example.
>
> Also, I didn't test building it on Sphinx (nor added any reference for it
> at MAINTAINERS), as my main goal here was to see how the model would fit
> for us.
>
> I noticed a few issues there:
>
> 1) Describing the "core" files. The media subsystem is complex: depending on
> the device type, we have different APIs and different cores. So, our core
> is actually a set of different cores. I ended by adding sub-sections.
>
> Also, several things were not described there. For example, we store
> DVB frontend drivers on an specific directory. We have drivers there organized
> by bus type. So, one directory for I2C, one for USB, one for PCI, ...
>
> It should probably make sense to be able to tell about that.
>
> So, I would actually prefer to have, at the profile, a "files" section,
> instead of "core".
>
> 2) As I said before, on media, we have sub-maintainers. Not sure how to
> describe them on this template.
>
> 3) I noticed one big missing thing there: in the case of media, we are
> responsible also to maintaining media staging drivers, that goes at
> drivers/staging/media. IMO, it would be important to be able to describe
> who maintains staging stuff - e. g. if the subsystem maintainers prefer
> to let staging up to staging maintainers or if they'll maintain themselves.
>
> In the latter, it probably makes sense to describe any specific thing
> related to it (where staging will be at the tree, are there any special
> rules for them?)
>
> Anyway, RFC patch follows.
>
> -
>
> [PATCH] [RFC] Add a system profile description for media subsystem
>
> This RFC aligns with current Dan's proposal for having subsystem
> specific ruleset stored at the Kernel tree.
>
> On this initial RFC, I opted to not add the reviewers e-mail
> (adding just a "<>") as a boilerplate. If we decide keeping emails
> there, I'll add them.
>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
>
> diff --git a/Documentation/media/subsystem-profile.rst b/Documentation/media/subsystem-profile.rst
> new file mode 100644
> index 000000000000..7a5d6f691d05
> --- /dev/null
> +++ b/Documentation/media/subsystem-profile.rst
> @@ -0,0 +1,186 @@
> +Media Subsystem Profile
> +=======================
> +
> +Overview
> +--------
> +
> +The media subsystem cover support for a variety of devices: stream
> +capture, analog and digital TV, cameras, remote controllers, HDMI CEC
> +and media pipeline control.
And radio tuners and modulators, including support for RDS (Radio Data System),
and also software defined radio tuners and modulators.
We also support video codecs (very important to mention this!) and video
memory-to-memory processing (deinterlacers, scalers, etc.).
There is also support for touch devices and VBI (Vertical Blanking Interface).
> +
> +Both our userspace and Kernel APIs are documented and should be kept in
> +sync with the API changes. It means that all patches that add new
> +features to the subsystem should also bring changes to the corresponding
> +API files.
> +
> +Also, patches for device drivers that changes the Open Firmware/Device
> +Tree bindings should be reviewed by the Device Tree maintainers.
> +
> +Due to the size and wide scope of the media subsystem, our
> +maintainership model is to have sub-maintainers that have a broad
> +knowledge of an specific aspect of the subsystem. It is a
> +sub-maintainers task to review the patches, providing feedback to users
> +if the patches are following the subsystem rules and are properly using
> +the media internal and external APIs.
> +
> +We have a set of compliance tools at https://git.linuxtv.org/v4l-utils.git/
> +that should be used in order to check if the drivers are properly
> +implementing the media APIs.
New V4L2 and HDMI CEC drivers must pass their corresponding compliance tests
before they are accepted into the kernel.
Adding new public V4L2 or HDMI CEC APIs will also require adding new compliance
tests to ensure that the new APIs are tested.
> +
> +Patches for the media subsystem should be sent to the media mailing list
> +at [email protected] as plain text only e-mail. emails with
> +HTML will be automacially rejected by the mail server.
> +
> +Our workflow is heavily based on Patchwork, meaning that, once a patch
> +is submitted, it should appear at:
> +
> + - https://patchwork.linuxtv.org/project/linux-media/list/
> +
> +If it doesn't automatically appear there after a few minutes, then
> +probably something got wrong on your submission. Please check if the
> +email is in plain text only and if your emailer is not mangling with
> +whitespaces before complaining or submit it again.
> +
> +Core
> +----
> +
> +Documentation
> ++++++++++++++
> +
> +F: Documentation/media
> +
> +Kernelspace API headers
> ++++++++++++++++++++++++
> +
> +F: include/media/*.h
> +
> +Digital TV Core
> ++++++++++++++++
> +
> +F: drivers/media/dvb-core
> +
> +HDMI CEC Core
> ++++++++++++++
> +
> +F: drivers/media/cec
> +
> +Media Controller Core
> ++++++++++++++++++++++
> +
> +F: drivers/media/media-\*.[ch]
> +
> +Remote Controller Core
> +++++++++++++++++++++++
> +
> +F: drivers/media/rc/rc-core-priv.h
> +F: drivers/media/rc/rc-ir-raw.c
> +F: drivers/media/rc/rc-main.c
> +F: drivers/media/rc/ir\*-decoder.c
> +F: drivers/media/rc/lirc_dev.c
> +
> +Video4linux Core
> +++++++++++++++++
> +
> +F: drivers/media/v4l2-core
We should mention vb2 here as well:
F: drivers/media/common/videobuf2
It's a critical part of the V4L2/DVB core infrastructure.
> +
> +Patches or Pull requests
> +------------------------
> +
> +All patches should be submitted via e-mail for review. We use
> +pull requests on our workflow between sub-maintainers and the
> +maintainer.
> +
> +
> +Last day for new feature submissions
> +------------------------------------
> +
> +Before -rc5
> +
> +
> +Last day to merge features
> +--------------------------
> +
> +Before -rc7
> +
> +
> +Non-author Ack / Review Tags Required
> +-------------------------------------
> +
> +Not required, but desirable
> +
> +
> +Test Suite
> +----------
> +
> +Use the several *-compliance tools that are part of the v4l-utils
> +package.
> +
> +
> +Trusted Reviewers
> +-----------------
> +
> +Sub-maintainers
> ++++++++++++++++
> +
> +At the media subsystem, we have a group of senior developers that are
> +responsible for doing the code reviews at the drivers (called
> +sub-maintainers), and another senior developer responsible for the
> +subsystem as a hole. For core changes, whenever possible, multiple
> +media (sub-)maintainers do the review.
> +
> +The sub-maintainers work on specific areas of the subsystem, as
> +described below:
> +
> +- Sensor drivers
> +
> + R: Sakari Ailus <>
> +
> +- V4L2 drivers
> +
> + R: Hans Verkuil <>
> +
> +- Media controller drivers
> +
> + R: Laurent Pinchart <>
> +
> +- HDMI CEC
That's me as well :-)
> +
> +- Remote Controllers
> +
> + R: Sean Young <>
> +
> +- Digital TV
> +
> + R: Michael Krufky <>
> + R: Sean Young <>
> +
> +
> +Resubmit Cadence
> +----------------
> +
> +Provided that your patch is at https://patchwork.linuxtv.org, it should
> +be sooner or later handled, so you don't need to re-submit a patch.
> +
> +Please notice that the media subsystem is a high traffic one, so it
> +could take a while for us to be able to review your patches. Feel free
> +to ping if you don't get a feedback on a couple of weeks.
Hmm, I would prefer a ping in a week for high-prio patches. Otherwise wait
2-3 weeks.
> +
> +Time Zone / Office Hours
> +------------------------
> +
> +Media developers are distributed all around the globe. So, don't assume
> +that we're on your time zone. We usually don't work on local holidays or
> +at weekends. Please also notice that, during the Kernel merge window,
> +we're usually busy ensuring that everything goes smoothly, meaning that
> +we usually have a lot of patches waiting for review just after that. So
> +you should expect a higher delay during the merge window and one week
> +before/after it.
> +
> +
> +Checkpatch / Style cleanups
> +---------------------------
> +
> +Standalone style-cleanups are welcome, but they should be grouped per
> +directory. So, for example, if you're doing a cleanup at drivers
> +under drivers/media, please send a single patch for all drivers under
> +drivers/media/pci, another one for drivers/media/usb and so on.
Regards,
Hans
>
>
>
>
>
> Cheers,
> Mauro
>
On Fri, Nov 16, 2018 at 11:57 AM Joe Perches <[email protected]> wrote:
>
> On Fri, 2018-11-16 at 14:44 +0200, Jani Nikula wrote:
> > I quickly cooked up this script to produce the top-5 commit prefixes for
> > the given files over the arbitrary last 200 commits. It'll give you a
> > pretty good idea if you're even close.
> >
> > ---
> > #!/bin/sh
> > # usage: subject-prefix FILE [...]
> > # show top 5 subject prefixes for FILEs
> >
> > git log --format=%s -n 200 -- "$@" |\
> > grep -v "^Merge " |\
--no-merges in git log can replace this line.
> > sed 's/\(.*\):.*/\1/' |\
> > sort | uniq -c | sort -nr | sed 's/ *[0-9]\+ //' |\
> > head -n 5
> > ---
> >
> > Someone who knows perl could turn that into a checkpatch check: See if
> > the patch subject prefix is one of the top-5 for all files changed by
> > the patch, and ask the user to double check if it isn't. Or some
> > heuristics thereof.
>
> This won't work when a patch contains multiple files
> from different paths, or even multiple files from a
> single driver.
Different paths is often, but not always a sign that patches may need
to be split up. Maybe that is something checkpatch should point out.
> Perhaps it's better to use a generic mechanism like
>
> basename $(dirname $filename):
>
> with some exceptions and add an override patch subject
> grammar to appropriate various sections of MAINTAINERS.
Perhaps just use the script as a starting point to populate
MAINTAINERS as it may never be that accurate.
> I also think it's better to use a separate script like
> scripts/spdxcheck.py and tie any necessary checkpatch
> use to that script.
Yes, checkpatch is getting pretty unwieldy.
Rob
On Sat, 17 Nov 2018, Rob Herring wrote:
> On Fri, Nov 16, 2018 at 11:57 AM Joe Perches <[email protected]> wrote:
> >
> > On Fri, 2018-11-16 at 14:44 +0200, Jani Nikula wrote:
> > > I quickly cooked up this script to produce the top-5 commit prefixes for
> > > the given files over the arbitrary last 200 commits. It'll give you a
> > > pretty good idea if you're even close.
> > >
> > > ---
> > > #!/bin/sh
> > > # usage: subject-prefix FILE [...]
> > > # show top 5 subject prefixes for FILEs
> > >
> > > git log --format=%s -n 200 -- "$@" |\
> > > grep -v "^Merge " |\
>
> --no-merges in git log can replace this line.
>
> > > sed 's/\(.*\):.*/\1/' |\
> > > sort | uniq -c | sort -nr | sed 's/ *[0-9]\+ //' |\
> > > head -n 5
> > > ---
> > >
> > > Someone who knows perl could turn that into a checkpatch check: See if
> > > the patch subject prefix is one of the top-5 for all files changed by
> > > the patch, and ask the user to double check if it isn't. Or some
> > > heuristics thereof.
> >
> > This won't work when a patch contains multiple files
> > from different paths, or even multiple files from a
> > single driver.
>
> Different paths is often, but not always a sign that patches may need
> to be split up. Maybe that is something checkpatch should point out.
Between v4.0 and v4.19, 18% of commits touch multiple .c files. 35% of
commits touch multiple files in general.
julia
>
> > Perhaps it's better to use a generic mechanism like
> >
> > basename $(dirname $filename):
> >
> > with some exceptions and add an override patch subject
> > grammar to appropriate various sections of MAINTAINERS.
>
> Perhaps just use the script as a starting point to populate
> MAINTAINERS as it may never be that accurate.
>
> > I also think it's better to use a separate script like
> > scripts/spdxcheck.py and tie any necessary checkpatch
> > use to that script.
>
> Yes, checkpatch is getting pretty unwieldy.
>
> Rob
>
On Fri, Nov 16, 2018 at 03:33:17AM -0800, Mauro Carvalho Chehab wrote:
> Em Thu, 15 Nov 2018 21:35:20 +0200
> Leon Romanovsky <[email protected]> escreveu:
>
> > On Thu, Nov 15, 2018 at 11:09:34AM -0800, Mauro Carvalho Chehab wrote:
> > > Em Thu, 15 Nov 2018 18:20:08 +0200
> > > Leon Romanovsky <[email protected]> escreveu:
> > >
> > > > On Thu, Nov 15, 2018 at 06:10:36AM -0800, Mauro Carvalho Chehab wrote:
> > > > > Em Thu, 15 Nov 2018 09:03:11 +0100
> > > > > Geert Uytterhoeven <[email protected]> escreveu:
> > > > >
> > > > > > Hi Dan,
> > > > > >
> > > > > > On Thu, Nov 15, 2018 at 6:06 AM Dan Williams <[email protected]> wrote:
> > > > > > > Document the basic policies of the libnvdimm subsystem and provide a
> > > > > > > first example of a Subsystem Profile for others to duplicate and edit.
> > > > > > >
> > > > > > > Cc: Ross Zwisler <[email protected]>
> > > > > > > Cc: Vishal Verma <[email protected]>
> > > > > > > Cc: Dave Jiang <[email protected]>
> > > > > > > Signed-off-by: Dan Williams <[email protected]>
> > > > > >
> > > > > > Thanks for your patch!
> > > > > >
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/nvdimm/subsystem-profile.rst
> > > > > >
> > > > > > > +Trusted Reviewers
> > > > > > > +-----------------
> > > > > > > +Johannes Thumshirn
> > > > > > > +Toshi Kani
> > > > > > > +Jeff Moyer
> > > > > > > +Robert Elliott
> > > > > >
> > > > > > Don't you want to add email addresses?
> > > > > > Only the first one is listed in MAINTAINERS.
> > > > >
> > > > > IMO, it makes sense to have their e-mails here, in a way that it could
> > > > > easily be parsed by get_maintainers.pl.
> > > >
> > > > I personally think that list of "trusted reviewers" makes more harm than
> > > > good. It creates unneeded negative feelings to those who wanted to be in
> > > > this list, but for any reason they don't. Those reviewers will feel
> > > > "untrusted".
> > >
> > > Yeah, perhaps something like "most active reviewers" would sound
> > > better.
> >
> > I would recommend to remove this section at all.
> > New maintainers won't come out of blue, but will be come
> > from existing community and such individuals for sure will see
> > and judge by themselves to whom they trust and to whom not.
>
> I see your point, but, on the other hand, having a list with the ones
> that are actively doing reviews helps newcomers.
>
> I would keep, but perhaps it makes sense to add some notice about a
> criteria about how to be included at the "active reviewers" list,
> (the criteria probably belongs to the subsystem profile), e. g.
> something like:
>
> "Active reviewers are developers that contribute with more than 25
> patches per year and do more than 50 reviews per year on
> on patches written for drivers that they're not usual contributors"
Someone will need to maintain it and it looks to me like over complication.
I think that get_maintainer.pl already provides at least part of needed
information, but definitely not in very intuitive way.
It is worth to document get_maintainers.pl and update it to create the
list of "trusted reviewers".
Thanks
> Cheers,
> Mauro
On Fri, Nov 16, 2018 at 03:39:47AM -0800, Mauro Carvalho Chehab wrote:
> Em Thu, 15 Nov 2018 11:43:51 -0800
> Joe Perches <[email protected]> escreveu:
>
> > On Thu, 2018-11-15 at 19:40 +0000, Luck, Tony wrote:
> > > > I would recommend to remove this section at all.
> > > > New maintainers won't come out of blue, but will be come
> > > > from existing community and such individuals for sure will see
> > > > and judge by themselves to whom they trust and to whom not.
> > >
> > > Perhaps this is more of a hint to contributors than to maintainers
> > > (see earlier discussion on who is the target audience for these documents).
> > >
> > > It would help contributors know some names of useful reviewers (and
> > > thus this list should be picked up by scripts/get_maintainer.pl to help
> > > the user compose Cc: lists for e-mail patches).
> >
> > Trusted reviewers should be specifically listed
> > in the MAINTAINERS file with an "R:" entry.
> >
> > get_maintainers should not look anywhere else.
>
> I know that making get_maintainers to look elsewhere can make it more
> complex and slower, but IMHO, by having a per-subsystem profile, this is
> unavoidable.
>
> The thing is that touching at a single MAINTAINERS file every time a new
> reviewer comes is painful. Also, MAINTAINERS file format doesn't allow
> adding free text explaining the criteria for someone to become a
> reviewer.
You are pointing to the actual problem -> someone needs to maintain such
lists, Removal of persons from that list won't be easy task too.
>
> IMO, having reviewers on a per-subsystem file, where one could explain
> the criteria for being added there will make easier to attract more
> reviewers.
>
> Cheers,
> Mauro
Em Fri, 16 Nov 2018 10:57:14 -0800
Dan Williams <[email protected]> escreveu:
> On Fri, Nov 16, 2018 at 4:04 AM Mauro Carvalho Chehab
> <[email protected]> wrote:
> >
> > Em Thu, 15 Nov 2018 16:11:59 -0800
> > Frank Rowand <[email protected]> escreveu:
> [..]
> > > > +Patches or Pull requests
> > > > +------------------------
> > > > +Some subsystems allow contributors to send pull requests, most require
> > > > +mailed patches. State “Patches only”, or “Pull requests accepted”.
> > >
> > > This may create some confusion if only "Pull requests accepted" is the
> > > contents of this section. My understanding has been that all changes
> > > should be available on a mail list for review before being accepted
> > > by the maintainer, even if eventually the final version of the series
> > > that is accepted is applied by the maintainer as a pull instead of
> > > via applying patches.
> > >
> > > Is my "must appear on a mail list" understanding correct?
> >
> > I guess this is true on almost all subsystems that accept pull requests.
> >
> > It could not be true if some subsystem moves to Github/Gitlab.
>
> Yes. Maybe a "Review Forum" section for subsystems that have
> transitioned from email to a web-based tool? There's also the
> exception of security disclosures, but the expectations for those
> patches are already documented.
Maybe. I would postpone adding a section like that until some
subsystem maintainer that actually changed to Github/Gitlab
would submit his subsystem profile.
>
> > > > +Last -rc for new feature submissions
> > > > +------------------------------------
> > > > +New feature submissions targeting the next merge window should have
> > > > +their first posting for consideration before this point. Patches that
> > > > +are submitted after this point should be clear that they are targeting
> > > > +the NEXT+1 merge window, or should come with sufficient justification
> > > > +why they should be considered on an expedited schedule. A general
> > > > +guideline is to set expectation with contributors that new feature
> > > > +submissions should appear before -rc5. The answer may be different for
> > > > +'Core:' files, include a second entry prefixed with 'Core:' if so.
> > >
> > > I would expect the -rc to vary based on the patch series size, complexity,
> > > risk, number of revisions that will occur, how controversial, number of
> > > -rc releases before Linus chooses to release, etc. You would need a
> > > crystal ball to predict much of this. I could see being able to provide
> > > a good estimate of this value for a relatively simple patch.
> >
> > That's only partially true. I mean, the usual flux is to go up to -rc7.
> > After that, the final version is usually merged (except when something
> > goes wrong).
> >
> > Anyway, I guess nobody would complain if the maintainers do an exception
> > here and accept more patches when they know that the last rc version
> > won't be -rc7, but, instead, -rc8 or -rc9.
> >
> > This is a general ruleset that describes the usual behavior, telling the
> > developers the expected behavior. If the maintainers can do more on some
> > particular development cycle, it should be fine.
>
> Yes, and perhaps I should clarify that this is the point at which a
> maintainer will start to push back in the typical case, and indicate
> to a contributor that they are standing in exceptional territory.
> Similar to how later in the -rc series patches get increasing
> scrutiny.
Makes sense. There's one issue, though.
I don't expect developers to read the profile template, as this is a
material for the maintainer themselves. Developers should likely read
just the specific subsystem profile for the patches that will be submitted.
So, either each subsystem profile should have a reference to the
profile template, or need to copy some "invariant" texts (with would be
really painful to maintain).
>
> > > > +Last -rc to merge features
> > > > +--------------------------
> > > > +Indicate to contributors the point at which an as yet un-applied patch
> > > > +set will need to wait for the NEXT+1 merge window. Of course there is no
> > > > +obligation to ever except any given patchset, but if the review has not
> > > > +concluded by this point the expectation the contributor should wait and
> > > > +resubmit for the following merge window. The answer may be different for
> > > > +'Core:' files, include a second entry prefixed with 'Core:' if so.
> > >
> > > Same comment as for the previous section, with the additional complexity
> > > that each unique patch series should bake in -next.
>
> The intent is to try to leave all "should" or prescriptive statements
> out of the profile. I'm looking to target other parts of the handbook
> to carry advice for integrating with -next and suggested soak times.
Makes sense. Again, it probably makes sense to have those parts
referenced on each profile.
> > > > +Non-author Ack / Review Tags Required
> > > > +-------------------------------------
> > >
> > > The title of this section seems a bit different than the description
> > > below. A more aligned title would be something like "Maintainer
> > > self-commit review requirements".
>
> This is intended to go beyond maintainer self-commits. Consider a pull
> request that contains commits without non-author tags.
>
> > > > +Let contributors and other maintainers know whether they can expect to
> > > > +see the maintainer self-commit patches without 3rd-party review. Some
> > > > +subsystem developer communities are so small as to make this requirement
> > > > +impractical. Others may have been bootstrapped by a submission of
> > > > +self-reviewed code at the outset, but have since moved to a
> > > > +non-author review-required stance. This section sets expectations on the
> > > > +code-review economics in the subsystem. For example, can a contributor
> > > > +trade review of a maintainer's, or other contributor's patches in
> > > > +exchange for consideration of their own.
> > >
> > > Then another section (which is the one I expected when I slightly
> > > mis-read the section title) would be: Review Tags Requirements",
> > > which would discuss what type of review is expected, encouraged,
> > > or required.
>
> Per the clarification above, I think the whole thing should be called
> "Review Tags Requirement".
Agreed.
> > > > +Test Suite
> > > > +----------
> > > > +Indicate the test suite all patches are expected to pass before being
> > > > +submitted for inclusion consideration.
> > > > +
> > > > +
> > > > +Resubmit Cadence
> > > > +----------------
> > > > +Define a rate at which a contributor should wait to resubmit a patchset
> > > > +that has not yet received comments. A general guideline is to try to
> > > > +meet a deadline of 1 - 2 weeks to acknowledge starting consideration for
> > > > +a patch set.
> > >
> > > Or ping instead of resubmitting? If you resubmit the same series with
> > > an unchanged version then comments could be split across multiple
> > > email threads. If you resubmit the same series with a new version
> > > number, the same comment split can occur plus it can be confusing
> > > that two versions have the same contents. I suspect that there may be
> > > some maintainers who do prefer a resubmit, but that is just wild
> > > conjecture on my part.
> >
> > That depends on how maintainers handle patches. Those subsystems that
> > use patchwork (like media) don't really require anyone to resubmit
> > patches. They're all stored there at patchwork database.
> >
> > My workflow is that, if I receive two patches from the same person with
> > the same subject, I simply mark the first one as obsoleted, as usually
> > the second one is a new version, and reviewers should need some
> > time to handle it.
> >
> > In other words, re-sending a patch may actually delay its handling, as
> > the second version will be later on my input queue, and I'll grant
> > additional time for people to review the second version at the community.
>
> Ok, this sounds like a separate policy. How often to follow-up
> separated from resend the full series vs send a ping mail.
>
> > > > +Trusted Reviewers
> > > > +-----------------
> > > > +While a maintainer / maintainer-team is expected to be reviewer of last
> > > > +resort the review load is less onerous when distributed amongst
> > > > +contributors and or a trusted set of individuals. This section is
> > > > +distinct from the R: tag (Designated Reviewer). Whereas R: identifies
> > > > +reviewers that should always be copied on a patch submission, the
> > > > +trusted reviewers here are individuals contributors can reach out to if
> > > > +a few 'Resubmit Cadence' intervals have gone by without maintainer
> > > > +action, or to otherwise consult for advice.
> > >
> > > This seems redundant with the MAINTAINERS reviewers list. It seems like
> > > the role specified in this section is more of an ombudsman or developer
> > > advocate who can assist with the review and/or accept flow if the
> > > maintainer is being slow to respond.
> >
> > Well, on subsystems that have sub-maintainers, there's no way to point to
> > it at MAINTAINERS file.
> >
> > Also, not sure about others, but I usually avoid touching at existing
> > MAINTAINERS file entries. This is a file that everyone touches, so it
> > has higher chances of conflicts.
> >
> > Also, at least on media, we have 5 different API sets (digital TV, V4L2, CEC,
> > media controller, remote controller). Yet, all drivers are stored at the
> > same place (as a single driver may use multiple APIs).
> >
> > The reviewers for each API set are different. There isn't a good way
> > to explain that inside a MAINTANERS file.
>
> Would it be worthwhile to have separate Subsystem Profiles for those
> API reviewers? If they end up merging patches and sending them
> upstream might we need a hierarchy of profiles for each hop along the
> upstream merge path?
I guess having hierarchical profiles will make it very confusing.
The point is: inside a subsystem, the same ruleset usually applies to
everything.
In the case of media, it is not uncommon to have patches that require
multiple APIs. Consider, for example, a SoC used on a TV box. The driver
itself should be placed at drivers/media/platform/, but it will end by
being a bunch of sub-drivers that together will add support for V4L,
Digital TV, remote controller, CEC and codecs, and need to be controlled
via the media controller API. It may even have camera sensors.
On other words, all media APIs will be used (after having it fully
sent upstream).
In practice, drivers for complex hardware like that is submitted in
parts. For example, one SoC vendor started sending us the remote
controller driver (as it would be the simplest one).
The only part of the policy that changes, depending of what API
is involved, is the one that will do the review.
As the driver itself will be at the same place, no matter what APIs
are used, get_maintainers.pl is not capable of identifying who are
the reviewers based "F:" tags[1].
[1] It could be possible to teach get_maintainers to better hint it,
by making it look who are the reviewers for the headers that are
included.
>
> > > > +Time Zone / Office Hours
> > > > +------------------------
> > > > +Let contributors know the time of day when one or more maintainers are
> > > > +usually actively monitoring the mailing list.
> > >
> > > I would strike "actively monitoring the mailing list". To me, it should
> > > be what are the hours of the day that the maintainer might happen to poll
> > > (or might receive an interrupt) from the appropriate communications
> > > channels (could be IRC, could be email, etc).
>
> Yes, makes sense.
>
> > > For my area, I would want to say something like: I tend to be active
> > > between 17:00 UTC (18:00 UTC when daylight savings) and 25:00 (26:00),
> > > but often will check for urgent or brief items up until 07:00 (08:00).
> > > I interact with email via a poll model. I interact with IRC via a
> > > pull model and often overlook IRC activity for multiple days).
> >
> > Frankly, for media, I don't think that working hours makes sense. Media
> > (sub-)maintainers are spread around the globe, on different time zones
> > (in US, Brazil and Europe). We also have several active developers in
> > Japan, so we may end by having some day reviewers/sub-maintainers from
> > there.
>
> For that case just say:
>
> "the sun never sets on the media subsystem" ;-)
:-)
>
> > At max, we can say that we won't warrant to patches on weekends or holidays.
>
> Yeah, maybe:
>
> "outside of weekends or holidays there's usually a maintainer or
> reviewer monitoring the mailing list"
Well, 24/7, there is always patchwork monitoring the ML and picking
the patches. When the patch will be handled by someone is a different
question. As it is a high-traffic subsystem with an even higher ML
traffic, each sub-maintainer have its own policy about when they
review patches (usually one or twice per week - as most maintainers
are also active developers, and don't want to mix their development
time with reviewing time).
I'm not quite sure about what you expect with this specific part of
the profile.
I mean: why a submitter should care about office hours?
Also, people may be OOT during some period of time, or working
remotely from some other office.
Except if the idea would be to point to some site that would
dynamically track each maintainer's weekly maintainership
window (with would be a real pain to keep updated), I guess this
is useless.
Thanks,
Mauro
Em Fri, 16 Nov 2018 15:44:45 -0800
Dan Williams <[email protected]> escreveu:
> On Fri, Nov 16, 2018 at 12:37 PM Rodrigo Vivi <[email protected]> wrote:
> >
> > On Thu, Nov 15, 2018 at 8:38 AM Leon Romanovsky <[email protected]> wrote:
> > >
> > > On Thu, Nov 15, 2018 at 06:10:36AM -0800, Mauro Carvalho Chehab wrote:
> > > > Em Thu, 15 Nov 2018 09:03:11 +0100
> > > > Geert Uytterhoeven <[email protected]> escreveu:
> > > >
> > > > > Hi Dan,
> > > > >
> > > > > On Thu, Nov 15, 2018 at 6:06 AM Dan Williams <[email protected]> wrote:
> > > > > > Document the basic policies of the libnvdimm subsystem and provide a
> > > > > > first example of a Subsystem Profile for others to duplicate and edit.
> > > > > >
> > > > > > Cc: Ross Zwisler <[email protected]>
> > > > > > Cc: Vishal Verma <[email protected]>
> > > > > > Cc: Dave Jiang <[email protected]>
> > > > > > Signed-off-by: Dan Williams <[email protected]>
> > > > >
> > > > > Thanks for your patch!
> > > > >
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/nvdimm/subsystem-profile.rst
> > > > >
> > > > > > +Trusted Reviewers
> > > > > > +-----------------
> > > > > > +Johannes Thumshirn
> > > > > > +Toshi Kani
> > > > > > +Jeff Moyer
> > > > > > +Robert Elliott
> > > > >
> > > > > Don't you want to add email addresses?
> > > > > Only the first one is listed in MAINTAINERS.
> > > >
> > > > IMO, it makes sense to have their e-mails here, in a way that it could
> > > > easily be parsed by get_maintainers.pl.
> > >
> > > I personally think that list of "trusted reviewers" makes more harm than
> > > good. It creates unneeded negative feelings to those who wanted to be in
> > > this list, but for any reason they don't. Those reviewers will feel
> > > "untrusted".
> >
> > I'd like to +1 on this concern here. Besides leaving all the other
> > people demotivated.
>
> Yes, that's a valid concern, I overlooked that unfortunate interpretation.
>
> >
> > A small group of trusted reviewers doesn't scale. People will get overloaded.
> > Or you won't be able to enforce that all patches need to get Reviews.
> >
> > Reviews should be coming from everywhere and commiters and maintainers
> > deciding on what to trust or re-review.
> >
> > Also the list is hard to maintain and keep the lists updated.
>
> I understand the concern, and as I saw feedback come in I realized
> there were more people that I would add to that reviewer list for
> libnvdimm.
>
> Stepping back the end goal is to have an initial list of recommended
> people to follow up with directly to seek a second opinion, or help in
> cases where a contributor otherwise needs some direction / engagement
> that they are not readily receiving from the maintainer. Typically
> someone just lurks on the mailing list for a few weeks to get a feel
> for who the usual suspects are in the subsystem, but for a new
> contributor identifying those individuals may be difficult.
>
> One of the contributing factors of lack of response to a patchset is
> that they are sent with the implicit expectation that the maintainer
> will get to eventually, and typically other people feel content to sit
> back and watch. If instead a contributor sent a direct mail to a
> "trusted reviewer" saying, "Hey, Alice, Bob seems busy can you help me
> out?" that seems more likely to rope in additional review help.
Hmm.. Perhaps the subsystem profile should point to IRC channels if any?
Several subsystems use them in order to provide newbies some directions
and to discuss other development issues.
Thanks,
Mauro
Em Sat, 17 Nov 2018 11:38:50 +1100
NeilBrown <[email protected]> escreveu:
> On Fri, Nov 16 2018, Dan Williams wrote:
>
> > On Fri, Nov 16, 2018 at 12:37 PM Rodrigo Vivi <[email protected]> wrote:
> >>
> >> On Thu, Nov 15, 2018 at 8:38 AM Leon Romanovsky <[email protected]> wrote:
> >> >
> >> > On Thu, Nov 15, 2018 at 06:10:36AM -0800, Mauro Carvalho Chehab wrote:
> >> > > Em Thu, 15 Nov 2018 09:03:11 +0100
> >> > > Geert Uytterhoeven <[email protected]> escreveu:
> >> > >
> >> > > > Hi Dan,
> >> > > >
> >> > > > On Thu, Nov 15, 2018 at 6:06 AM Dan Williams <[email protected]> wrote:
> >> > > > > Document the basic policies of the libnvdimm subsystem and provide a
> >> > > > > first example of a Subsystem Profile for others to duplicate and edit.
> >> > > > >
> >> > > > > Cc: Ross Zwisler <[email protected]>
> >> > > > > Cc: Vishal Verma <[email protected]>
> >> > > > > Cc: Dave Jiang <[email protected]>
> >> > > > > Signed-off-by: Dan Williams <[email protected]>
> >> > > >
> >> > > > Thanks for your patch!
> >> > > >
> >> > > > > --- /dev/null
> >> > > > > +++ b/Documentation/nvdimm/subsystem-profile.rst
> >> > > >
> >> > > > > +Trusted Reviewers
> >> > > > > +-----------------
> >> > > > > +Johannes Thumshirn
> >> > > > > +Toshi Kani
> >> > > > > +Jeff Moyer
> >> > > > > +Robert Elliott
> >> > > >
> >> > > > Don't you want to add email addresses?
> >> > > > Only the first one is listed in MAINTAINERS.
> >> > >
> >> > > IMO, it makes sense to have their e-mails here, in a way that it could
> >> > > easily be parsed by get_maintainers.pl.
> >> >
> >> > I personally think that list of "trusted reviewers" makes more harm than
> >> > good. It creates unneeded negative feelings to those who wanted to be in
> >> > this list, but for any reason they don't. Those reviewers will feel
> >> > "untrusted".
> >>
> >> I'd like to +1 on this concern here. Besides leaving all the other
> >> people demotivated.
> >
> > Yes, that's a valid concern, I overlooked that unfortunate interpretation.
> >
> >>
> >> A small group of trusted reviewers doesn't scale. People will get overloaded.
> >> Or you won't be able to enforce that all patches need to get Reviews.
> >>
> >> Reviews should be coming from everywhere and commiters and maintainers
> >> deciding on what to trust or re-review.
> >>
> >> Also the list is hard to maintain and keep the lists updated.
> >
> > I understand the concern, and as I saw feedback come in I realized
> > there were more people that I would add to that reviewer list for
> > libnvdimm.
> >
> > Stepping back the end goal is to have an initial list of recommended
> > people to follow up with directly to seek a second opinion, or help in
> > cases where a contributor otherwise needs some direction / engagement
> > that they are not readily receiving from the maintainer. Typically
> > someone just lurks on the mailing list for a few weeks to get a feel
> > for who the usual suspects are in the subsystem, but for a new
> > contributor identifying those individuals may be difficult.
> >
> > One of the contributing factors of lack of response to a patchset is
> > that they are sent with the implicit expectation that the maintainer
> > will get to eventually, and typically other people feel content to sit
> > back and watch. If instead a contributor sent a direct mail to a
> > "trusted reviewer" saying, "Hey, Alice, Bob seems busy can you help me
> > out?" that seems more likely to rope in additional review help.
>
> In here is, I think, a real issue that listing "trusted reviewers" might
> help address.
> As you say, people don't feel the need to comment - particularly if they
> don't see anything wrong (often best to insert a bug to encourage
> responses!).
> Maybe if we list people, it will make them feel that their opinion is
> valuable (trusted!) and that will encourage them to Ack or Review a
> patch.
> I have found that being given a title of responsibility can change my
> thinking from "someone should" to "I should".
I heard a similar feedback from some subsystems: giving someone a
formal credit may actually help to get more reviews.
However, as Leon pointed later in this tread:
Em Sun, 18 Nov 2018 09:12:54 +0200
Leon Romanovsky <[email protected]> escreveu:
> On Fri, Nov 16, 2018 at 03:39:47AM -0800, Mauro Carvalho Chehab wrote:
> > Em Thu, 15 Nov 2018 11:43:51 -0800
> > Joe Perches <[email protected]> escreveu:
> >
> > > On Thu, 2018-11-15 at 19:40 +0000, Luck, Tony wrote:
> > > > > I would recommend to remove this section at all.
> > > > > New maintainers won't come out of blue, but will be come
> > > > > from existing community and such individuals for sure will see
> > > > > and judge by themselves to whom they trust and to whom not.
> > > >
> > > > Perhaps this is more of a hint to contributors than to maintainers
> > > > (see earlier discussion on who is the target audience for these documents).
> > > >
> > > > It would help contributors know some names of useful reviewers (and
> > > > thus this list should be picked up by scripts/get_maintainer.pl to help
> > > > the user compose Cc: lists for e-mail patches).
> > >
> > > Trusted reviewers should be specifically listed
> > > in the MAINTAINERS file with an "R:" entry.
> > >
> > > get_maintainers should not look anywhere else.
> >
> > I know that making get_maintainers to look elsewhere can make it more
> > complex and slower, but IMHO, by having a per-subsystem profile, this is
> > unavoidable.
> >
> > The thing is that touching at a single MAINTAINERS file every time a new
> > reviewer comes is painful. Also, MAINTAINERS file format doesn't allow
> > adding free text explaining the criteria for someone to become a
> > reviewer.
>
> You are pointing to the actual problem -> someone needs to maintain such
> lists, Removal of persons from that list won't be easy task too.
While adding new reviewers is easy (someone just need to send a patch,
with the Acked-by from the reviewer to be included), getting someone
removed from it can be very painful (and will likely require some written
policy about how to do that).
Thanks,
Mauro
On Sun, Nov 18, 2018 at 4:58 AM Mauro Carvalho Chehab
<[email protected]> wrote:
>
> Em Fri, 16 Nov 2018 10:57:14 -0800
> Dan Williams <[email protected]> escreveu:
> > On Fri, Nov 16, 2018 at 4:04 AM Mauro Carvalho Chehab
> > <[email protected]> wrote:
[..]
> > Yes. Maybe a "Review Forum" section for subsystems that have
> > transitioned from email to a web-based tool? There's also the
> > exception of security disclosures, but the expectations for those
> > patches are already documented.
>
> Maybe. I would postpone adding a section like that until some
> subsystem maintainer that actually changed to Github/Gitlab
> would submit his subsystem profile.
Sure.
> > > > > +Last -rc for new feature submissions
[..]
> > > This is a general ruleset that describes the usual behavior, telling the
> > > developers the expected behavior. If the maintainers can do more on some
> > > particular development cycle, it should be fine.
> >
> > Yes, and perhaps I should clarify that this is the point at which a
> > maintainer will start to push back in the typical case, and indicate
> > to a contributor that they are standing in exceptional territory.
> > Similar to how later in the -rc series patches get increasing
> > scrutiny.
>
> Makes sense. There's one issue, though.
>
> I don't expect developers to read the profile template, as this is a
> material for the maintainer themselves. Developers should likely read
> just the specific subsystem profile for the patches that will be submitted.
>
> So, either each subsystem profile should have a reference to the
> profile template, or need to copy some "invariant" texts (with would be
> really painful to maintain).
Agree, a general link back to the handbook template for clarification
on any of the sections seems sufficient.
[..]
> > > > > +Trusted Reviewers
> > > > > +-----------------
> > > > > +While a maintainer / maintainer-team is expected to be reviewer of last
> > > > > +resort the review load is less onerous when distributed amongst
> > > > > +contributors and or a trusted set of individuals. This section is
> > > > > +distinct from the R: tag (Designated Reviewer). Whereas R: identifies
> > > > > +reviewers that should always be copied on a patch submission, the
> > > > > +trusted reviewers here are individuals contributors can reach out to if
> > > > > +a few 'Resubmit Cadence' intervals have gone by without maintainer
> > > > > +action, or to otherwise consult for advice.
> > > >
> > > > This seems redundant with the MAINTAINERS reviewers list. It seems like
> > > > the role specified in this section is more of an ombudsman or developer
> > > > advocate who can assist with the review and/or accept flow if the
> > > > maintainer is being slow to respond.
> > >
> > > Well, on subsystems that have sub-maintainers, there's no way to point to
> > > it at MAINTAINERS file.
> > >
> > > Also, not sure about others, but I usually avoid touching at existing
> > > MAINTAINERS file entries. This is a file that everyone touches, so it
> > > has higher chances of conflicts.
> > >
> > > Also, at least on media, we have 5 different API sets (digital TV, V4L2, CEC,
> > > media controller, remote controller). Yet, all drivers are stored at the
> > > same place (as a single driver may use multiple APIs).
> > >
> > > The reviewers for each API set are different. There isn't a good way
> > > to explain that inside a MAINTANERS file.
> >
> > Would it be worthwhile to have separate Subsystem Profiles for those
> > API reviewers? If they end up merging patches and sending them
> > upstream might we need a hierarchy of profiles for each hop along the
> > upstream merge path?
>
> I guess having hierarchical profiles will make it very confusing.
> The point is: inside a subsystem, the same ruleset usually applies to
> everything.
Ok.
> In the case of media, it is not uncommon to have patches that require
> multiple APIs. Consider, for example, a SoC used on a TV box. The driver
> itself should be placed at drivers/media/platform/, but it will end by
> being a bunch of sub-drivers that together will add support for V4L,
> Digital TV, remote controller, CEC and codecs, and need to be controlled
> via the media controller API. It may even have camera sensors.
>
> On other words, all media APIs will be used (after having it fully
> sent upstream).
>
> In practice, drivers for complex hardware like that is submitted in
> parts. For example, one SoC vendor started sending us the remote
> controller driver (as it would be the simplest one).
>
> The only part of the policy that changes, depending of what API
> is involved, is the one that will do the review.
>
> As the driver itself will be at the same place, no matter what APIs
> are used, get_maintainers.pl is not capable of identifying who are
> the reviewers based "F:" tags[1].
>
> [1] It could be possible to teach get_maintainers to better hint it,
> by making it look who are the reviewers for the headers that are
> included.
>
> >
> > > > > +Time Zone / Office Hours
> > > > > +------------------------
> > > > > +Let contributors know the time of day when one or more maintainers are
> > > > > +usually actively monitoring the mailing list.
> > > >
> > > > I would strike "actively monitoring the mailing list". To me, it should
> > > > be what are the hours of the day that the maintainer might happen to poll
> > > > (or might receive an interrupt) from the appropriate communications
> > > > channels (could be IRC, could be email, etc).
> >
> > Yes, makes sense.
> >
> > > > For my area, I would want to say something like: I tend to be active
> > > > between 17:00 UTC (18:00 UTC when daylight savings) and 25:00 (26:00),
> > > > but often will check for urgent or brief items up until 07:00 (08:00).
> > > > I interact with email via a poll model. I interact with IRC via a
> > > > pull model and often overlook IRC activity for multiple days).
> > >
> > > Frankly, for media, I don't think that working hours makes sense. Media
> > > (sub-)maintainers are spread around the globe, on different time zones
> > > (in US, Brazil and Europe). We also have several active developers in
> > > Japan, so we may end by having some day reviewers/sub-maintainers from
> > > there.
> >
> > For that case just say:
> >
> > "the sun never sets on the media subsystem" ;-)
>
> :-)
>
> >
> > > At max, we can say that we won't warrant to patches on weekends or holidays.
> >
> > Yeah, maybe:
> >
> > "outside of weekends or holidays there's usually a maintainer or
> > reviewer monitoring the mailing list"
>
> Well, 24/7, there is always patchwork monitoring the ML and picking
> the patches. When the patch will be handled by someone is a different
> question. As it is a high-traffic subsystem with an even higher ML
> traffic, each sub-maintainer have its own policy about when they
> review patches (usually one or twice per week - as most maintainers
> are also active developers, and don't want to mix their development
> time with reviewing time).
>
> I'm not quite sure about what you expect with this specific part of
> the profile.
>
> I mean: why a submitter should care about office hours?
>
> Also, people may be OOT during some period of time, or working
> remotely from some other office.
>
> Except if the idea would be to point to some site that would
> dynamically track each maintainer's weekly maintainership
> window (with would be a real pain to keep updated), I guess this
> is useless.
True, will remove. What's the point of stating daily active hours when
we already have "Resubmit Cadence" (I think I'll rename it "Follow
Cadence") measured in multiple days / weeks.
On Fri, 16 Nov 2018, Joe Perches <[email protected]> wrote:
> On Fri, 2018-11-16 at 14:44 +0200, Jani Nikula wrote:
>> I quickly cooked up this script to produce the top-5 commit prefixes for
>> the given files over the arbitrary last 200 commits. It'll give you a
>> pretty good idea if you're even close.
>>
>> ---
>> #!/bin/sh
>> # usage: subject-prefix FILE [...]
>> # show top 5 subject prefixes for FILEs
>>
>> git log --format=%s -n 200 -- "$@" |\
>> grep -v "^Merge " |\
>> sed 's/\(.*\):.*/\1/' |\
>> sort | uniq -c | sort -nr | sed 's/ *[0-9]\+ //' |\
>> head -n 5
>> ---
>>
>> Someone who knows perl could turn that into a checkpatch check: See if
>> the patch subject prefix is one of the top-5 for all files changed by
>> the patch, and ask the user to double check if it isn't. Or some
>> heuristics thereof.
>
> This won't work when a patch contains multiple files
> from different paths, or even multiple files from a
> single driver.
*shrug*
You can give it multiple files as argument, and it'll give you an
approximation of what the prefix could be, whether you're way off or
not. Close enough at least for the single driver case. Obviously not
perfect, but hey, it took me all of five minutes to write that. ;)
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
On Sun, 18 Nov 2018, Mauro Carvalho Chehab <[email protected]> wrote:
> Hmm.. Perhaps the subsystem profile should point to IRC channels if any?
> Several subsystems use them in order to provide newbies some directions
> and to discuss other development issues.
MAINTAINERS:
C: URI for chat protocol, server and channel where developers
usually hang out, for example irc://server/channel.
Alas very few subsystems have added that.
I actually fear very few subsystems will add a subsystem profile for
that matter.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
On Tue, Nov 20, 2018 at 12:09 AM Jani Nikula <[email protected]> wrote:
>
> On Sun, 18 Nov 2018, Mauro Carvalho Chehab <[email protected]> wrote:
> > Hmm.. Perhaps the subsystem profile should point to IRC channels if any?
> > Several subsystems use them in order to provide newbies some directions
> > and to discuss other development issues.
>
> MAINTAINERS:
>
> C: URI for chat protocol, server and channel where developers
> usually hang out, for example irc://server/channel.
>
> Alas very few subsystems have added that.
>
> I actually fear very few subsystems will add a subsystem profile for
> that matter.
I'm not too worried about the adoption rate of the Subsystem Profile.
If a few subsystems pick it up and contributors find it useful then it
will naturally proliferate, if not then perhaps the initial concern
about contributor pain stumbling through the discovery of these local
policy details was misplaced.
On Wed 2018-11-14 20:53:13, Dan Williams wrote:
> At a recently concluded session at the Linux Plumbers Conference I
> proposed a "Subsystem Profile" as a document that a maintainer can
> provide to set contributor expectations and provide fodder for a
> discussion between maintainers about the merits of different maintainer
> policies.
>
> For those that did not attend, the goal of the Subsystem Profile, and the
> Maintainer Handbook more generally, is to provide a desk reference for
> maintainers both new and experienced. The session introduction was:
>
> The first rule of kernel maintenance is that there are no hard and
> fast rules. That state of affairs is both a blessing and a curse. It
> has served the community well to be adaptable to the different
> people and different problem spaces that inhabit the kernel
> community. However, that variability also leads to inconsistent
> experiences for contributors, little to no guidance for new
> contributors, and unnecessary stress on current maintainers. There
> are quite a few of people who have been around long enough to make
> enough mistakes that they have gained some hard earned proficiency.
> However if the kernel community expects to keep growing it needs to
> be able both scale the maintainers it has and ramp new ones without
> necessarily let them make a decades worth of mistakes to learn the
> ropes.
>
> To be clear, the proposed document does not impose or suggest new
> rules. Instead it provides an outlet to document the unwritten rules
> and policies in effect for each subsystem, and that each subsystem
> might decide differently for whatever reason.
Sounds like a new rules to me :-(, making submitting simple patches
harder.
It would be good if the rules were similar / same accross the
subsystems, documenting "it is okay to be different" is not really helpful.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sun, Nov 25, 2018 at 2:58 AM Pavel Machek <[email protected]> wrote:
>
> On Wed 2018-11-14 20:53:13, Dan Williams wrote:
> > At a recently concluded session at the Linux Plumbers Conference I
> > proposed a "Subsystem Profile" as a document that a maintainer can
> > provide to set contributor expectations and provide fodder for a
> > discussion between maintainers about the merits of different maintainer
> > policies.
> >
> > For those that did not attend, the goal of the Subsystem Profile, and the
> > Maintainer Handbook more generally, is to provide a desk reference for
> > maintainers both new and experienced. The session introduction was:
> >
> > The first rule of kernel maintenance is that there are no hard and
> > fast rules. That state of affairs is both a blessing and a curse. It
> > has served the community well to be adaptable to the different
> > people and different problem spaces that inhabit the kernel
> > community. However, that variability also leads to inconsistent
> > experiences for contributors, little to no guidance for new
> > contributors, and unnecessary stress on current maintainers. There
> > are quite a few of people who have been around long enough to make
> > enough mistakes that they have gained some hard earned proficiency.
> > However if the kernel community expects to keep growing it needs to
> > be able both scale the maintainers it has and ramp new ones without
> > necessarily let them make a decades worth of mistakes to learn the
> > ropes.
> >
> > To be clear, the proposed document does not impose or suggest new
> > rules. Instead it provides an outlet to document the unwritten rules
> > and policies in effect for each subsystem, and that each subsystem
> > might decide differently for whatever reason.
>
> Sounds like a new rules to me :-(, making submitting simple patches
> harder.
I'm missing how documenting how a subsystem generally handles patches
makes submitting simple patches harder?
> It would be good if the rules were similar / same accross the
> subsystems, documenting "it is okay to be different" is not really helpful.
It is not documenting "it is okay to be different", it is
acknowledging that processes are already different. The goals are to
allow comparing notes across subsystems with the hope of some
convergence down the road, provide a template for new maintainers to
copy rather than reinvent, and give submitters a resource to discover
subsystem local policy.
Hi Jani,
Em Tue, 20 Nov 2018 10:10:25 +0200
Jani Nikula <[email protected]> escreveu:
> On Sun, 18 Nov 2018, Mauro Carvalho Chehab <[email protected]> wrote:
> > Hmm.. Perhaps the subsystem profile should point to IRC channels if any?
> > Several subsystems use them in order to provide newbies some directions
> > and to discuss other development issues.
>
> MAINTAINERS:
>
> C: URI for chat protocol, server and channel where developers
> usually hang out, for example irc://server/channel.
>
> Alas very few subsystems have added that.
>
> I actually fear very few subsystems will add a subsystem profile for
> that matter.
Sorry for a late reply. In the case of the media subsystem, we
actually use 2 different channels, one when the matter is related
to digital TV (#linuxtv) and another one for the rest (#v4l), both
at freenode.
That's something that wouldn't fit well into MAINTAINERS file, as
we can't explain what should be used for each.
So, the way it is it won't work well for us.
Thanks,
Mauro
On Mon, 2018-11-26 at 09:12 -0200, Mauro Carvalho Chehab wrote:
> Hi Jani,
>
> Em Tue, 20 Nov 2018 10:10:25 +0200
> Jani Nikula <[email protected]> escreveu:
>
> > On Sun, 18 Nov 2018, Mauro Carvalho Chehab <[email protected]> wrote:
> > > Hmm.. Perhaps the subsystem profile should point to IRC channels if any?
> > > Several subsystems use them in order to provide newbies some directions
> > > and to discuss other development issues.
> >
> > MAINTAINERS:
> >
> > C: URI for chat protocol, server and channel where developers
> > usually hang out, for example irc://server/channel.
> >
> > Alas very few subsystems have added that.
> >
> > I actually fear very few subsystems will add a subsystem profile for
> > that matter.
>
> Sorry for a late reply. In the case of the media subsystem, we
> actually use 2 different channels, one when the matter is related
> to digital TV (#linuxtv) and another one for the rest (#v4l), both
> at freenode.
>
> That's something that wouldn't fit well into MAINTAINERS file, as
> we can't explain what should be used for each.
>
> So, the way it is it won't work well for us.
So maybe add a comment to both like:
C: <URI1> # only for digital tv #linuxtv
C: <URI2> # everything else