kevin/linux
1
Fork 0
Code

docs: try to encourage (netdev?) reviewers

Browse Source 
Add a section to netdev 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".

Extend the section of 7.AdvancedTopics.rst which deals
with reviews a little bit to add stuff we had been recommending
locally.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
This commit is contained in:
Jakub Kicinski 2023-10-10 19:42:24 -07:00 committed by David S. Miller
parent 4d825faf3e
commit 6e55b1cbf0
2 changed files with 33 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
Documentation/process/7.AdvancedTopics.rst
View File

@ -146,6 +146,7 @@ pull. The git request-pull command can be helpful in this regard; it will
format the request as other developers expect, and will also check to be format the request as other developers expect, and will also check to be
sure that you have remembered to push those changes to the public server. sure that you have remembered to push those changes to the public server.
.. _development_advancedtopics_reviews:
Reviewing patches Reviewing patches
----------------- -----------------
@ -167,6 +168,12 @@ comments as questions rather than criticisms. Asking "how does the lock
get released in this path?" will always work better than stating "the get released in this path?" will always work better than stating "the
locking here is wrong." locking here is wrong."
Another technique that is useful in case of a disagreement is to ask for others
to chime in. If a discussion reaches a stalemate after a few exchanges,
then call for opinions of other reviewers or maintainers. Often those in
agreement with a reviewer remain silent unless called upon.
The opinion of multiple people carries exponentially more weight.
Different developers will review code from different points of view. Some Different developers will review code from different points of view. Some
are mostly concerned with coding style and whether code lines have trailing are mostly concerned with coding style and whether code lines have trailing
white space. Others will focus primarily on whether the change implemented white space. Others will focus primarily on whether the change implemented
@ -176,3 +183,14 @@ security issues, duplication of code found elsewhere, adequate
documentation, adverse effects on performance, user-space ABI changes, etc. documentation, adverse effects on performance, user-space ABI changes, etc.
All types of review, if they lead to better code going into the kernel, are All types of review, if they lead to better code going into the kernel, are
welcome and worthwhile. welcome and worthwhile.
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 or reply is obviously necessary otherwise
maintainers will not know that the reviewer has looked at the patch at all!
Last but not least patch review may become a negative process, focused
on pointing out problems. Please throw in a compliment once in a while,
particularly for newbies!

15
Documentation/process/maintainer-netdev.rst
View File

@ -441,6 +441,21 @@ in a way which would break what would normally be considered uAPI.
new ``netdevsim`` features must be accompanied by selftests under new ``netdevsim`` features must be accompanied by selftests under
``tools/testing/selftests/``. ``tools/testing/selftests/``.
Reviewer guidance
-----------------
Reviewing other people's patches on the list is highly encouraged,
regardless of the level of expertise. For general guidance and
helpful tips please see :ref:`development_advancedtopics_reviews`.
It's safe to assume that netdev 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.
Less experienced reviewers are highly encouraged to do more in-depth
review of submissions and not focus exclusively on trivial or subjective
matters like code formatting, tags etc.
Testimonials / feedback Testimonials / feedback
----------------------- -----------------------