Review and Etiquette

You sent the patch. Now the part that is actually social.

This chapter is about what to do when a reviewer replies, when they reply bluntly, when they reject it outright, and — the most common case, and the one people handle worst — when nobody replies at all.

None of it is about being deferential. Maintainers do not want deference; they want patches that do not cost them time. Everything below reduces to that.

The Mechanics of a Reply

> On the patch, a reviewer quoted three lines and asked a question.
> 
> +	if (copy_from_user(&karg, uarg, sizeof(karg)))
> +		return -EFAULT;
>
> What happens if usize is smaller than sizeof(karg)? An older userspace
> would fail here where it should succeed.

You are right. I will use copy_struct_from_user() in v2, which zero-fills
the tail for an older caller and returns -E2BIG when a newer caller passes
non-zero bytes we do not understand.

> +	if (flags & ~LAB_F_ALL)
>
> Nit: LAB_F_ALL is not defined in this patch.

It is added in 2/4. I will reorder so the definition comes first.

The rules, all of which are about the reader's time:

Tip: Reply to review even when you agree and will just fix it. A one-line "Good catch, will fix in v2" costs you nothing and tells the reviewer their time was not wasted. Reviewers who feel ignored stop reviewing, and review is the scarcest resource in the kernel.

The Four Kinds of Feedback

When you think the reviewer is wrong

It happens, and handling it well is a large part of your reputation.

1. ASSUME THEY ARE RIGHT FIRST, and go check. They have usually seen this
   code path fail in a way you have not. Roughly half the time you will find
   they are right, and you will have spent twenty minutes instead of a
   round trip and some credibility.

2. If you still disagree, respond with EVIDENCE, not opinion:
     - a code path they may not have considered, quoted
     - a measurement, with the method
     - a link to a prior thread or commit that settles it

3. Say what would change your mind. "If X is true then you are right and I
   will do it your way" moves the discussion toward something checkable.

4. If it is a matter of taste, DEFER. It is their subsystem, they will
   maintain your code after you have moved on, and dying on a naming hill
   costs you the patch and the next one.

5. If you are wrong, say so plainly and move on. Nobody thinks less of you
   for it. Prolonged defence of a wrong position is what they remember.

What a NAK means

A NAK is not "revise this". It is "the approach is wrong". Responding with a v2 that ignores the objection is the single most reliable way to be ignored permanently.

The only productive responses:

• Understand it. Ask a clarifying question if you genuinely do not.
• Solve the actual objection, which usually means a different design, not a different patch.
• Accept it and go do something else. Not every idea should be merged, and the maintainer's job is largely to say no.

Silence

The most common outcome, and the one that makes people quit. It is ambiguous by construction, so resolve the ambiguity with data rather than feeling.

# Before concluding anything:
#   1. Is it on lore?      search your subject at https://lore.kernel.org/all/
#   2. Patchwork state?    https://patchwork.kernel.org/project/<project>/list/?submitter=...
#   3. Where in the cycle?
git fetch origin --tags && git describe --tags origin/master
#   4. Did it get applied without a reply? (This happens more than you expect.)
git fetch <subsys> && git log --oneline --author="Your Name" <subsys>/for-next | head

The ping, when it is warranted:

On <date>, I wrote:
> [PATCH] subsys: fix the thing
>
> https://lore.kernel.org/all/<message-id>/

Gentle ping. Happy to rework or drop this if the approach is wrong —
just let me know.

Once. Two weeks apart. In the original thread so the context is attached. Never a fresh angry email, never Cc'ing Linus, never "why is nobody looking at this".

Warning: Timing matters more than people expect. During the two-week merge window, maintainers are pulling, testing, and firefighting; a patch sent then may sit untouched for a fortnight through no fault of yours. Check git describe before you interpret silence as rejection.

The Trailers

These are attestations by named people. Understand exactly what each claims before you give or collect one.

Two hard rules:

Never invent one. Adding Reviewed-by: <someone> who did not say it is a serious breach of trust, and it is visible forever in git log.

Drop them when you rework. If v2 substantially changes the patch, the v1 review no longer applies. Drop the trailer and say so in the changelog: "Dropped Jane's Reviewed-by as the locking changed significantly." Carrying a stale Reviewed-by: across a rewrite is how you get a reputation you cannot fix.

Collect them mechanically rather than by hand:

b4 trailers -u          # reads the list, applies every trailer to the right commit

Producing a Version 2

# 1. Collect what you were given.
b4 trailers -u

# 2. Make the changes. Rebase onto the current tip of the target tree.
git fetch net-next && git rebase net-next/main

# 3. Verify it still bisects.
git rebase --exec 'make -j$(nproc) O=../build' net-next/main

# 4. Prove v2 is what you say it is.
git range-diff net-next/main..<v1-tip> net-next/main..HEAD

# 5. Regenerate, with the changelog under ---
git format-patch -v2 --cover-letter --base=auto --thread=shallow net-next/main..HEAD

The changelog goes below the ---, so it does not become permanent history, and it credits who asked for what:

---
Changes in v2:
 - Use copy_struct_from_user() so an older userspace still works (Arnd)
 - Reject unknown flag bits instead of masking them (Arnd)
 - Split the unrelated whitespace fix into a separate patch (Greg)
 - Dropped Jane's Reviewed-by: the locking changed substantially

v1: https://lore.kernel.org/all/<v1-message-id>/

Then reply in the v1 thread saying v2 is out, with a link. Reviewers do not watch for new threads.

Tone

Kernel review is terse. A reply that is three words long, points at a line, and moves on is not contempt — it is a person with four hundred unread patches operating at the only throughput that works.

Things that are normal and not personal:

• No greeting, no signature, no softening.
• "No." with a one-line reason.
• A correction of something you thought was obviously right.
• Being told to read a document.

Things that are not acceptable, and where you have recourse:

$EDITOR ~/kernel/linux/Documentation/process/code-of-conduct.rst
$EDITOR ~/kernel/linux/Documentation/process/code-of-conduct-interpretation.rst

There is a Code of Conduct and a committee that enforces it. Personal abuse, harassment, and discrimination are reportable. Bluntness about your code is not.

The distinction that matters: review is about the patch, not about you. "This is wrong and here is why" is review. If you find yourself reading terseness as an insult, re-read it as a very busy person being efficient, because that is almost always what it is.

Giving Review

This is Lab 9, and it is the highest-leverage thing a newcomer can do. Review is what maintainers are drowning in; someone who gives good review becomes a person whose own patches get read first.

The most useful review a beginner can give is usually: "I applied this and ran it on my hardware, here is what happened" — a Tested-by:. It is genuinely scarce, it requires no expertise you do not have, and it is exactly what a maintainer needs.

Common Mistakes and Their Symptoms

Validation / Self-check

1. Why inline, trimmed, bottom-posted replies? Give the archive-facing reason and the reviewer-facing one.
2. You agree with a review comment and will just fix it. Should you reply? Why?
3. Give the five-step procedure for when you believe a reviewer is wrong. Which step do people skip?
4. What does a NAK mean, and what are the only three productive responses?
5. Name the five causes of silence, and the correct action for each. How do you tell them apart?
6. How long do you wait before pinging, how many times, and where?
7. Distinguish Reviewed-by:, Acked-by:, and Tested-by:. Who may give each?
8. When must you drop a trailer you were given, and how do you communicate that?
9. What are the five steps of producing a v2, and which one proves the changelog is honest?
10. Where does a v2 changelog go, and why not in the commit message?
11. What is the difference between review that is blunt and review that violates the Code of Conduct?
12. What is the single most useful review a newcomer can give, and why is it scarce?

Next: Style and Checks — everything that will be run against your patch, and how to run it first.