Changes since v1 [1]:
- Simplify the profile to a hopefully non-controversial set of
attributes that address the most common sources of contributor
confusion, or maintainer frustration.
- Rename "Subsystem Profile" to "Maintainer Entry Profile". Not every
entry in MAINTAINERS represents a full subsystem. There may be driver
local considerations to communicate to a submitter in addition to wider
subsystem guidelines.
- Delete the old P: tag in MAINTAINERS rather than convert to a new E:
tag (Joe Perches).
[1]: http://lore.kernel.org/r/154225759358.2499188.15268218778137905050.stgit@dwillia2-desk3.amr.corp.intel.com
---
At last years Plumbers Conference I proposed the Maintainer Entry
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 Maintainer Entry 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 Maintainer Entry Profile
Maintainer Handbook: Maintainer Entry Profile
libnvdimm, MAINTAINERS: Maintainer Entry Profile
Documentation/maintainer/index.rst | 1
.../maintainer/maintainer-entry-profile.rst | 99 ++++++++++++++++++++
Documentation/nvdimm/maintainer-entry-profile.rst | 64 +++++++++++++
MAINTAINERS | 20 ++--
4 files changed, 175 insertions(+), 9 deletions(-)
create mode 100644 Documentation/maintainer/maintainer-entry-profile.rst
create mode 100644 Documentation/nvdimm/maintainer-entry-profile.rst
Document the basic policies of the libnvdimm subsystem and provide a first
example of a Maintainer Entry Profile for others to duplicate and edit.
Cc: Vishal Verma <[email protected]>
Cc: Dave Jiang <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
Documentation/nvdimm/maintainer-entry-profile.rst | 64 +++++++++++++++++++++
MAINTAINERS | 4 +
2 files changed, 68 insertions(+)
create mode 100644 Documentation/nvdimm/maintainer-entry-profile.rst
diff --git a/Documentation/nvdimm/maintainer-entry-profile.rst b/Documentation/nvdimm/maintainer-entry-profile.rst
new file mode 100644
index 000000000000..c102950f2257
--- /dev/null
+++ b/Documentation/nvdimm/maintainer-entry-profile.rst
@@ -0,0 +1,64 @@
+LIBNVDIMM Maintainer Entry Profile
+==================================
+
+Overview
+--------
+The libnvdimm subsystem manages persistent memory across multiple
+architectures. The mailing list, is tracked by patchwork here:
+https://patchwork.kernel.org/project/linux-nvdimm/list/
+...and that instance is configured to give feedback to submitters on
+patch acceptance and upstream merge. Patches are merged to either the
+'libnvdimm-fixes', or 'libnvdimm-for-next' branch. Those branches are
+available here:
+https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/
+
+In general patches can be submitted against the latest -rc, however if
+the incoming code change is dependent on other pending changes then the
+patch should be based on the libnvdimm-for-next branch. However, since
+persistent memory sits at the intersection of storage and memory there
+are cases where patches are more suitable to be merged through a
+Filesystem or the Memory Management tree. When in doubt copy the nvdimm
+list and the maintainers will help route.
+
+Submissions will be exposed to the kbuild robot for compile regression
+testing. It helps to get a success notification from that infrastructure
+before submitting, but it is not required.
+
+
+Submit Checklist Addendum
+-------------------------
+There are unit tests for the subsystem via the ndctl utility:
+https://github.com/pmem/ndctl
+Those tests need to be passed before the patches go upstream, but not
+necessarily before initial posting. Contact the list if you need help
+getting the test environment set up.
+
+
+Key Cycle Dates
+---------------
+New submissions can be sent at any time, but if they intend to hit the
+next merge window they should be sent before -rc4, and ideally
+stabilized in the libnvdimm-for-next branch by -rc6. Of course if a
+patch set requires more than 2 weeks of review -rc4 is already too late
+and some patches may require multiple development cycles to review.
+
+
+Coding Style Addendum
+---------------------
+libnvdimm expects multi-line statements to be double indented. I.e.
+
+ if (x...
+ && ...y) {
+
+
+Review Cadence
+--------------
+In general, please wait up to one week before pinging for feedback. A
+private mail reminder is preferred. Alternatively ask for other
+developers that have Reviewed-by tags for libnvdimm changes to take a
+look and offer their opinion.
+
+
+Style Cleanup Patches
+---------------------
+Standalone style-cleanups are welcome.
diff --git a/MAINTAINERS b/MAINTAINERS
index e5d111a86e61..2be1e18a368e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9185,6 +9185,7 @@ M: Dan Williams <[email protected]>
M: Vishal Verma <[email protected]>
M: Dave Jiang <[email protected]>
L: [email protected]
+P: Documentation/nvdimm/maintainer-entry-profile.rst
Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
S: Supported
F: drivers/nvdimm/blk.c
@@ -9195,6 +9196,7 @@ M: Vishal Verma <[email protected]>
M: Dan Williams <[email protected]>
M: Dave Jiang <[email protected]>
L: [email protected]
+P: Documentation/nvdimm/maintainer-entry-profile.rst
Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
S: Supported
F: drivers/nvdimm/btt*
@@ -9204,6 +9206,7 @@ M: Dan Williams <[email protected]>
M: Vishal Verma <[email protected]>
M: Dave Jiang <[email protected]>
L: [email protected]
+P: Documentation/nvdimm/maintainer-entry-profile.rst
Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
S: Supported
F: drivers/nvdimm/pmem*
@@ -9223,6 +9226,7 @@ M: Dave Jiang <[email protected]>
M: Keith Busch <[email protected]>
M: Ira Weiny <[email protected]>
L: [email protected]
+P: Documentation/nvdimm/maintainer-entry-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 delete the others that do not include an
email address. The P: tag will be used to indicate the location of a
Profile for a given MAINTAINERS entry.
Cc: Joe Perches <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
MAINTAINERS | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index e7a47b5210fd..3f171339df53 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -76,7 +76,6 @@ trivial patch so apply some common sense.
Descriptions of section entries:
- P: Person (obsolete)
M: Mail patches to: FullName <address@domain>
R: Designated reviewer: FullName <address@domain>
These reviewers should be CCed on patches.
@@ -811,7 +810,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
@@ -10085,7 +10084,6 @@ F: drivers/staging/media/tegra-vde/
MEDIA INPUT INFRASTRUCTURE (V4L/DVB)
M: Mauro Carvalho Chehab <[email protected]>
-P: LinuxTV.org Project
L: [email protected]
W: https://linuxtv.org
Q: http://patchwork.kernel.org/project/linux-media/list/
@@ -13452,7 +13450,6 @@ S: Maintained
F: arch/mips/ralink
RALINK RT2X00 WIRELESS LAN DRIVER
-P: rt2x00 project
M: Stanislaw Gruszka <[email protected]>
M: Helmut Schaa <[email protected]>
L: [email protected]
@@ -13787,7 +13784,6 @@ S: Supported
F: drivers/net/ethernet/rocker/
ROCKETPORT DRIVER
-P: Comtrol Corp.
W: http://www.comtrol.com
S: Maintained
F: Documentation/driver-api/serial/rocket.rst
@@ -14668,15 +14664,13 @@ 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]>
+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]>
+M: Vincent Sanders <[email protected]>
M: Simtec Linux Team <[email protected]>
W: http://www.simtec.co.uk/products/EB2410ITX/
S: Supported
As presented at the 2018 Linux Plumbers conference [1], the Maintainer
Entry Profile (formerly Subsystem Profile) is proposed as a way to reduce
friction between committers and maintainers and encourage conversations
amongst maintainers about common best practices. While coding-style,
submit-checklist, and submitting-drivers lay out some common expectations
there remain local customs and maintainer preferences that vary by
subsystem.
The profile contains short answers to some of the common policy questions a
contributor might have that are local to the subsystem / device-driver, or
otherwise not covered by the top-level process documents.
Overview: General introduction to how the subsystem operates
Submit Checklist Addendum: Mechanical items that gate submission staging
Key Cycle Dates:
- Last -rc for new feature submissions: Expected lead time for submissions
- Last -rc to merge features: Deadline for merge decisions
Coding Style Addendum: Clarifications of local style preferences
Resubmit Cadence: When to ping the maintainer
Checkpatch / Style Cleanups: Policy on pure cleanup patches
See Documentation/maintainer/maintainer-entry-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]>
Cc: Alexandre Belloni <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
Documentation/maintainer/index.rst | 1
.../maintainer/maintainer-entry-profile.rst | 99 ++++++++++++++++++++
MAINTAINERS | 4 +
3 files changed, 104 insertions(+)
create mode 100644 Documentation/maintainer/maintainer-entry-profile.rst
diff --git a/Documentation/maintainer/index.rst b/Documentation/maintainer/index.rst
index 56e2c09dfa39..d904e74e1159 100644
--- a/Documentation/maintainer/index.rst
+++ b/Documentation/maintainer/index.rst
@@ -12,4 +12,5 @@ additions to this manual.
configure-git
rebasing-and-merging
pull-requests
+ maintainer-entry-profile
diff --git a/Documentation/maintainer/maintainer-entry-profile.rst b/Documentation/maintainer/maintainer-entry-profile.rst
new file mode 100644
index 000000000000..aaedf4d6dbf7
--- /dev/null
+++ b/Documentation/maintainer/maintainer-entry-profile.rst
@@ -0,0 +1,99 @@
+.. _maintainerentryprofile:
+
+Maintainer Entry Profile
+========================
+
+The Maintainer Entry Profile supplements the top-level process documents
+(coding-style, submitting-patches...) with subsystem/device-driver-local
+customs as well as details about the patch submission lifecycle. A
+contributor uses this document to level set their expectations and avoid
+common mistakes, maintainers may use these profiles to look across
+subsystems for opportunities to converge on common practices.
+
+
+Overview
+--------
+Provide an introduction to how the subsystem operates. While MAINTAINERS
+tells the contributor where to send patches for which files, it does not
+convey other subsystem-local infrastructure and mechanisms that aid
+development.
+Example questions to consider:
+- Are there notifications when patches are applied to the local tree, or
+ merged upstream?
+- Does the subsystem have a patchwork instance?
+- Any bots or CI infrastructure that watches the list?
+- Git branches that are pulled into -next?
+- What branch should contributors submit against?
+- Links to any other Maintainer Entry Profiles? For example a
+ device-driver may point to an entry for its parent subsystem. This makes
+ the contributor aware of obligations a maintainer may have have for
+ other maintainers in the submission chain.
+
+
+Submit Checklist Addendum
+-------------------------
+List mandatory and advisory criteria, beyond the common "submit-checklist",
+for a patch to be considered healthy enough for maintainer attention.
+For example: "pass checkpatch.pl with no errors, or warning. Pass the
+unit test detailed at $URI".
+
+
+Key Cycle Dates
+---------------
+One of the common misunderstandings of submitters is that patches can be
+sent at any time before the merge window closes and can still be
+considered for the next -rc1. The reality is that most patches need to
+be settled in soaking in linux-next in advance of the merge window
+opening. Clarify for the submitter the key dates (in terms rc release
+week) that patches might considered for merging and when patches need to
+wait for the next -rc. At a minimum:
+- 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.
+
+- Last -rc to merge features: Deadline for merge decisions
+ 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.
+
+Optional:
+- First -rc at which the development baseline branch, listed in the
+ overview section, should be considered ready for new submissions.
+
+
+Coding Style Addendum
+---------------------
+While the top-level "coding-style" document covers most style
+considerations there are still discrepancies and local preferences
+across subsystems. If a submitter should be aware of incremental / local
+style guidelines like x-mas tree variable declarations, indentation
+for multi line statements, function definition style, etc., list them
+here.
+
+
+Review Cadence
+--------------
+One of the largest sources of contributor angst is how soon to ping
+after a patchset has been posted without receiving any feedback. In
+addition to specifying how long to wait before a resubmission this
+section can also indicate a preferred style of update like, resend the
+full series, or privately send a reminder email. This section might also
+list how review works for this code area and methods to get feedback
+that are not directly from the maintainer.
+
+
+Style Cleanup Patches
+---------------------
+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".
diff --git a/MAINTAINERS b/MAINTAINERS
index 3f171339df53..e5d111a86e61 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -98,6 +98,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 more details submitting
+ patches to the given subsystem. This is either an in-tree file,
+ or a URI. See Documentation/maintainer/maintainer-entry-profile.rst
+ for details.
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
Hi Dan!
> At last years Plumbers Conference I proposed the Maintainer Entry
> 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.
This looks pretty good to me.
After the Plumbers session last year I wrote this for SCSI based on a
prior version by Christoph. It's gone a bit stale but I'll update it to
match your template.
--
Martin K. Petersen Oracle Linux Engineering
================================================
Linux SCSI Subsystem Patch Submission Guidelines
================================================
Martin K. Petersen <[email protected]>
Release cycles and when to submit patches
=========================================
Each release cycle consists of:
* an 8 week submission window in which new core features and driver updates
are added (scsi-queue)
* a 2 week merge window where it is customary not to post patches
* an 8 week stabilization window before final release where only bug fix
patches are merged (scsi-fixes)
The submission/stabilization cycle may be shorter or longer than 8 weeks.
However, 8 weeks is the most common. Note that the code *submission window*
for the *next* kernel release overlaps with the *stabilization window* for the
*current* release:
+-------------+---------------------------------+-------------+-----------------
| Week 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12 | 13 | 14 | .....
+-------------+---------------------------------+-------------+-----------------
+-------------+---------------------------------+-------------+
| 4.x merge | 4.x stabilization window | 4.x release |
| window | rc1 rc2 rc3 rc4 rc5 rc6 rc7 rc8 | |
+-------------+---------------------------------+-------------+
| Merge fixes | Big bug fixes.............small | Stable |
+-------------+---------------------------------+-------------+
+---------------------------------+-------------+-----------------
| 4.x+1 submission window | 4.x+1 merge | 4.x+1 stabili-
| | window | zation window
| | | rc1 rc2 rc3 ...
+---------------------------------+-------------+-----------------
| Big features/updates......small | Merge fixes | Big bug fixes..
+---------------------------------+-------------+-----------------
Bug Fixes (4.x/scsi-fixes)
~~~~~~~~~~~~~~~~~~~~~~~~~~
During the two week merge window only fixes related to the merge process or
any critical functional deficiences discovered in the newly submitted code
will be merged.
During the subsequent stabilization window (rc cycle) the expectation is that
bug fix patches will become increasingly smaller and simpler. In other words:
There may be enough fallout from a merged driver update to justify sending a
5-patch remedial bug fix series during rc1/rc2. But at the end of the rc cycle
patches must be trivial one-liners.
After the final 4.x has been released, subsequent bug fixes will have to go
through the stable-kernel-rules.rst process.
New Features/Core Code Changes/Driver Updates (4.x+1/scsi-queue)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
New features for 4.x+1 should be sent to linux-scsi once the merge window is
over. I.e. once rc1 has been released. This ensures there is enough time to
undergo several review cycles before the submission window is closed.
At the end of the submission window (rc7/rc8) the expectation is that posted
patches will be small and fairly simple. No patch series.
Only critical patches are merged during the last (rc8) submission window week
to ensure sufficient zeroday test robot and linux-next coverage before sending
Linus the final pull request.
GIT Trees
=========
The SCSI patch submission trees can be found at:
git://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git
There are two concurrently active branches:
* 4.x/scsi-fixes for bug fixes to the current kernel rc
* 4.x+1/scsi-queue for new features and driver submissions for the next merge
window
Both trees are usually based on Linus' 4.x-rc1.
Patches accepted into 4.x/scsi-fixes are typically submitted to Linus on a
weekly basis. Patches accepted into 4.x+1/scsi-queue are submitted at the
beginning of the merge window.
It is sometimes necessary to set up a separate 4.x+1/scsi-postmerge branch
which contains patches that depend on newly merged functionality in other
subsystems such as block or libata. The postmerge tree will be submitted to
Linus at the end of the merge window once all external dependencies have been
merged.
Patch Submission Process
========================
* All modifications to files under drivers/scsi should go through the SCSI
tree.
* Read Documentation/process/submitting-patches.rst
* Send the patch or patch series to [email protected]
* Please make sure to Cc: the maintainer of the driver/component you are
changing.
* Do not use custom To: and Cc: for individual patches. We want to see the
whole series, even patches that potentially need to go through a different
subsystem tree.
* Clearly indicate in the introductory letter which tree your submission is
aimed at (scsi-fixes or scsi-queue). For individual patches you can indicate
the desired tree below the --- separator.
* As a rule of thumb, any patch that goes through the scsi-fixes tree should
have a stable tag unless it was a regression introduced in the current
release.
* If the patch is a bugfix, please add:
Fixes: 1234deadbeef ("Implement foobarbaz")
Cc: <[email protected]> # v4.x+
Where the v4.x indicates when commit 1234deadbeef was added to the kernel.
* When posting a revised patch or series, please manually add any Acked-by:,
Reviewed-by:, or Tested-by: tags you may have received. Tags are only valid
if the patch has not changed significantly. Rebasing without conflicts and
typo fixes are generally OK and do not invalidate tags.
* Please CC: everyone that provided feedback when posting a revised
patch or series.
* Custom patch number formatting often confuses patchwork. Please use git
send-email -v instead of manually adding version numbers to individual
patches. To send version 3 of a 10 patch series from the tip of the current
tree you can do:
git send-email -10 -v3 --compose [email protected]
* Please put a change log after a --- separator in each revised patch. If the
reason for the repost is global (i.e. a rebase) it is fine to put the
changelog in the cover letter.
Patch Acceptance Criteria
=========================
* A patch needs two positive reviews (non-author Signed-off-by:, Acked-by:,
Reviewed-by:, or Tested-by:).
* A Tested-by: is worth a thousand reviews.
* For drivers, at least one review is required from somebody outside the
submitting company.
* Nobody has expressed any concerns about the patch on linux-scsi.
* The patch must apply cleanly. Use checkpatch and git send-email.
* The patch must compile without warnings (make C=1 CF="-D__CHECK_ENDIAN__")
and does not incur any zeroday test robot complaints.
* The patch must have a commit message that describes, comprehensively and in
plain English, what the patch does.
- Do not link to vendor bugzilla entries that require special access
credentials. If there is anything of importance in bugzilla, put it in the
commit message.
- The bigger and more intrusive the patch, the bigger the accompanying
commit message needs to be. "fooscsi: Add PCI id for model XYZ" is fine as
a one-liner. "fooscsi: Rewrite DMA allocation and queue setup" most likely
needs several paragraphs of discussion in the commit message.
Patch Review Communities
========================
While core SCSI folks generally perform a cursory review of every patch,
it is imperative that contributors with an interest in a particular
component form small communities that can review and test each other's
code. Such informal communities exist for several components or
sub-subsystems underneath the SCSI tree.
The best way to facilitate that your patch gets merged is to review patches
submitted by your fellow contributors and hope that they return the favor.
Patch Reviewer Responsibilities
===============================
If you provide feedback to a patch and the patch author addresses your
concerns in a new version, you are expected to respond to the new
patch. Many code contributions series fail to make forward progress
because reviewers do not ack or nack the changes they requested to be
made. Please make sure to always follow up when your review comments are
being addressed.
Driver Maintainer Responsibilities
==================================
Many of the actively developed SCSI subsystem drivers have one or more
official maintainers. These maintainers are often employed by the company
manufacturing the hardware in question and therefore have a vested interest in
making sure the driver is working correctly.
A driver maintainer must review and respond to *all submitted patches* to the
driver in a timely manner (5 business days). This includes:
* Proposed functional changes to the driver code
* Security fixes
* Trivial and typo patches
* Changes compelled by kernel interface changes
* Patches required to work correctly on architectures not commercially
supported by the hardware manufacturer
Patch Acceptance Status
=======================
* Submitted patches and their current status can be viewed in patchwork at:
https://patchwork.kernel.org/project/linux-scsi/list/
* If a submitted patch is not visible in patchwork, it does not exist. Please
repost.
* If only a portion of a patch series visible in patchwork, the series does
not exist. Please repost.
* Once a patch has been merged, you will receive a mail indicating into which
tree it has been accepted.
* Patches that have not received sufficient Acked-by:, Reviewed-by:, or
Tested-by: tags after two weeks get moved to "Deferred" status. This
indicates that it necessary to resubmit the patches to the linux-scsi list.
* Patches submitted to linux-scsi but which need to go through another
subsystem tree will be set to "Not Applicable" status.
* Patches which require rework will be set to "Changes Requested" status.
* Patches which have been obsoleted by a later version will be set to
"Superceded" status.
On 9/11/19 8:48 AM, Dan Williams wrote:
> Document the basic policies of the libnvdimm subsystem and provide a first
> example of a Maintainer Entry Profile for others to duplicate and edit.
>
> Cc: Vishal Verma <[email protected]>
> Cc: Dave Jiang <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
Acked-by: Dave Jiang <[email protected]>
> ---
> Documentation/nvdimm/maintainer-entry-profile.rst | 64 +++++++++++++++++++++
> MAINTAINERS | 4 +
> 2 files changed, 68 insertions(+)
> create mode 100644 Documentation/nvdimm/maintainer-entry-profile.rst
>
> diff --git a/Documentation/nvdimm/maintainer-entry-profile.rst b/Documentation/nvdimm/maintainer-entry-profile.rst
> new file mode 100644
> index 000000000000..c102950f2257
> --- /dev/null
> +++ b/Documentation/nvdimm/maintainer-entry-profile.rst
> @@ -0,0 +1,64 @@
> +LIBNVDIMM Maintainer Entry Profile
> +==================================
> +
> +Overview
> +--------
> +The libnvdimm subsystem manages persistent memory across multiple
> +architectures. The mailing list, is tracked by patchwork here:
> +https://patchwork.kernel.org/project/linux-nvdimm/list/
> +...and that instance is configured to give feedback to submitters on
> +patch acceptance and upstream merge. Patches are merged to either the
> +'libnvdimm-fixes', or 'libnvdimm-for-next' branch. Those branches are
> +available here:
> +https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/
> +
> +In general patches can be submitted against the latest -rc, however if
> +the incoming code change is dependent on other pending changes then the
> +patch should be based on the libnvdimm-for-next branch. However, since
> +persistent memory sits at the intersection of storage and memory there
> +are cases where patches are more suitable to be merged through a
> +Filesystem or the Memory Management tree. When in doubt copy the nvdimm
> +list and the maintainers will help route.
> +
> +Submissions will be exposed to the kbuild robot for compile regression
> +testing. It helps to get a success notification from that infrastructure
> +before submitting, but it is not required.
> +
> +
> +Submit Checklist Addendum
> +-------------------------
> +There are unit tests for the subsystem via the ndctl utility:
> +https://github.com/pmem/ndctl
> +Those tests need to be passed before the patches go upstream, but not
> +necessarily before initial posting. Contact the list if you need help
> +getting the test environment set up.
> +
> +
> +Key Cycle Dates
> +---------------
> +New submissions can be sent at any time, but if they intend to hit the
> +next merge window they should be sent before -rc4, and ideally
> +stabilized in the libnvdimm-for-next branch by -rc6. Of course if a
> +patch set requires more than 2 weeks of review -rc4 is already too late
> +and some patches may require multiple development cycles to review.
> +
> +
> +Coding Style Addendum
> +---------------------
> +libnvdimm expects multi-line statements to be double indented. I.e.
> +
> + if (x...
> + && ...y) {
> +
> +
> +Review Cadence
> +--------------
> +In general, please wait up to one week before pinging for feedback. A
> +private mail reminder is preferred. Alternatively ask for other
> +developers that have Reviewed-by tags for libnvdimm changes to take a
> +look and offer their opinion.
> +
> +
> +Style Cleanup Patches
> +---------------------
> +Standalone style-cleanups are welcome.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e5d111a86e61..2be1e18a368e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9185,6 +9185,7 @@ M: Dan Williams <[email protected]>
> M: Vishal Verma <[email protected]>
> M: Dave Jiang <[email protected]>
> L: [email protected]
> +P: Documentation/nvdimm/maintainer-entry-profile.rst
> Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
> S: Supported
> F: drivers/nvdimm/blk.c
> @@ -9195,6 +9196,7 @@ M: Vishal Verma <[email protected]>
> M: Dan Williams <[email protected]>
> M: Dave Jiang <[email protected]>
> L: [email protected]
> +P: Documentation/nvdimm/maintainer-entry-profile.rst
> Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
> S: Supported
> F: drivers/nvdimm/btt*
> @@ -9204,6 +9206,7 @@ M: Dan Williams <[email protected]>
> M: Vishal Verma <[email protected]>
> M: Dave Jiang <[email protected]>
> L: [email protected]
> +P: Documentation/nvdimm/maintainer-entry-profile.rst
> Q: https://patchwork.kernel.org/project/linux-nvdimm/list/
> S: Supported
> F: drivers/nvdimm/pmem*
> @@ -9223,6 +9226,7 @@ M: Dave Jiang <[email protected]>
> M: Keith Busch <[email protected]>
> M: Ira Weiny <[email protected]>
> L: [email protected]
> +P: Documentation/nvdimm/maintainer-entry-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
>
On Wed, Sep 11, 2019 at 08:48:59AM -0700, Dan Williams wrote:
> +Coding Style Addendum
> +---------------------
> +libnvdimm expects multi-line statements to be double indented. I.e.
> +
> + if (x...
> + && ...y) {
That looks horrible and it causes a checkpatch warning. :( Why not
do it the same way that everyone else does it.
if (blah_blah_x && <-- && has to be on the first line for checkpatch
blah_blah_y) { <-- [tab][space][space][space][space]blah
Now all the conditions are aligned visually which makes it readable.
They aren't aligned with the indent block so it's easy to tell the
inside from the if condition.
I kind of hate all this extra documentation because now everyone thinks
they can invent new hoops to jump through.
Speaking of hoops, the BPF documentation says that you have to figure
out which tree a patch applies to instead of just working against
linux-next. Is there an automated way to do that? I looked through my
inbox and there are a bunch of patches marked as going through the
bpf-next tree but about half were marked incorrectly so it looks like
everyone who tries to tag their patches is going to do it badly anyway.
regards,
dan carpenter
> Document the basic policies of the libnvdimm subsystem and provide a first
> example of a Maintainer Entry Profile for others to duplicate and edit.
>
> Cc: Vishal Verma <[email protected]>
> Cc: Dave Jiang <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> Documentation/nvdimm/maintainer-entry-profile.rst | 64 +++++++++++++++++++++
> MAINTAINERS | 4 +
> 2 files changed, 68 insertions(+)
> create mode 100644 Documentation/nvdimm/maintainer-entry-profile.rst
>
Looks good to me,
Acked-by: Vishal Verma <[email protected]>
On Wed, 2019-09-11 at 08:48 -0700, Dan Williams wrote:
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3f171339df53..e5d111a86e61 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -98,6 +98,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 more details submitting
> + patches to the given subsystem. This is either an in-tree file,
> + or a URI. See Documentation/maintainer/maintainer-entry-profile.rst
> + for details.
Considering each maintainer entry or driver may not have a subdirectory
under Documentation/ to put these under, would it be better to collect
them in one place, say Documentation/maintainer-entry-profiles/ ?
On Wed, 2019-09-11 at 08:48 -0700, Dan Williams wrote:
> Document the basic policies of the libnvdimm subsystem and provide a first
> example of a Maintainer Entry Profile for others to duplicate and edit.
[]
> +Coding Style Addendum
> +---------------------
> +libnvdimm expects multi-line statements to be double indented. I.e.
> +
> + if (x...
> + && ...y) {
I don't find a good reason to do this.
> diff --git a/MAINTAINERS b/MAINTAINERS
[]
> @@ -9185,6 +9185,7 @@ M: Dan Williams <[email protected]>
> M: Vishal Verma <[email protected]>
> M: Dave Jiang <[email protected]>
> L: [email protected]
> +P: Documentation/nvdimm/maintainer-entry-profile.rst
I also think the indirection to a separate
file is not a good thing.
Separate styles for individual subsystems is
not something I would want to encourage.
On 9/11/19 12:43 PM, Dan Carpenter wrote:
> On Wed, Sep 11, 2019 at 08:48:59AM -0700, Dan Williams wrote:
>> +Coding Style Addendum
>> +---------------------
>> +libnvdimm expects multi-line statements to be double indented. I.e.
>> +
>> + if (x...
>> + && ...y) {
>
> That looks horrible and it causes a checkpatch warning. :( Why not
> do it the same way that everyone else does it.
>
> if (blah_blah_x && <-- && has to be on the first line for checkpatch
> blah_blah_y) { <-- [tab][space][space][space][space]blah
>
> Now all the conditions are aligned visually which makes it readable.
> They aren't aligned with the indent block so it's easy to tell the
> inside from the if condition.
>
> I kind of hate all this extra documentation because now everyone thinks
> they can invent new hoops to jump through.
FWIW, I completely agree with Dan (Carpenter) here. I absolutely
dislike having these kinds of files, and with subsystems imposing weird
restrictions on style (like the quoted example, yuck).
Additionally, it would seem saner to standardize rules around when
code is expected to hit the maintainers hands for kernel releases. Both
yours and Martins deals with that, there really shouldn't be the need
to have this specified in detail per sub-system.
--
Jens Axboe
On Wed, Sep 11, 2019 at 3:11 PM Jens Axboe <[email protected]> wrote:
>
> On 9/11/19 12:43 PM, Dan Carpenter wrote:
> > On Wed, Sep 11, 2019 at 08:48:59AM -0700, Dan Williams wrote:
> >> +Coding Style Addendum
> >> +---------------------
> >> +libnvdimm expects multi-line statements to be double indented. I.e.
> >> +
> >> + if (x...
> >> + && ...y) {
> >
> > That looks horrible and it causes a checkpatch warning. :( Why not
> > do it the same way that everyone else does it.
> >
> > if (blah_blah_x && <-- && has to be on the first line for checkpatch
> > blah_blah_y) { <-- [tab][space][space][space][space]blah
> >
> > Now all the conditions are aligned visually which makes it readable.
> > They aren't aligned with the indent block so it's easy to tell the
> > inside from the if condition.
> >
> > I kind of hate all this extra documentation because now everyone thinks
> > they can invent new hoops to jump through.
>
> FWIW, I completely agree with Dan (Carpenter) here. I absolutely
> dislike having these kinds of files, and with subsystems imposing weird
> restrictions on style (like the quoted example, yuck).
>
> Additionally, it would seem saner to standardize rules around when
> code is expected to hit the maintainers hands for kernel releases. Both
> yours and Martins deals with that, there really shouldn't be the need
> to have this specified in detail per sub-system.
So this is *the* point of the exercise.
I picked up this indentation habit a long while back in response to
review feedback on a patch to a subsystem that formatted code this
way. At the time CodingStyle did not contradict the maintainer's
preference, so I adopted it across the board.
Now I come to find that CodingStyle has settled on clang-format (in
the last 15 months) as the new standard which is a much better answer
to me than a manually specified style open to interpretation. I'll
take a look at getting libnvdimm converted over.
If we can take that further and standardize all of the places where
contributors see variations across subsystems on the fundamental
questions of style, timing, follow-up, and unit test invocation the
Maintainer Entry Profile can be superseded with common guidelines.
Otherwise, yes I completely agree with you that a "Maintainer Entry
Profile" is a sad comment on the state of what contributors need to
navigate, but that's today's reality that needs to be addressed.
On 9/11/19 5:40 PM, Martin K. Petersen wrote:
> * Do not use custom To: and Cc: for individual patches. We want to see the
> whole series, even patches that potentially need to go through a different
> subsystem tree.
Hi Martin,
Thanks for having written this summary. This is very helpful. For the
above paragraph, should it be clarified whether that requirement applies
to mailing list e-mail addresses only or also to individual e-mail
addresses? When using git send-email it is easy to end up with different
cc-lists per patch.
> * The patch must compile without warnings (make C=1 CF="-D__CHECK_ENDIAN__")
> and does not incur any zeroday test robot complaints.
How about adding W=1 to that make command?
How about existing drivers that trigger tons of endianness warnings,
e.g. qla2xxx? How about requiring that no new warnings are introduced?
> * The patch must have a commit message that describes, comprehensively and in
> plain English, what the patch does.
How about making this requirement more detailed and requiring that not
only what has been changed is document but also why that change has been
made?
> * Patches which have been obsoleted by a later version will be set to
> "Superceded" status.
Did you perhaps mean "Superseded"?
Thanks,
Bart.
On Thu, Sep 12, 2019 at 9:43 AM Dan Williams <[email protected]> wrote:
>
> Now I come to find that CodingStyle has settled on clang-format (in
> the last 15 months) as the new standard which is a much better answer
> to me than a manually specified style open to interpretation. I'll
> take a look at getting libnvdimm converted over.
Note that clang-format cannot do everything as we want within the
kernel just yet, but it is a close enough approximation -- it is near
the point where we could simply agree to use it and stop worrying
about styling issues. However, that would mean everyone needs to have
a recent clang-format available, which I think is the biggest obstacle
at the moment.
Cheers,
Miguel
On 9/11/19 4:48 PM, Dan Williams wrote:
> At last years Plumbers Conference I proposed the Maintainer Entry
> 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 Maintainer Entry 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.
Any maintainer who reads this might interpret this as an encouragement
to establish custom policies. I think one of the conclusions of the
Linux Plumbers 2019 edition is that too much diversity is bad and that
we need more uniformity across kernel subsystems with regard what is
expected from patch contributors. I would appreciate if a summary of
https://linuxplumbersconf.org/event/4/contributions/554/attachments/353/584/Reflections__Kernel_Summit_2019.pdf
would be integrated in the maintainer handbook.
Thanks,
Bart.
On Thu, 2019-09-12 at 14:31 +0100, Bart Van Assche wrote:
> On 9/11/19 5:40 PM, Martin K. Petersen wrote:
> > * Do not use custom To: and Cc: for individual patches. We want to see the
> > whole series, even patches that potentially need to go through a different
> > subsystem tree.
That's not currently feasible when cc'ing any vger.kernel.org list
as those lists have a maximum email header size and patches that
span multiple subsystems can have very long to: and cc: lists.
> > * The patch must compile without warnings (make C=1 CF="-D__CHECK_ENDIAN__")
> > and does not incur any zeroday test robot complaints.
>
> How about adding W=1 to that make command?
That's rather too compiler version dependent and new
warnings frequently get introduced by new compiler versions.
> How about existing drivers that trigger tons of endianness warnings,
> e.g. qla2xxx? How about requiring that no new warnings are introduced?
Adding a sparse clean C=2 requirement might be useful.
> > * The patch must have a commit message that describes, comprehensively and in
> > plain English, what the patch does.
>
> How about making this requirement more detailed and requiring that not
> only what has been changed is document but also why that change has been
> made?
I believe the "why" is rather more important than the "how"
and should be the primary thing described in the commit message.
On 9/12/19 8:34 AM, Joe Perches wrote:
> On Thu, 2019-09-12 at 14:31 +0100, Bart Van Assche wrote:
>> On 9/11/19 5:40 PM, Martin K. Petersen wrote:
>>> * The patch must compile without warnings (make C=1 CF="-D__CHECK_ENDIAN__")
>>> and does not incur any zeroday test robot complaints.
>>
>> How about adding W=1 to that make command?
>
> That's rather too compiler version dependent and new
> warnings frequently get introduced by new compiler versions.
I've never observed this myself. If a new compiler warning is added to
gcc and if it produces warnings that are not useful for kernel code
usually Linus or someone else is quick to suppress that warning.
Another argument in favor of W=1 is that the formatting of kernel-doc
headers is checked only if W=1 is passed to make.
Bart.
On Thu, 2019-09-12 at 13:01 -0700, Bart Van Assche wrote:
> Another argument in favor of W=1 is that the formatting of kernel-doc
> headers is checked only if W=1 is passed to make.
Actually, but for the fact there are far too many
to generally enable that warning right now,
(an x86-64 defconfig has more than 1000)
that sounds pretty reasonable to me.
On 9/12/19 12:13 AM, Dan Carpenter wrote:
> On Wed, Sep 11, 2019 at 08:48:59AM -0700, Dan Williams wrote:
>> +Coding Style Addendum
>> +---------------------
>> +libnvdimm expects multi-line statements to be double indented. I.e.
>> +
>> + if (x...
>> + && ...y) {
>
> That looks horrible and it causes a checkpatch warning. :( Why not
> do it the same way that everyone else does it.
>
> if (blah_blah_x && <-- && has to be on the first line for checkpatch
> blah_blah_y) { <-- [tab][space][space][space][space]blah
>
> Now all the conditions are aligned visually which makes it readable.
> They aren't aligned with the indent block so it's easy to tell the
> inside from the if condition.
I came across this while sending patches to libnvdimm subsystem. W.r.t
coding Style can we have consistent styles across the kernel? Otherwise,
one would have to change the editor settings as they work across
different subsystems in the kernel. In this specific case both
clang-format and emacs customization tip in the kernel documentation
directory suggest the later style.
-aneesh
On Fri, Sep 13, 2019 at 07:41:55AM +0530, Aneesh Kumar K.V wrote:
> On 9/12/19 12:13 AM, Dan Carpenter wrote:
> > On Wed, Sep 11, 2019 at 08:48:59AM -0700, Dan Williams wrote:
> > > +Coding Style Addendum
> > > +---------------------
> > > +libnvdimm expects multi-line statements to be double indented. I.e.
> > > +
> > > + if (x...
> > > + && ...y) {
> >
> > That looks horrible and it causes a checkpatch warning. :( Why not
> > do it the same way that everyone else does it.
> >
> > if (blah_blah_x && <-- && has to be on the first line for checkpatch
> > blah_blah_y) { <-- [tab][space][space][space][space]blah
> >
> > Now all the conditions are aligned visually which makes it readable.
> > They aren't aligned with the indent block so it's easy to tell the
> > inside from the if condition.
>
>
> I came across this while sending patches to libnvdimm subsystem. W.r.t
> coding Style can we have consistent styles across the kernel? Otherwise, one
> would have to change the editor settings as they work across different
> subsystems in the kernel. In this specific case both clang-format and emacs
> customization tip in the kernel documentation directory suggest the later
> style.
We _should_ have a consistent coding style across the whole kernel,
that's the whole reason for having a coding style in the first place!
The problem is, we all have agreed on the "basics" a long time ago, but
are now down in the tiny nits as to what some minor things should, or
should not, look like.
It might be time to just bite the bullet and do something like
"clang-format" to stop arguing about stuff like this for new
submissions, if for no other reason to keep us from wasting mental
energy on trivial things like this.
thanks,
greg k-h
On Wed, 11 Sep 2019 16:11:29 -0600
Jens Axboe <[email protected]> wrote:
> On 9/11/19 12:43 PM, Dan Carpenter wrote:
> >
> > I kind of hate all this extra documentation because now everyone thinks
> > they can invent new hoops to jump through.
>
> FWIW, I completely agree with Dan (Carpenter) here. I absolutely
> dislike having these kinds of files, and with subsystems imposing weird
> restrictions on style (like the quoted example, yuck).
>
> Additionally, it would seem saner to standardize rules around when
> code is expected to hit the maintainers hands for kernel releases. Both
> yours and Martins deals with that, there really shouldn't be the need
> to have this specified in detail per sub-system.
This sort of objection came up at the maintainers summit yesterday; the
consensus was that, while we might not like subsystem-specific rules, they
do currently exist and we're just documenting reality. To paraphrase
Phillip K. Dick, reality is that which, when you refuse to document it,
doesn't go away.
So I'm expecting to take this kind of stuff into Documentation/. My own
personal hope is that it can maybe serve to shame some of these "local
quirks" out of existence. The evidence from this brief discussion suggests
that this might indeed happen.
Thanks,
jon
On Fri, Sep 13, 2019 at 01:09:37AM -0600, Jonathan Corbet wrote:
> On Wed, 11 Sep 2019 16:11:29 -0600
> Jens Axboe <[email protected]> wrote:
>
> > On 9/11/19 12:43 PM, Dan Carpenter wrote:
> > >
> > > I kind of hate all this extra documentation because now everyone thinks
> > > they can invent new hoops to jump through.
> >
> > FWIW, I completely agree with Dan (Carpenter) here. I absolutely
> > dislike having these kinds of files, and with subsystems imposing weird
> > restrictions on style (like the quoted example, yuck).
> >
> > Additionally, it would seem saner to standardize rules around when
> > code is expected to hit the maintainers hands for kernel releases. Both
> > yours and Martins deals with that, there really shouldn't be the need
> > to have this specified in detail per sub-system.
>
> This sort of objection came up at the maintainers summit yesterday; the
> consensus was that, while we might not like subsystem-specific rules, they
> do currently exist and we're just documenting reality. To paraphrase
> Phillip K. Dick, reality is that which, when you refuse to document it,
> doesn't go away.
There aren't that many subsystem rules. The big exception is
networking, with the comment style and reverse Chrismas tree
declarations. Also you have to label which git tree the patch applies
to like [net] or [net-next].
It used to be that infiniband used "sizeof foo" instead of sizeof(foo)
but now there is a new maintainer.
There is one subsystem which where the maintainer will capitalize your
patch prefix and complain. There are others where they will silently
change it to lower case. (Maybe that has changed in recent years).
There is one subsystem where the maintainer is super strict rules that
you can't use "I" or "we" in the commit message. So you can't say "I
noticed a bug while reviewing", you have to say "The code has a bug".
Some maintainers have rules about what you can put in the declaration
block. No kmalloc() in the declarations is a common rule.
"struct foo *p = kmalloc();".
Some people (I do) have strict rules for error handling, but most won't
complain unless the error handling has bugs.
The bpf people want you to put [bpf] or [bpf-next] in the subject.
Everyone just guesses, and uneducated guesses are worse than leaving it
blank, but that's just my opinion.
> So I'm expecting to take this kind of stuff into Documentation/. My own
> personal hope is that it can maybe serve to shame some of these "local
> quirks" out of existence. The evidence from this brief discussion suggests
> that this might indeed happen.
I don't think it's shaming, I think it's validating. Everyone just
insists that since it's written in the Book of Rules then it's our fault
for not reading it. It's like those EULA things where there is more
text than anyone can physically read in a life time.
And the documentation doesn't help. For example, I knew people's rules
about capitalizing the subject but I'd just forget. I say that if you
can't be bothered to add it to checkpatch then it means you don't really
care that strongly.
regards,
dan carpenter
On 9/13/19 4:48 AM, Dan Carpenter wrote:
>> So I'm expecting to take this kind of stuff into Documentation/. My own
>> personal hope is that it can maybe serve to shame some of these "local
>> quirks" out of existence. The evidence from this brief discussion suggests
>> that this might indeed happen.
>
> I don't think it's shaming, I think it's validating. Everyone just
> insists that since it's written in the Book of Rules then it's our fault
> for not reading it. It's like those EULA things where there is more
> text than anyone can physically read in a life time.
Yes, agreed.
> And the documentation doesn't help. For example, I knew people's rules
> about capitalizing the subject but I'd just forget. I say that if you
> can't be bothered to add it to checkpatch then it means you don't really
> care that strongly.
If a subsystem requires a certain spelling/capitalization in patch email
subjects, it should be added to MAINTAINERS IMO. E.g.,
E: NuBus
--
~Randy
On Fri, Sep 13, 2019 at 4:00 PM Randy Dunlap <[email protected]> wrote:
>
> On 9/13/19 4:48 AM, Dan Carpenter wrote:
>
> >> So I'm expecting to take this kind of stuff into Documentation/. My own
> >> personal hope is that it can maybe serve to shame some of these "local
> >> quirks" out of existence. The evidence from this brief discussion suggests
> >> that this might indeed happen.
> >
> > I don't think it's shaming, I think it's validating. Everyone just
> > insists that since it's written in the Book of Rules then it's our fault
> > for not reading it. It's like those EULA things where there is more
> > text than anyone can physically read in a life time.
>
> Yes, agreed.
>
> > And the documentation doesn't help. For example, I knew people's rules
> > about capitalizing the subject but I'd just forget. I say that if you
> > can't be bothered to add it to checkpatch then it means you don't really
> > care that strongly.
>
> If a subsystem requires a certain spelling/capitalization in patch email
> subjects, it should be added to MAINTAINERS IMO. E.g.,
> E: NuBus
+1
Better make this a regex to deal with (net|net-next).
We could probably script populating MAINTAINERS with this using how it
is done manually: git log --oneline <dir>
Rob
Em Wed, 11 Sep 2019 08:48:48 -0700
Dan Williams <[email protected]> escreveu:
> Fixup some P: entries to be M: and delete the others that do not include an
> email address. The P: tag will be used to indicate the location of a
> Profile for a given MAINTAINERS entry.
>
> Cc: Joe Perches <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> MAINTAINERS | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
...
> MEDIA INPUT INFRASTRUCTURE (V4L/DVB)
> M: Mauro Carvalho Chehab <[email protected]>
> -P: LinuxTV.org Project
> L: [email protected]
> W: https://linuxtv.org
> Q: http://patchwork.kernel.org/project/linux-media/list/
> @@ -13452,7 +13450,6 @@ S: Maintained
> F: arch/mips/ralink
Acked-by: Mauro Carvalho Chehab <[email protected]>
Thanks,
Mauro
On Fri, Sep 13, 2019 at 3:12 PM Joe Perches <[email protected]> wrote:
>
> On Thu, 2019-09-12 at 13:01 -0700, Bart Van Assche wrote:
>
> > Another argument in favor of W=1 is that the formatting of kernel-doc
> > headers is checked only if W=1 is passed to make.
>
> Actually, but for the fact there are far too many
> to generally enable that warning right now,
> (an x86-64 defconfig has more than 1000)
> that sounds pretty reasonable to me.
It's in the 1000s on arm because W=1 turns on more checks in building
.dts files. There are lots of duplicates as most of the dts content is
as an include file (e.g. board dts includes soc dts).
We could shift the dts checks to W=2 (and what's enabled by W=2 now to
3) I guess.
Why not just promote what doesn't give false or still noisy warnings
out of W=1 to be the default?
Rob
Hi Randy,
On Fri, Sep 13, 2019 at 5:00 PM Randy Dunlap <[email protected]> wrote:
> On 9/13/19 4:48 AM, Dan Carpenter wrote:
> >> So I'm expecting to take this kind of stuff into Documentation/. My own
> >> personal hope is that it can maybe serve to shame some of these "local
> >> quirks" out of existence. The evidence from this brief discussion suggests
> >> that this might indeed happen.
> >
> > I don't think it's shaming, I think it's validating. Everyone just
> > insists that since it's written in the Book of Rules then it's our fault
> > for not reading it. It's like those EULA things where there is more
> > text than anyone can physically read in a life time.
>
> Yes, agreed.
>
> > And the documentation doesn't help. For example, I knew people's rules
> > about capitalizing the subject but I'd just forget. I say that if you
> > can't be bothered to add it to checkpatch then it means you don't really
> > care that strongly.
>
> If a subsystem requires a certain spelling/capitalization in patch email
> subjects, it should be added to MAINTAINERS IMO. E.g.,
> E: NuBus
Oh, I understood the question differently. I thought it was about
"sub: system: Fix foo" vs. "sub: system: fix foo".
For simple and trivial things, I tend to make changes while applying, as that's
usually less work than complaining, and verifying that it's been fixed in the
next (if any) version n days/weeks/months later.
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
On Fri, 2019-09-13 at 15:26 +0100, Rob Herring wrote:
> On Fri, Sep 13, 2019 at 3:12 PM Joe Perches <[email protected]> wrote:
> > On Thu, 2019-09-12 at 13:01 -0700, Bart Van Assche wrote:
> >
> > > Another argument in favor of W=1 is that the formatting of kernel-doc
> > > headers is checked only if W=1 is passed to make.
> >
> > Actually, but for the fact there are far too many
> > to generally enable that warning right now,
> > (an x86-64 defconfig has more than 1000)
> > that sounds pretty reasonable to me.
> It's in the 1000s on arm because W=1 turns on more checks in building
> .dts files. There are lots of duplicates as most of the dts content is
> as an include file (e.g. board dts includes soc dts).
Yeah, dts[i] files in arm compilations are very noisy at W=1
so moving those message types to W=2 seems sensible.
$ { opt="ARCH=arm CROSS_COMPILE=arm-unknown-linux-gnueabi-" ; make $opt clean ; make $opt defconfig ; make $opt W=1 -j4 ; } > arm_make.log 2>&1
$ grep -i -P 'dtsi?:.*warning' arm_make.log | wc -l
69164
Just fyi: for an x86-64 defconfig with gcc 8.3
$ { make clean ; make defconfig ; make -j4 W=1 ; } > make.log 2>&1
There are ~300 W=1 for non kernel-doc -W<foo> warnings.
$ grep -i -P -oh '\[\-W[\w\-]+\]' make.log |sort | uniq -c | sort -rn
163 [-Wmissing-prototypes]
69 [-Wunused-but-set-variable]
16 [-Wempty-body]
10 [-Wtype-limits]
6 [-Woverride-init]
2 [-Wstringop-truncation]
2 [-Wcast-function-type]
1 [-Wunused-but-set-parameter]
And there are ~1000 kernel-doc only messages in various files
$ grep -i 'function parameter' make.log | cut -f1 -d: | sort | uniq -c
6 arch/x86/events/intel/pt.c
2 arch/x86/kernel/apic/apic.c
10 arch/x86/kernel/cpu/mtrr/generic.c
5 arch/x86/kernel/crash_dump_64.c
1 arch/x86/kernel/i8259.c
3 arch/x86/kernel/smpboot.c
3 arch/x86/kernel/tsc.c
2 arch/x86/kernel/uprobes.c
1 arch/x86/lib/cmdline.c
1 arch/x86/lib/insn.c
2 arch/x86/lib/insn-eval.c
4 arch/x86/lib/msr.c
2 arch/x86/lib/usercopy_64.c
1 arch/x86/mm/pat.c
13 arch/x86/mm/pgtable.c
1 arch/x86/pci/i386.c
2 arch/x86/power/cpu.c
2 arch/x86/power/hibernate.c
8 certs/system_keyring.c
4 crypto/asymmetric_keys/asymmetric_type.c
3 crypto/asymmetric_keys/pkcs7_trust.c
16 crypto/jitterentropy.c
3 drivers/acpi/acpi_apd.c
3 drivers/acpi/bus.c
2 drivers/acpi/cppc_acpi.c
5 drivers/acpi/device_sysfs.c
2 drivers/acpi/dock.c
2 drivers/acpi/nvs.c
1 drivers/acpi/pci_root.c
5 drivers/acpi/property.c
4 drivers/acpi/sleep.c
7 drivers/acpi/utils.c
1 drivers/ata/libata-acpi.c
2 drivers/ata/libata-pmp.c
6 drivers/ata/libata-transport.c
4 drivers/ata/pata_amd.c
4 drivers/base/attribute_container.c
1 drivers/base/devcon.c
3 drivers/base/platform-msi.c
3 drivers/base/power/runtime.c
5 drivers/base/power/wakeup.c
2 drivers/char/agp/backend.c
3 drivers/char/agp/generic.c
2 drivers/clk/clk.c
1 drivers/clk/clk-fixed-factor.c
1 drivers/clk/clk-fixed-rate.c
3 drivers/connector/cn_proc.c
31 drivers/cpufreq/cpufreq.c
3 drivers/cpufreq/cpufreq_governor.c
7 drivers/cpufreq/freq_table.c
1 drivers/cpufreq/intel_pstate.c
6 drivers/cpuidle/sysfs.c
1 drivers/dma-buf/dma-buf.c
1 drivers/dma-buf/dma-fence-chain.c
6 drivers/dma/dmaengine.c
7 drivers/firewire/init_ohci1394_dma.c
2 drivers/firmware/efi/memmap.c
1 drivers/firmware/efi/vars.c
20 drivers/gpu/drm/drm_agpsupport.c
8 drivers/hid/hid-core.c
3 drivers/hid/hid-quirks.c
5 drivers/hid/usbhid/hid-pidff.c
3 drivers/input/mouse/synaptics.c
2 drivers/iommu/amd_iommu_init.c
2 drivers/iommu/dmar.c
1 drivers/iommu/intel-pasid.c
1 drivers/iommu/iommu.c
2 drivers/leds/led-class.c
2 drivers/mailbox/pcc.c
6 drivers/net/ethernet/intel/e1000/e1000_hw.c
21 drivers/net/ethernet/intel/e1000/e1000_main.c
1 drivers/net/ethernet/intel/e1000e/80003es2lan.c
6 drivers/net/ethernet/intel/e1000e/ich8lan.c
42 drivers/net/ethernet/intel/e1000e/netdev.c
3 drivers/net/ethernet/intel/e1000e/phy.c
2 drivers/net/ethernet/intel/e1000e/ptp.c
1 drivers/net/netconsole.c
3 drivers/net/phy/mdio-boardinfo.c
2 drivers/net/phy/mdio_device.c
2 drivers/pci/ats.c
3 drivers/pci/hotplug/acpi_pcihp.c
4 drivers/pci/pcie/aer.c
2 drivers/pci/pcie/pme.c
1 drivers/pci/setup-bus.c
1 drivers/pci/vc.c
22 drivers/pcmcia/cistpl.c
13 drivers/pcmcia/pcmcia_cis.c
13 drivers/pcmcia/pcmcia_resource.c
11 drivers/pcmcia/rsrc_nonstatic.c
1 drivers/pnp/system.c
11 drivers/rtc/interface.c
3 drivers/rtc/sysfs.c
2 drivers/thermal/step_wise.c
2 drivers/thermal/user_space.c
6 drivers/tty/n_tty.c
4 drivers/tty/pty.c
7 drivers/tty/tty_audit.c
7 drivers/tty/tty_baudrate.c
13 drivers/tty/tty_buffer.c
25 drivers/tty/tty_io.c
5 drivers/tty/tty_jobctrl.c
6 drivers/tty/tty_ldisc.c
6 drivers/tty/tty_port.c
5 drivers/tty/vt/consolemap.c
4 drivers/tty/vt/vt.c
3 drivers/tty/vt/vt_ioctl.c
12 drivers/usb/common/debug.c
1 drivers/usb/host/pci-quirks.c
1 drivers/usb/host/xhci.c
6 drivers/usb/host/xhci-mem.c
2 drivers/video/backlight/backlight.c
1 drivers/video/fbdev/core/fbmon.c
2 drivers/video/fbdev/core/fb_notify.c
7 fs/devpts/inode.c
4 fs/direct-io.c
3 fs/eventpoll.c
6 fs/fat/dir.c
3 fs/fat/misc.c
6 fs/fat/nfs.c
4 fs/fs_context.c
1 fs/fs_parser.c
2 fs/fs-writeback.c
5 fs/ioctl.c
1 fs/libfs.c
3 fs/namespace.c
2 fs/nfs_common/grace.c
2 fs/open.c
3 fs/posix_acl.c
1 fs/proc/proc_net.c
2 fs/proc/vmcore.c
2 fs/read_write.c
1 fs/super.c
5 fs/xattr.c
2 kernel/cgroup/cpuset.c
1 kernel/cpu.c
4 kernel/events/core.c
2 kernel/events/hw_breakpoint.c
5 kernel/fork.c
27 kernel/irq/irqdomain.c
3 kernel/irq/matrix.c
1 kernel/kprobes.c
5 kernel/locking/rtmutex.c
1 kernel/notifier.c
3 kernel/power/main.c
2 kernel/power/qos.c
51 kernel/power/snapshot.c
2 kernel/power/suspend.c
14 kernel/power/swap.c
1 kernel/reboot.c
4 kernel/seccomp.c
2 kernel/stacktrace.c
4 kernel/time/alarmtimer.c
1 kernel/time/clockevents.c
4 kernel/time/posix-cpu-timers.c
1 kernel/time/tick-broadcast.c
6 kernel/time/tick-oneshot.c
3 kernel/time/tick-sched.c
3 kernel/time/timeconv.c
23 kernel/time/timekeeping.c
9 kernel/trace/ring_buffer.c
8 kernel/trace/trace_events_filter.c
2 kernel/trace/trace_events_trigger.c
1 kernel/trace/trace_seq.c
1 lib/argv_split.c
1 lib/cpumask.c
5 lib/genalloc.c
4 lib/iov_iter.c
2 lib/nlattr.c
3 lib/percpu_counter.c
6 lib/radix-tree.c
2 lib/scatterlist.c
3 lib/win_minmax.c
1 mm/compaction.c
2 mm/oom_kill.c
1 mm/page_vma_mapped.c
2 mm/swap_state.c
1 mm/vmalloc.c
1 mm/vmscan.c
8 net/ipv4/cipso_ipv4.c
3 net/ipv4/ipmr.c
1 net/ipv4/tcp_input.c
5 net/ipv4/tcp_output.c
2 net/ipv4/tcp_timer.c
3 net/ipv4/udp.c
4 net/ipv6/calipso.c
1 net/ipv6/exthdrs.c
1 net/ipv6/ip6_output.c
3 net/ipv6/udp.c
1 net/mac80211/tx.c
5 net/netlabel/netlabel_calipso.c
2 net/netlabel/netlabel_domainhash.c
3 net/sched/ematch.c
1 net/socket.c
1 net/wireless/radiotap.c
4 net/wireless/reg.c
6 security/commoncap.c
1 security/lsm_audit.c
12 security/selinux/avc.c
7 security/selinux/netlabel.c
2 security/selinux/netport.c
4 security/selinux/ss/mls.c
34 security/selinux/ss/services.c
3 sound/hda/hdac_bus.c
1 sound/hda/hdac_component.c
2 sound/hda/hdac_controller.c
10 sound/hda/hdac_device.c
2 sound/hda/hdac_stream.c
1 sound/pci/hda/hda_codec.c
Hi Bart,
> On 9/11/19 5:40 PM, Martin K. Petersen wrote:
>> * Do not use custom To: and Cc: for individual patches. We want to see the
>> whole series, even patches that potentially need to go through a different
>> subsystem tree.
>
> Thanks for having written this summary. This is very helpful. For the
> above paragraph, should it be clarified whether that requirement
> applies to mailing list e-mail addresses only or also to individual
> e-mail addresses? When using git send-email it is easy to end up with
> different cc-lists per patch.
I prefer to have the entire series sent to linux-scsi or
target-devel. It wouldn't be so bad if discussions about the merits of a
tree-wide change consistently happened in responses to the cover
letter. But more often than not discussion happens in response to a
patch touching a different subsystem and therefore in a mail exchange
that doesn't end up on linux-scsi.
>> * The patch must compile without warnings (make C=1 CF="-D__CHECK_ENDIAN__")
>> and does not incur any zeroday test robot complaints.
>
> How about adding W=1 to that make command?
>
> How about existing drivers that trigger tons of endianness warnings,
> e.g. qla2xxx? How about requiring that no new warnings are introduced?
This was in response to a driver submission (for a different driver)
around the time this doc was written. The problem is that it's sometimes
hard to distinguish new warnings from old ones. I'm all for requiring
that no new warnings are introduced.
>> * The patch must have a commit message that describes,
>> comprehensively and in plain English, what the patch does.
>
> How about making this requirement more detailed and requiring that not
> only what has been changed is document but also why that change has
> been made?
I'd really like all this patch submission guideline material to live in
Documentation/process. But yes.
--
Martin K. Petersen Oracle Linux Engineering
On Fri, 2019-09-13 at 16:46 +0100, Rob Herring wrote:
> On Fri, Sep 13, 2019 at 4:00 PM Randy Dunlap <[email protected]> wrote:
> > On 9/13/19 4:48 AM, Dan Carpenter wrote:
> >
> > > > So I'm expecting to take this kind of stuff into Documentation/. My own
> > > > personal hope is that it can maybe serve to shame some of these "local
> > > > quirks" out of existence. The evidence from this brief discussion suggests
> > > > that this might indeed happen.
> > >
> > > I don't think it's shaming, I think it's validating. Everyone just
> > > insists that since it's written in the Book of Rules then it's our fault
> > > for not reading it. It's like those EULA things where there is more
> > > text than anyone can physically read in a life time.
> >
> > Yes, agreed.
> >
> > > And the documentation doesn't help. For example, I knew people's rules
> > > about capitalizing the subject but I'd just forget. I say that if you
> > > can't be bothered to add it to checkpatch then it means you don't really
> > > care that strongly.
> >
> > If a subsystem requires a certain spelling/capitalization in patch email
> > subjects, it should be added to MAINTAINERS IMO. E.g.,
> > E: NuBus
>
> +1
>
> Better make this a regex to deal with (net|net-next).
>
> We could probably script populating MAINTAINERS with this using how it
> is done manually: git log --oneline <dir>
I made a similar proposal nearly a decade ago to add a grammar
to MAINTAINERS sections for patch subject prefixes.
https://lore.kernel.org/lkml/1289919077.28741.50.camel@Joe-Laptop/
Em Fri, 13 Sep 2019 11:42:38 -0700
Joe Perches <[email protected]> escreveu:
> On Fri, 2019-09-13 at 15:26 +0100, Rob Herring wrote:
> > On Fri, Sep 13, 2019 at 3:12 PM Joe Perches <[email protected]> wrote:
> > > On Thu, 2019-09-12 at 13:01 -0700, Bart Van Assche wrote:
> > >
> > > > Another argument in favor of W=1 is that the formatting of kernel-doc
> > > > headers is checked only if W=1 is passed to make.
> > >
> > > Actually, but for the fact there are far too many
> > > to generally enable that warning right now,
> > > (an x86-64 defconfig has more than 1000)
> > > that sounds pretty reasonable to me.
>
> > It's in the 1000s on arm because W=1 turns on more checks in building
> > .dts files. There are lots of duplicates as most of the dts content is
> > as an include file (e.g. board dts includes soc dts).
>
> Yeah, dts[i] files in arm compilations are very noisy at W=1
> so moving those message types to W=2 seems sensible.
>
> $ { opt="ARCH=arm CROSS_COMPILE=arm-unknown-linux-gnueabi-" ; make $opt clean ; make $opt defconfig ; make $opt W=1 -j4 ; } > arm_make.log 2>&1
>
> $ grep -i -P 'dtsi?:.*warning' arm_make.log | wc -l
> 69164
Yeah, makes sense moving them to W=2, or to make them somehow produce
less noise.
> Just fyi: for an x86-64 defconfig with gcc 8.3
>
> $ { make clean ; make defconfig ; make -j4 W=1 ; } > make.log 2>&1
>
> There are ~300 W=1 for non kernel-doc -W<foo> warnings.
>
> $ grep -i -P -oh '\[\-W[\w\-]+\]' make.log |sort | uniq -c | sort -rn
> 163 [-Wmissing-prototypes]
> 69 [-Wunused-but-set-variable]
> 16 [-Wempty-body]
> 10 [-Wtype-limits]
> 6 [-Woverride-init]
> 2 [-Wstringop-truncation]
> 2 [-Wcast-function-type]
> 1 [-Wunused-but-set-parameter]
On my eyes, it doesn't sound too much. I suspect that,
with gcc-9, it should produce more warnings, though.
Perhaps we could "promote" most of those to W=0.
>
> And there are ~1000 kernel-doc only messages in various files
A significant amount of those warnings appear with "make htmldocs".
Not having the kernel-doc warning as part of W=0 helps to make
very hard to have them cleared.
IMHO, those should be enabled by default with W=0, at least for the
files that are included on some kernel-doc tag.
I mean, perhaps, when W=0, Kernel build could run:
$ ./scripts/kernel-doc -none $(git grep kernel-doc:: Documentation/|cut -d: -f4-|sort|uniq|grep -vE "\bsource\b")
That produces 165 warnings (against v5.3-rc4 + media -next patches).
Thanks,
Mauro
On Fri, 2019-09-13 at 16:17 -0300, Mauro Carvalho Chehab wrote:
> Em Fri, 13 Sep 2019 11:42:38 -0700
> Joe Perches <[email protected]> escreveu:
[]
> > Just fyi: for an x86-64 defconfig with gcc 8.3
> >
> > $ { make clean ; make defconfig ; make -j4 W=1 ; } > make.log 2>&1
> >
> > There are ~300 W=1 for non kernel-doc -W<foo> warnings.
> >
> > $ grep -i -P -oh '\[\-W[\w\-]+\]' make.log |sort | uniq -c | sort -rn
> > 163 [-Wmissing-prototypes]
> > 69 [-Wunused-but-set-variable]
> > 16 [-Wempty-body]
> > 10 [-Wtype-limits]
> > 6 [-Woverride-init]
> > 2 [-Wstringop-truncation]
> > 2 [-Wcast-function-type]
> > 1 [-Wunused-but-set-parameter]
>
> On my eyes, it doesn't sound too much.
In general, I agree and most of these are pretty
trivial to remove. It'd just take some time to
remove most of the missing-prototypes and
unused-but-set warnings before being able to
enable the warnings at the default W=0.
> I suspect that,
> with gcc-9, it should produce more warnings, though.
It doesn't though.
At least not so far as I can tell.
gcc-9.1 produces the same output.
$ { make clean ; make defconfig ; make CC=/usr/bin/gcc-9 -j4 W=1 V=1 ; } > make_gcc9.log 2>&1
$ grep -i -P -oh '\[\-W[\w\-]+\]' make_gcc9.log | sort | uniq -c | sort -rn
163 [-Wmissing-prototypes]
69 [-Wunused-but-set-variable]
16 [-Wempty-body]
10 [-Wtype-limits]
6 [-Woverride-init]
2 [-Wstringop-truncation]
2 [-Wcast-function-type]
1 [-Wunused-but-set-parameter]
Jens,
> Additionally, it would seem saner to standardize rules around when
> code is expected to hit the maintainers hands for kernel
> releases. Both yours and Martins deals with that, there really
> shouldn't be the need to have this specified in detail per sub-system.
Yeah. There is basically nothing specific about SCSI in my write-up
outside of the branch naming.
I deliberately didn't mention coding style preferences. We have so much
20+ year old cruft in SCSI that's impossible to even entertain. But I do
request new code to follow coding-style.rst. BYOXT.
Also note that the original target audience for my document. It was
aimed at onboarding new driver contributors from hardware companies. So
people that don't live and breathe Linux development and who are not
intimately familiar with the kernel development process. It's possible
that we have this information in Documentation/ these days; I'll go
look. But it didn't exist when this doc was written. And in my
experience nobody coming to Linux development from the outside
understands what the "merge window" is. And when the appropriate time is
to submit patches and features. I think this would be a great area to
have a common set of guidelines and documentation. I'd prefer for this
to be global and then let maintainers apply their own wiggle room
instead of documenting particular rules for a given subsystem.
One pet peeve I have is that people are pretty bad at indicating the
intended target tree. I often ask for it in private mail but the
practice doesn't seem to stick. I spend a ton of time guessing whether a
patch is a fix for a new feature in the x+1 queue or a fix for the
current release. It is not always obvious.
--
Martin K. Petersen Oracle Linux Engineering
On Fri, Sep 13, 2019 at 05:44:39PM -0400, Martin K. Petersen wrote:
> One pet peeve I have is that people are pretty bad at indicating the
> intended target tree. I often ask for it in private mail but the
> practice doesn't seem to stick. I spend a ton of time guessing whether a
> patch is a fix for a new feature in the x+1 queue or a fix for the
> current release. It is not always obvious.
The Fixes tag doesn't help?
Of course, you've never asked me or anyone on kernel-newbies that
question. We don't normally know the answer either. I do always try to
figure it out for networking, but it's kind of a pain in the butt and I
mess up surpisingly often for how much effort I put into getting it
right.
regards,
dan carpenter
On Wed, 11 Sep 2019, Dan Williams <[email protected]> wrote:
> As presented at the 2018 Linux Plumbers conference [1], the Maintainer
> Entry Profile (formerly Subsystem Profile) is proposed as a way to reduce
> friction between committers and maintainers and encourage conversations
> amongst maintainers about common best practices. While coding-style,
> submit-checklist, and submitting-drivers lay out some common expectations
> there remain local customs and maintainer preferences that vary by
> subsystem.
>
> The profile contains short answers to some of the common policy questions a
> contributor might have that are local to the subsystem / device-driver, or
> otherwise not covered by the top-level process documents.
>
> Overview: General introduction to how the subsystem operates
> Submit Checklist Addendum: Mechanical items that gate submission staging
> Key Cycle Dates:
> - Last -rc for new feature submissions: Expected lead time for submissions
> - Last -rc to merge features: Deadline for merge decisions
> Coding Style Addendum: Clarifications of local style preferences
> Resubmit Cadence: When to ping the maintainer
> Checkpatch / Style Cleanups: Policy on pure cleanup patches
>
> See Documentation/maintainer/maintainer-entry-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]>
> Cc: Alexandre Belloni <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> Documentation/maintainer/index.rst | 1
> .../maintainer/maintainer-entry-profile.rst | 99 ++++++++++++++++++++
> MAINTAINERS | 4 +
> 3 files changed, 104 insertions(+)
> create mode 100644 Documentation/maintainer/maintainer-entry-profile.rst
>
> diff --git a/Documentation/maintainer/index.rst b/Documentation/maintainer/index.rst
> index 56e2c09dfa39..d904e74e1159 100644
> --- a/Documentation/maintainer/index.rst
> +++ b/Documentation/maintainer/index.rst
> @@ -12,4 +12,5 @@ additions to this manual.
> configure-git
> rebasing-and-merging
> pull-requests
> + maintainer-entry-profile
>
> diff --git a/Documentation/maintainer/maintainer-entry-profile.rst b/Documentation/maintainer/maintainer-entry-profile.rst
> new file mode 100644
> index 000000000000..aaedf4d6dbf7
> --- /dev/null
> +++ b/Documentation/maintainer/maintainer-entry-profile.rst
> @@ -0,0 +1,99 @@
> +.. _maintainerentryprofile:
> +
> +Maintainer Entry Profile
> +========================
> +
> +The Maintainer Entry Profile supplements the top-level process documents
> +(coding-style, submitting-patches...) with subsystem/device-driver-local
> +customs as well as details about the patch submission lifecycle. A
> +contributor uses this document to level set their expectations and avoid
> +common mistakes, maintainers may use these profiles to look across
> +subsystems for opportunities to converge on common practices.
In general I think we're already pretty good at adding and accumulating
documentation, but not so much cleaning up existing and outright
throwing out obsolete documentation.
'wc -l Documentation/process/*' says we already have 11k lines of
generic process documentation.
I would much rather see a streamlined and friendly guide to contributing
to the kernel than more rules, and driver/subsystem specific rules at
that. I would much rather get the feeling from submissions that the long
tail of contributors have actually read and understood the most relevant
parts of Documentation/process.
I'm pretty sure *I* haven't read all of Documentation/process.
My fear is that the entry profiles will only be helpful to the
contributors that already "get it". It may be helpful to the maintainers
in the sense that they can reply to patches with a pointer to the
relevant hoops to jump through.
---
Even so, if this gets merged, I'll probably add something helpful for
drm and/or i915 contributors. But what's the buy-in across the kernel?
If my grep serves me right, there are something like 2000+ entries in
MAINTAINERS. If 10% of them adds a profile, that's a fairly serious
amount of documentation. But can you actually expect more than a handful
of subsystems/drivers to add a profile? Then what's the coverage?
For reference, I've added or promoted adding the B: and C: entries to
MAINTAINERS. Various subsystems and drivers are really picky about where
and how they want bugs reported; in particular some folks only accept
email. Yet networking is the only one to indicate the email preference
in MAINTAINERS. And adding that is a one line change. (There are 19 B:
entries and 7 C: entries in total.)
BR,
Jani.
> +
> +
> +Overview
> +--------
> +Provide an introduction to how the subsystem operates. While MAINTAINERS
> +tells the contributor where to send patches for which files, it does not
> +convey other subsystem-local infrastructure and mechanisms that aid
> +development.
> +Example questions to consider:
> +- Are there notifications when patches are applied to the local tree, or
> + merged upstream?
> +- Does the subsystem have a patchwork instance?
> +- Any bots or CI infrastructure that watches the list?
> +- Git branches that are pulled into -next?
> +- What branch should contributors submit against?
> +- Links to any other Maintainer Entry Profiles? For example a
> + device-driver may point to an entry for its parent subsystem. This makes
> + the contributor aware of obligations a maintainer may have have for
> + other maintainers in the submission chain.
> +
> +
> +Submit Checklist Addendum
> +-------------------------
> +List mandatory and advisory criteria, beyond the common "submit-checklist",
> +for a patch to be considered healthy enough for maintainer attention.
> +For example: "pass checkpatch.pl with no errors, or warning. Pass the
> +unit test detailed at $URI".
> +
> +
> +Key Cycle Dates
> +---------------
> +One of the common misunderstandings of submitters is that patches can be
> +sent at any time before the merge window closes and can still be
> +considered for the next -rc1. The reality is that most patches need to
> +be settled in soaking in linux-next in advance of the merge window
> +opening. Clarify for the submitter the key dates (in terms rc release
> +week) that patches might considered for merging and when patches need to
> +wait for the next -rc. At a minimum:
> +- 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.
> +
> +- Last -rc to merge features: Deadline for merge decisions
> + 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.
> +
> +Optional:
> +- First -rc at which the development baseline branch, listed in the
> + overview section, should be considered ready for new submissions.
> +
> +
> +Coding Style Addendum
> +---------------------
> +While the top-level "coding-style" document covers most style
> +considerations there are still discrepancies and local preferences
> +across subsystems. If a submitter should be aware of incremental / local
> +style guidelines like x-mas tree variable declarations, indentation
> +for multi line statements, function definition style, etc., list them
> +here.
> +
> +
> +Review Cadence
> +--------------
> +One of the largest sources of contributor angst is how soon to ping
> +after a patchset has been posted without receiving any feedback. In
> +addition to specifying how long to wait before a resubmission this
> +section can also indicate a preferred style of update like, resend the
> +full series, or privately send a reminder email. This section might also
> +list how review works for this code area and methods to get feedback
> +that are not directly from the maintainer.
> +
> +
> +Style Cleanup Patches
> +---------------------
> +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".
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3f171339df53..e5d111a86e61 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -98,6 +98,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 more details submitting
> + patches to the given subsystem. This is either an in-tree file,
> + or a URI. See Documentation/maintainer/maintainer-entry-profile.rst
> + for details.
> 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 Fri, 13 Sep 2019, Dan Carpenter <[email protected]> wrote:
> And the documentation doesn't help. For example, I knew people's rules
> about capitalizing the subject but I'd just forget. I say that if you
> can't be bothered to add it to checkpatch then it means you don't really
> care that strongly.
It would be entirely possible to add the subsystem/driver specific
checkpatch rules to MAINTAINERS entries or directory specific config
files. As checkpatch options directly. For example
--strict --max-line-length=100 --ignore=BIT_MACRO,SPLIT_STRING,LONG_LINE_STRING,BOOL_MEMBER
or whatever.
SMOP. ;)
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
Dan,
>> One pet peeve I have is that people are pretty bad at indicating the
>> intended target tree. I often ask for it in private mail but the
>> practice doesn't seem to stick. I spend a ton of time guessing whether a
>> patch is a fix for a new feature in the x+1 queue or a fix for the
>> current release. It is not always obvious.
>
> The Fixes tag doesn't help?
Always.
> Of course, you've never asked me or anyone on kernel-newbies that
> question. We don't normally know the answer either. I do always try
> to figure it out for networking, but it's kind of a pain in the butt
> and I mess up surpisingly often for how much effort I put into getting
> it right.
It's not a big issue for your patches. These are inevitably fixes and
I'll pick an appropriate tree depending on where we are in the cycle,
how likely one is to hit the issue, whether the driver is widely used,
etc.
Vendor driver submissions, however, are generally huge. Sometimes 50+
patches per submission window. And during this window I often get on the
order of 10 to 20 patches for the same driver in the fixes tree. It is
not always easy to determine whether a bug fix series is for one tree or
the other.
--
Martin K. Petersen Oracle Linux Engineering
On Mon, Sep 16, 2019 at 01:08:45PM -0400, Martin K. Petersen wrote:
> Vendor driver submissions, however, are generally huge. Sometimes 50+
> patches per submission window. And during this window I often get on the
> order of 10 to 20 patches for the same driver in the fixes tree. It is
> not always easy to determine whether a bug fix series is for one tree or
> the other.
I get the impression that a lot of vendors just don't distinguish and
only really care about getting things upstream, especially in the
embedded world many of them realistically expect to be shipping a pile
of out of tree patches to anyone using anything other than mainline
anyway so it's not super clear to them that it's worthwhile. This goes
back to the converstations about stable and how vendors interact with
that.
On Fri, Sep 13, 2019 at 02:48:50PM +0300, Dan Carpenter wrote:
> It used to be that infiniband used "sizeof foo" instead of sizeof(foo)
> but now there is a new maintainer.
These days I run everything through checkpatch and generally don't
want to see much deviation from the 'normal' style, a few minor
clang-format quibbles and other check patch positives excluded.
This means when people touch lines they have to adjust minor things
like the odd 'sizeof foo' to make it conforming.
Like others there is a big historical mismatch and the best I hope for
is that new stuff follow the cannonical style. Trying to guess what
some appropriate mongral style is for each patch is just a waste of my
time.
I also hold drivers/infiniband as an example of why the column
alignment style is harmful. That has not aged well and is the cause of
a lot of ugly things.
> There is one subsystem where the maintainer is super strict rules that
> you can't use "I" or "we" in the commit message. So you can't say "I
> noticed a bug while reviewing", you have to say "The code has a bug".
Ah, the imperative mood nitpick. This one is very exciting to explain
to non-native speakers. With many regular submitters I'm still at the
"I wish you would use proper grammer and sentence structure" phase..
These days I just end up copy editing most of the commit messages :(
> I don't think it's shaming, I think it's validating. Everyone just
> insists that since it's written in the Book of Rules then it's our fault
> for not reading it. It's like those EULA things where there is more
> text than anyone can physically read in a life time.
Yeah, I tend to agree.
The big special cases with high patch volumes (net being the classic
example) should remain special.
But everyone else is not special, and shouldn't act the same.
The work people like DanC do with static analysis is valuable, and we
should not be insisting that those contributors have to jump through a
thousand special hoops.
I have simply viewed it as the job of the maintainer to run the
process and deal with minor nit picks on the fly.
Maybe that is what we should be documenting?
Jason
On Tue, Sep 17, 2019 at 9:16 AM Jason Gunthorpe <[email protected]> wrote:
>
> On Fri, Sep 13, 2019 at 02:48:50PM +0300, Dan Carpenter wrote:
>
> > It used to be that infiniband used "sizeof foo" instead of sizeof(foo)
> > but now there is a new maintainer.
>
> These days I run everything through checkpatch and generally don't
> want to see much deviation from the 'normal' style, a few minor
> clang-format quibbles and other check patch positives excluded.
>
> This means when people touch lines they have to adjust minor things
> like the odd 'sizeof foo' to make it conforming.
>
> Like others there is a big historical mismatch and the best I hope for
> is that new stuff follow the cannonical style. Trying to guess what
> some appropriate mongral style is for each patch is just a waste of my
> time.
>
> I also hold drivers/infiniband as an example of why the column
> alignment style is harmful. That has not aged well and is the cause of
> a lot of ugly things.
>
> > There is one subsystem where the maintainer is super strict rules that
> > you can't use "I" or "we" in the commit message. So you can't say "I
> > noticed a bug while reviewing", you have to say "The code has a bug".
>
> Ah, the imperative mood nitpick. This one is very exciting to explain
> to non-native speakers. With many regular submitters I'm still at the
> "I wish you would use proper grammer and sentence structure" phase..
>
> These days I just end up copy editing most of the commit messages :(
>
> > I don't think it's shaming, I think it's validating. Everyone just
> > insists that since it's written in the Book of Rules then it's our fault
> > for not reading it. It's like those EULA things where there is more
> > text than anyone can physically read in a life time.
>
> Yeah, I tend to agree.
>
> The big special cases with high patch volumes (net being the classic
> example) should remain special.
>
> But everyone else is not special, and shouldn't act the same.
>
> The work people like DanC do with static analysis is valuable, and we
> should not be insisting that those contributors have to jump through a
> thousand special hoops.
>
> I have simply viewed it as the job of the maintainer to run the
> process and deal with minor nit picks on the fly.
>
> Maybe that is what we should be documenting?
In theory, yes, in practice, as long as there is an exception to the
rule, it comes down to a question of "is this case special like net or
not?". I'd rather not waste time debating that on a per-subsystem
basis vs just getting it all documented for contributors.
I do think it is worth clarifying in the guidelines of writing a
profile to make an effort to not be special, and that odd looking
rules will be questioned (like libnvdimm statement continuation), but
lets not fight the new standards fight until it becomes apparent where
the outliers lie.
On Wed, 11 Sep 2019 08:48:54 -0700
Dan Williams <[email protected]> wrote:
> As presented at the 2018 Linux Plumbers conference [1], the Maintainer
> Entry Profile (formerly Subsystem Profile) is proposed as a way to reduce
> friction between committers and maintainers and encourage conversations
> amongst maintainers about common best practices. While coding-style,
> submit-checklist, and submitting-drivers lay out some common expectations
> there remain local customs and maintainer preferences that vary by
> subsystem.
>
> The profile contains short answers to some of the common policy questions a
> contributor might have that are local to the subsystem / device-driver, or
> otherwise not covered by the top-level process documents.
>
> Overview: General introduction to how the subsystem operates
> Submit Checklist Addendum: Mechanical items that gate submission staging
> Key Cycle Dates:
> - Last -rc for new feature submissions: Expected lead time for submissions
> - Last -rc to merge features: Deadline for merge decisions
> Coding Style Addendum: Clarifications of local style preferences
> Resubmit Cadence: When to ping the maintainer
> Checkpatch / Style Cleanups: Policy on pure cleanup patches
So I'm finally back home after my European tour, and I have it on good
authority that my bag might even get here eventually too. That means I'm
digging through a pile of docs stuff I've been neglecting badly...
My intention is to apply these patches. But as I was reading through
them, one little nagging thing came to mind...
> See Documentation/maintainer/maintainer-entry-profile.rst for more details,
> and a follow-on example profile for the libnvdimm subsystem.
Thus far, the maintainer guide is focused on how to *be* a maintainer.
This document, instead, is more about how to deal with specific
maintainers. So I suspect that Documentation/maintainer might be the
wrong place for it.
Should we maybe place it instead under Documentation/process, or even
create a new top-level "book" for this information?
Thanks,
jon
Jonathan,
> Thus far, the maintainer guide is focused on how to *be* a maintainer.
> This document, instead, is more about how to deal with specific
> maintainers. So I suspect that Documentation/maintainer might be the
> wrong place for it.
>
> Should we maybe place it instead under Documentation/process, or even
> create a new top-level "book" for this information?
I think Documentation/process is the right place for all the common
practices and guidelines for code submission. Documentation is already
pretty big. And based on the discussions in this thread, I think we're
better off enhancing the existing process documents instead of
introducing more places for people to look.
--
Martin K. Petersen Oracle Linux Engineering
Hi, Dan,
A month or so ago I wrote...
> > See Documentation/maintainer/maintainer-entry-profile.rst for more details,
> > and a follow-on example profile for the libnvdimm subsystem.
>
> Thus far, the maintainer guide is focused on how to *be* a maintainer.
> This document, instead, is more about how to deal with specific
> maintainers. So I suspect that Documentation/maintainer might be the
> wrong place for it.
>
> Should we maybe place it instead under Documentation/process, or even
> create a new top-level "book" for this information?
Unless I missed it, I've not heard back from you on this. I'd like to get
this stuff pulled in for 5.5 if possible... would you object if I were to
apply your patches, then tack on a move over to the process guide?
Thanks,
jon
On Thu, Nov 7, 2019 at 12:13 PM Jonathan Corbet <[email protected]> wrote:
>
> Hi, Dan,
>
> A month or so ago I wrote...
>
> > > See Documentation/maintainer/maintainer-entry-profile.rst for more details,
> > > and a follow-on example profile for the libnvdimm subsystem.
> >
> > Thus far, the maintainer guide is focused on how to *be* a maintainer.
> > This document, instead, is more about how to deal with specific
> > maintainers. So I suspect that Documentation/maintainer might be the
> > wrong place for it.
> >
> > Should we maybe place it instead under Documentation/process, or even
> > create a new top-level "book" for this information?
>
> Unless I missed it, I've not heard back from you on this. I'd like to get
> this stuff pulled in for 5.5 if possible... would you object if I were to
> apply your patches, then tack on a move over to the process guide?
Sorry for the delay.
Yes, the process book is a better location now that this information
is focused on being supplemental guidelines for submitters rather than
a "how to maintain X subsystem" guide.
I do want to respin this without the Coding Style addendum to address
the specific feedback there, but other than that I'm happy to see this
move forward.
On 9/11/19 5:40 PM, Martin K. Petersen wrote:
> After the Plumbers session last year I wrote this for SCSI based on a
> prior version by Christoph. It's gone a bit stale but I'll update it to
> match your template.
>
Hi Martin,
The Maintainer profile is very helpful. Are you planning to send another
version and address Bart's comments?
Thanks,
Roman