Code Review Standards and the Upstream-First Debate

Good afternoon, everyone. This is our monthly tech-sharing session, and today's topic is one that people have strong feelings about: how we review code, and whether we should adopt an upstream-first policy for kernel changes. I'll share some data first, and then I'd like a real debate. Nobody needs to agree with me.

I'm here mostly because every out-of-tree patch becomes my problem at rebase time. So I'm biased, but I'll try to listen.

And I'm here because review is the last line of defense before a vulnerability ships. I want to make sure security doesn't get lost in a conversation about process.

Perfect. Those are exactly the perspectives I was hoping for.

Let's start with the numbers. Our Orion kernel carries about eleven hundred patches on top of the Android common kernel. Of those, roughly two hundred are vendor drivers that will never go upstream. The rest are ours: fixes, tuning, and features. Only about forty of them have ever been sent upstream.

And each rebase costs us. When we moved from six-one to six-six for the internal test branch, three engineers spent five weeks resolving conflicts. About a third of those conflicts were in patches that fixed bugs upstream had already fixed in a different way.

So we were fixing the same bug twice, and then paying again to throw our version away.

Exactly. And nobody noticed, because our patch never went outside the building.

On the review side, our median kernel review takes about six hours from upload to approval. That sounds fast, but in a sample of fifty reviews, thirty-one had only one comment or none. Upstream, the same kind of patch typically gets several rounds of feedback.

To be fair, a review with no comments isn't automatically a bad review. Sometimes the patch is just good.

That's fair. But when I traced the eight regressions from the last quarter back to their commits, six of them had been approved with no comments at all.

Let me make the case. Upstream first means that before a generic kernel fix lands in our tree, it's posted to the upstream mailing list. We don't have to wait for it to be merged, but it has to be posted, and the link goes into our commit message.

Three things happen when you do that. First, you get reviewed by the people who wrote the code you're changing. Second, if upstream already has a better fix, someone tells you within days instead of us discovering it during a rebase two years later. And third, once it's merged, the patch disappears from our pile forever.

That third point is the one I care about. Every patch we carry is a patch somebody has to carry again on the next kernel.

Google is also moving in this direction. For GKI, they already reject vendor hooks that don't have a clear upstream story. So in some sense the decision is being made for us anyway.

I'll take the other side, because someone should. My worry is speed. When we have a launch-blocking bug, I need the fix in the build tonight, not after a public discussion that can take weeks. Upstream reviewers don't care about our launch date.

But that's not what the policy says. You post it and land it the same day. The upstream review runs in parallel. If they ask for changes, we pick up the improved version later.

Then what do we gain if we land it anyway? We still carry the patch until upstream accepts it.

We gain the feedback, and we gain a deadline for the patch to stop being ours. Right now there's no such deadline, so patches live forever.

My concern is different. Some of our changes reveal product plans. If we post a patch that adds support for a new display mode three months before launch, people can read it and guess what we're building.

That's a legitimate concern, and it's why I'd scope the policy to generic fixes, not new hardware features. A locking bug in the scheduler doesn't reveal anything about our roadmap.

Here's where I need to push back on everyone. Security fixes are a special case. If we find a vulnerability in the kernel, posting the fix publicly before devices are patched tells attackers exactly where to look. That has to go through the embargoed security process, not the public list.

Completely agree. The policy would explicitly exclude anything that goes through the security team. Those follow the coordinated disclosure timeline.

Good. My second point is about our internal reviews. I'd like any change that touches parsing of untrusted input, memory permissions, or the boundary between user space and the kernel to require a second reviewer from a different team. One comment-free approval isn't enough for those areas.

How would people know when a change falls into that category? Engineers don't always realize their patch touches a security boundary.

We can do it by path. We already have a list of sensitive directories and files. The review tool can add a security reviewer automatically when a patch touches one of them.

Let's talk about review quality itself, because upstream first only covers part of the tree. What do we actually expect from a reviewer?

For me, the reviewer should be able to explain the change to someone else. If you can't explain why the patch is correct, you shouldn't approve it. And the commit message should make that easy: what was broken, why, and how this fixes it.

Upstream maintainers are brutal about commit messages, and honestly, they're right. Half of our internal commit messages just say 'fix crash' with a ticket number. Two years later, the ticket system has been migrated and nobody knows what the crash was.

I've lived that. During the last rebase, I spent a whole day trying to figure out whether a patch was still needed, and the only clue was a ticket link that returned an error.

Fine, I can get behind better commit messages. That's cheap and it helps everyone. But I don't want reviews to become a ritual where people leave comments just to prove they read the code.

Agreed. The goal isn't more comments. The goal is that the reviewer actually understood the change. A short note saying what you checked is often enough, for example, 'verified the lock is held on all callers.'

Let me ask the practical question. Who's going to do this? Posting upstream means responding to feedback, sending new versions, sometimes arguing for weeks. Our engineers are already fully loaded.

I won't pretend it's free. My estimate is about ten percent of a kernel engineer's time. But compare that with five weeks for three people at every rebase. Over two years, it's cheaper to do it upstream.

There's also a hiring and growth benefit. Engineers who have patches in the mainline kernel get better at writing code that others can review. That skill comes back into our internal reviews.

I'll concede that point. Some of the best reviewers on my team learned by getting torn apart on a public list.

Can I add one practical suggestion? If we make upstream work visible, people will treat it as real work. Right now, nobody gets credit for a patch that's merged upstream, because it doesn't show up in our project tracking at all. If it appeared in performance reviews the same way a feature does, I think engineers would volunteer for it.

That's a great point, and it costs almost nothing. I'll talk to the engineering managers about adding upstream contributions to the quarterly goals, starting with the people in the pilot.

Let me make this concrete with a case from July. Some of you will remember the Orion Lite camera hang. The root cause was a patch in the workqueue code that changed how a flush was handled. It was approved in under two hours with no comments.

And here's the painful part. The same race had been discussed on the upstream list eight months earlier. A maintainer explained exactly why the obvious fix was wrong, and the correct fix was already in a newer kernel. If our engineer had posted the patch, someone would have pointed to that thread within a day.

How long did the hang take to fix in the end?

Three weeks from the first field report to the fix, plus a maintenance release, plus the SQE time to reproduce it. Compare that with the five minutes it would have taken to send an email to the mailing list. I'm not saying every case is like this, but this one cost us a lot.

That's a strong example, but I want to be careful about one thing. The engineer who wrote that patch was new, and the reviewer was overloaded that week. I don't want this discussion to turn into blaming individuals. The process let them down.

Completely agree, and that's why I didn't put any names on the slide. The question is what process would have caught it, not who should have caught it.

Before we talk about policy, I think tools can take a lot of the load off reviewers. Right now, our review system runs the build and a basic boot test, and that's it. Upstream, every patch is checked for style, and many subsystems run static analysis automatically.

We could add the upstream style checker and a sparse run to every kernel review in about a week. Neither of them finds deep bugs, but they remove the boring comments, so the human reviewer can focus on the logic.

On the framework side, we already have lint checks, and honestly they changed the culture. People stopped arguing about formatting and started arguing about design, which is what we actually want.

I'd also like lockdep and the kernel address sanitizer enabled on the nightly debug builds for Atlas. They catch exactly the kind of locking and memory bugs that slip through a quick review.

As long as they stay on debug builds, I have no objection. The performance cost would be unacceptable on user builds, but nightly builds are exactly the right place.

One more angle we haven't covered is vendor code. Qorvix and Arvenna send us large drops of kernel patches. Do we hold them to the same standard?

We should, at least for generic fixes. When I looked at the last Qorvix drop, about sixty patches had nothing to do with their hardware. They were fixes to the memory manager and the scheduler that Qorvix found on their own reference boards. Some of them were good, but none were upstream.

So we end up carrying their debt as well as ours. I'd like to add a line to the Atlas BSP agreement with Arvenna: generic core-kernel changes must come with an upstream link, or a written reason why they can't go upstream.

Is that realistic this late in the Atlas contract? The BSP 1.0 date is next week.

Not for 1.0, no. But the contract has quarterly updates for three years. If we ask now, it can apply from the first quarterly update. Arvenna's own engineers have told me privately that they'd prefer to upstream more, but they need a customer to ask for it.

Then let's be that customer. I'll raise it with the partner management team so it goes through the right channel.

Let me try to pull this together. I'm hearing agreement on a few things, and real doubts on others, so let me propose a pilot rather than a policy.

For the next three months, on the Atlas kernel only, generic fixes must be posted upstream before or on the same day they land, with the link in the commit message. Security fixes and new hardware features are excluded. Every commit message must explain the problem and the fix, not just a ticket number. And changes in sensitive paths automatically get a security reviewer from another team.

I'm happy with that. I'll send the list of sensitive paths to the tools team this week.

Atlas is a good place to try it, because it's still early and nothing there is launch-critical yet. I'd like to see the numbers at the end before we talk about Orion.

I'm still skeptical about speed, but a pilot is a fair way to find out. If it slows down a single urgent fix, I'll be the first to say so.

That's exactly the kind of feedback I want. I'll measure three things: how many patches we post, how many get merged, and how long it takes to land urgent fixes compared with today. Jiwoo, can you run an upstreaming office hour every Thursday for anyone who needs help with their first submission?

Happy to. I'll also write a one-page guide on how to format a patch for the mailing list, because that's where most people get stuck the first time.

And I'll add a field to the rebase tracker so we can see how many patches the pilot removes from our carry list. If the number goes down, that's the strongest argument for expanding it.

Great. Thank you all for a genuinely useful debate. We'll review the pilot results at the December session.