netdev - [RFC] docs: netdev: encourage reviewers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231006163007.3383971-1-kuba@kernel.org>
Date: Fri,  6 Oct 2023 09:30:07 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: davem@...emloft.net
Cc: netdev@...r.kernel.org,
	edumazet@...gle.com,
	pabeni@...hat.com,
	Jakub Kicinski <kuba@...nel.org>,
	andrew@...n.ch,
	jesse.brandeburg@...el.com,
	sd@...asysnail.net,
	horms@...ge.net.au
Subject: [RFC] docs: netdev: encourage reviewers

Add a section to our maintainer doc encouraging reviewers
to chime in on the mailing list.

The questions about "when is it okay to share feedback"
keep coming up (most recently at netconf) and the answer
is "pretty much always".

The contents are partially based on a doc we wrote earlier
and shared with the vendors (for the "driver review rotation").

Signed-off-by: Jakub Kicinski <kuba@...nel.org>
--
CC: andrew@...n.ch
CC: jesse.brandeburg@...el.com
CC: sd@...asysnail.net
CC: horms@...ge.net.au

Sending as RFC for early round of reviews before I CC docs@
and expose this to potentially less constructive feedback :)
---
 Documentation/process/maintainer-netdev.rst | 43 +++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst
index 09dcf6377c27..d5cbcfd44cf8 100644
--- a/Documentation/process/maintainer-netdev.rst
+++ b/Documentation/process/maintainer-netdev.rst
@@ -441,6 +441,49 @@ in a way which would break what would normally be considered uAPI.
 new ``netdevsim`` features must be accompanied by selftests under
 ``tools/testing/selftests/``.
 
+Reviewer guidance
+-----------------
+
+Reviewing other people's patches on the list is highly encouraged,
+regardless of the level of expertise. Code reviews not only help
+the maintainers but also help reviewers themselves to learn and
+become part of the community.
+
+Reviewers can interact with the submissions by: asking questions
+about the code, sharing relevant experience, suggesting changes, etc.
+Asking questions is a particularly useful technique, for example rather
+than stating:
+
+``I think there can be a deadlock between A and B here.``
+
+it may be better to phrase the feedback as a question:
+
+``Could you explain what prevents deadlocks between A and B?``
+
+After all, patch submissions should be clear both in terms of the code and
+the commit message.
+
+Another technique useful in case of a disagreement is to ask for others
+to chime in. I.e. if a discussion reaches a stalemate after a few exchanges,
+calling for opinions of other reviewers or maintainers. Often those in
+agreement with a reviewer remain silent unless called upon.
+Opinion of multiple people carries exponentially more weight.
+
+There is no strict requirement to use specific tags like ``Reviewed-by``.
+In fact reviews in plain English are more informative and encouraged
+even when a tag is provided (e.g. "I looked at aspects A, B and C of this
+submission and it looks good to me.")
+Some form of a review message / reply is obviously necessary otherwise
+maintainers will not know that the reviewer has looked at the patch at all!
+
+It's safe to assume that the maintainers know the community and the level
+of expertise of the reviewers. The reviewers should not be concerned about
+their comments impeding or derailing the patch flow.
+
+Last but not least patch review may become a negative process, focused
+on pointing out problems. Please throw in a complement once in a while,
+particularly for newbies!
+
 Testimonials / feedback
 -----------------------
 
-- 
2.41.0