 Okay, so this talk is about Linux kernel code reviews and as an introduction, this is a talk that I gave, actually it was a formal Intel university class that I taught at Intel last August, actually. And it's from the perspective of a vendor tree provider. So for the past, oh jeez, it feels like 20 years, but for the past seven years or so I've been working in the Android space providing on the Linux kernel for Android at Intel since about 2009 or 2010. And so this is based on my experience from doing that. So I just kind of kept it in a course lecture format just because I thought it would be easiest for me. So I'm just going to review the goals of the course and we're going to talk, and there will be just a handful of fairly lame or tame examples coming from this GitHub tree, which is really, what this is, is this is a rebase of an Android things kernel that from a 4.4 kernel to a 4.9 kernel and I'm trying to get mature to be a base for, sure enough to feed actually the YAKTA meta Intel layer. And when I first thought about doing this course, I thought it would be hilarious to basically throw up a lot of examples and talk smack about the patches. But then as I got time went on, I started thinking more, he's like, yeah, I don't know if I really want to be shown internal code. And I'm pretty sure I don't want to take code from other vendors and embarrass people too much. So I decided just to, you know, so the examples aren't as expressive as the examples I have for the internal class. And that's just the way it is. So I will describe some patches as we go as well. If you have a burning question, go ahead and interrupt me. But otherwise, I just kind of want to go through the class, go through the lecture and then take questions at the end. So the goals of this class, so basically by the end of the class, I really want people to kind of understand what code reviews are expected to look for as you, oh, I also want to back up just one little bit. This is, I want to make clear that these code review practices are important for people working on non-upstream code. So this would be like a Linux stable tree or a tree that isn't tracking upstream. So a number of the things don't make sense when applied to upstream development. So keep that in mind. So by the end of the class, you'll understand what the code reviews are expected to look at when they're looking at your code. And hopefully you'll get a little bit better appreciation of what's expected from your code during a review. You'll also hopefully understand the mindset of customers a little better. You'll understand the utility of having a good prefix discipline. You'll be able to identify a problem with a patch that needs to be fixed and you'll be able to explain what the issue is with the patch. So I left this in here just as a reference because I believe that everybody working on any kernel code should read these two files, the submitting patches and coding style about twice a year. So they're here for your reference so you can refresh your memories. So what do customers think of you and your code? Well, customers actually don't trust you and they don't trust your code. They do not want to see changes after beta releases other than bug fixes they care about explicitly. They scrub every change in detail that you provide as you provide your kernel. And as work progresses, you give them an initial release and they start working on it themselves and you keep working on it and as you provide releases later and later in the program, they really get resentful of your updates. And if they could bill you back for the overhead associated with refreshing their tree with your update, they would do that. It's not really a good relationship, typically. To be fair, I have my own opinions about the customer's kernel practices. So basically customers are system integration teams, system integration and system debug teams. They can't understand how to use anything but Garrett in a mechanical way to manage their technical debt. They don't plan for security maintenance. They don't appreciate security maintenance. They don't plan for rebases and kernel migrations. They don't upstream their bug fixes that they depend on and by upstream I mean feed them back to me as a provider of the kernel to them. They don't provide visibility into changes they're making and they're driven by purely time to market risk and short-term cost and long-term program viability. They just don't care. Let's see. I think I wanted to say something a little bit more about this. Okay. Let's see. I did have another thing I wanted to say. One of the reasons why we... I'll get to it later. I'll get to it later. Okay. Some high-level policies. Pretty much as you do in a code review, you need to make sure that a good commit comment prefix discipline is applied. And the reason why we need this discipline is because you may be working on the 4.4 kernel today but you're going to be working on the 4.9 kernel or some other kernel tomorrow. And without having this discipline migrating your code from one kernel version to the other is almost... You can't do it. I don't think you can do it. And as a code review, it's important for you to review this policy because as a member of the kernel team, assuming you guys are a part of the kernel team, it's going to be your job to migrate to that new kernel. So this is to cover your butt. Okay. So this is a copy and paste from that slightly modified copy and paste from that URL from the Brillo code. So this is essentially the commit prefix conventions that the Chrome OS team created and they expanded and they continued it on and they're applying it in the Brillo space. So essentially, there's a number of these prefixes that they want you to put in the front of the commit comment. So you have upstream and what upstream means is basically you were able to successfully do a git cherry pit dash capital X from the upstream kernel to get whatever patch you need into your older kernel. And there were no merge commits. Backport, it's just like upstream except there is a commit... There is a merge conflict and you had to fix it up. The from list, this is... I use it for both from mailing list and from feeder trees that are feeding Linux next but essentially this is code that isn't yet in upstream... By the way, upstream means kernel.org minus this tree. From list means to me it means it either came from a mailing list or it came from a developer maintainer tree that's feeding Linux next. So this is basically things like I have some USB patches from Felipe. I don't think they're actually on any mailing list but they're in his tree that's queued up for Linux next. So that's what you would prefix your patch if you got it from there. Then you have some OS specific patches. These can either come from the... In the Android and Brillo case, they come from the AOSP command.get project. They have a bunch of reference branches. But these could be... And I have an example later associated with Yachto. But they could be anything that's really not upstreamable or not upstream yet that is important for a particular operating system. The vendor tag, this is where most of the patches should be. That your debug team will be creating patches and stuff like that. And new drivers that haven't even started on the upstream path, you'll just put the vendor tag on it and put your name by it, or the vendor name by it. Like vendor Intel for example. RFC, I haven't actually used the RFC one or seen it used yet but from the text, it's basically if you're too impatient to wait for it to actually get into something that's feeding Linux next, use the RFC one. And I got some examples. So these are two simple bad examples that I want to try to make the point of imagine a tree with about anywhere between 500 and 5000 patches. And more than half of the patches look like these two. And you're given the job to migrate these patches to a new kernel version. Say you have a product that has an extended lifespan and you're concerned about security updates and being able to maintain security updates. This is the kind of thing that will hurt you, ruin your day. Let's see, it's 741. Let's see. Oops. Darn it. No. This one. Okay. So here's a patch that basically says it corrected some value and it has a bug number. And then it does some stuff and it's actually not a bad patch. But the commit comment is hideous. Okay? Because what's missing from this patch is what's so bad about having an incorrect TSF value? I really don't know. What's in this bug number? What fails? Is there a testable issue that says, oh, if I don't take this patch, this happens. This bad thing happens. There's no clear reason why it's important to fix this, have a cracked TSF value here. Now, clearly I'm showing I don't really understand 802.11 very well. It might actually be really obvious if you're a network guy. But in general, when the customer gets this, they won't have access to this bug database. They won't be able to look that up. And I can't tell if this is actually a really important thing that needs to be ported forward or not. Let's see. My next one. Is this it? Yes, this is it. So I was hoping Matt would be here because I wanted to tease him. Okay. So here's an example from a friend of mine. It's another example of a one-line patch. And it also uses revertMe. What does that mean, revertMe? When will it be appropriate to revert this patch? I can't tell. It's developers seem to think that wedging revertMe into the commit comment title is a get out of jail free card. It's not because so I've had to port this patch from 4.4 to 4.9 and I don't know if it's important. I'm pretty sure it is. Flushing the firmware from the cache is probably important to do but at least before you start up the device. But we have cache coherency on our hardware so I'm not sure if this is actually needed. So other than that, it's actually a fine patch. It does what it says, well, it does something with flushing cache so that's okay. It could be worse. Okay, so there's those too. Yeah, so you really kind of need to think about, imagine yourself rebasing these things. Oops. Let's see if I got that okay. Also, I saw a talk yesterday and the person was looking for good reasons to justify upstreaming their code. And one of the things that I really think is it should be an easy sell for most businesses is to just talk about the security aspects of their and the security liability of their code. If your management and engineering culture is in the mindset that okay, we're not network connected. We'll never have to update the kernel. We're all good. We can use a 3.4 kernel if we wanted to and get away with it and never do security patches then okay. But that's not us and that's not what we're working toward. So hopefully I provided some motivation. Now I'm just going to kind of talk about what's the structure of a code review. So for my team, basically the engineers are expected to develop for, or they're expected to review for supportability and correctness. And supportability is, is it clear what will happen if this patch isn't accepted? And will this patch be supportable one year from now when the authors aren't there? And how hard is this patch going to be to migrate to a new kernel version? Because you're going to do the migration, not the author in most cases. And then for correctness, you need to check for bugs and coding standards. And is it well aligned with upstream? I'll talk more about those through the talk. So supportability, basically supportability mostly relates to commit comments and inline comments. So does the commit comment, oh, by the way, these slides are, well, earlier versions of these slides are already up on the website, or the conference site. And I'll refresh them with the final version here after, later today. Does the commit comment explain what's going to break and why it's important to take this patch? You know, does it explain what the change is? And does it actually match the change? Because I've seen more than a few patches where the developer cut and paste a commit comment from one patch or one change to the next and sent it my way. And that's kind of annoying. As you're reviewing these commits, you need to think about the commit as a sentence. You know, they need to stand alone just like a sentence in a story or in a paragraph. And if they don't stand alone or if they're a sentence fragment or they're basically incomplete. And I really dislike those. The typical example of a sentence fragment would be someone adding a data member, changing a data structure in a header file, but never dereferencing that change anywhere else in the patch. That's quite annoying. Another thing that you need to look out for, which actually hit me when I did that rebase, when I rebased those changes from 4.4 to 4.9, was there was a patch that said, there's a patch that used to be in there that talked about adding a worn on or a worn once in a file. And after the rebase, the worn on actually fell out. So it was a patch that I probably didn't need to rebase at all. But I ended up with a patch that actually didn't match the commit comment. So this can happen to people by accident and on purpose. You can either get it to happen just by being lazy or just not as careful as you really need to be. So after a rebase, you actually really have to kind of re-review these patches from the perspective of supportability again. So, look out. Locking, basically these are my hot buttons when I see people do stuff. So locking needs to be well-documented, what the lock is actually protecting and it needs to make sense. Magic numbers really need to be traceable to specifications whenever possible. And if they're experimentally derived, admit it. And actually in the comment, you probably ought to tell me what's going to go wrong if I use a different number there. Typically this happens and bring up people's sprinkle delay loops all over the place. And then later on in the program, those delay loops you start optimizing for boot time or performance or something. And now you start, you find these delay loops, they just jump out at you as you do your perf analysis. And when you're sitting there scratching your head, well, is it safe to remove that delay? And you have to rediscover the failure case when you change the delay. And that's really kind of rude. So you're just being rude to your future selves or somebody else. Either way, it's rude. Complicated and confusing logic needs to be, needs to be, have some helpful inline commenting. Print K's and log messages. So as a reviewer, you've got to look at these print K's and log messages and think in terms of each one of these is a possible support call. And so you really want to make all your log messages and your print K's count. They need to be kept to a minimum and they need to be useful. Just, I was here, or Kilroy was here kind of print K's, aren't that helpful. Oh, reverts. Yeah. So you review this kernel code. Reverts are very common in a integration environment because you have a system integration lead who is revert happy and they smell, if they smell a regression they just revert the thing and ask questions later. So if you make them, if the developer makes the mistake of wedging in multiple features or multiple changes and bug fixes into a single patch, that patch, all those features are at risk for revert, getting reverted. And you really don't want to be the guy who forces, you know, forces a team to make a hard call between accepting all these features with or the revert. You know, it's, they don't want to accept the revert typically. So make sure that, don't accept patches, basically don't accept patches that mix, mix bug, multiple features or bug fixes in a single go. Just because, actually, maybe multiple bug fixes is okay to mix, but adding features and bug fixes, you're gonna, you're putting your feature at risk. Don't, don't do that. Another thing that the code reviewer needs to do is basically you need to imagine, as you're looking at this patch, you need to imagine yourself, you know, basically a year from now or something when the developers are no longer available and there's a big escalation, let's say it's a security issue, you know, and there's a patch that's identified as the problem. Is there enough information in this patch that you can understand what that patch is about and perhaps make some progress on it? And, and you really need to put yourself in the customer's position, in the customer's shoes, because they don't have access to the original developers and you might after one year, you probably won't, but you know, you really kind of need to think of that. Or another way to think of things is, you know, you know, think of yourself two years from now, assuming you stay with the team, you know, and you're forced to, you're forced to rebase this, this set of, set of beauties to a new kernel, you know, you're going to be able to do that and do a good job of it. Because just mechanical rebasing, I mean, rebasing is fairly mechanical, but you really, you know, like I said earlier, you know, with that example, you really kind of need to look at the patches as you do the rebase. You know, it's easy to fall into doing a git rebase or some other rebase and just kind of plowing through and you don't really look at the patches, you just try to get them to rebase and compile, right? And then once it compiles, you move on is typically the behavior, but you really have to go back and review the patches again, the properly. And so make sure the patches are good. You're just trying, I'm just trying to protect you from yourself. You're trying to protect your future self from your, your today self. Okay. And just want to show a patch that I consider is not too bad. It's over here. Is this the right one? Nope, that's not the one. That's not it. That's not it. Shit. Should be here. No, I think that's it. Yeah. Okay. So here's a super trivial patch. It all fits on one screen. Essentially what this patch does is it replaces binoc with user binoc. And this patch is actually, you know, uh, you know, the commit comment, you know, we see you got the OS. It's for Linux like Yachto. And, you know, it says, you know, it says use, use user binoc instead of binoc. And essentially the reason why we need this patch is because, uh, the Pocky build will, will basically fail when it tries to get the version from the Linux with, without this patch. So you know that if you don't take this patch, you're going to break, you're going to bake Yachto essentially. Um, incidentally, the reason why this patch exists is because someone from Red Hat, and apparently Red Hat likes binoc for where it likes to put its Ock program. Uh, but everybody else kind of likes user binoc. So this is just to work around a change that was put in. Um, but this like that, consider this a pretty good patch. This, this would be hard to say no to. And this is what you want all your patches to be. You want all your patches to be hard to say no to. Um, you know, you'll have testable behaviors associated with not taking the patch. You know, that's, um, you need that motivation on the patches. Okay. So now I'm going to shift gears and talk about correctness. Um, uh, let's see. We're, we're doing pretty good on time. Um, so, you know, I was doing a code where you need to look for correctness, you know, so you need to do all the normal things, you know, is the logic correct? You know, is there a cut and paste error? Are the error paths correct? You know, are you leaking, uh, are you leaking locks or, or, or memory? Uh, does code make sense? You know, sometimes, you know, a lot of times it might not. Um, uh, does, does the code match, does the code match to commit comment? Um, I know this is kind of redundant to supportability, but if it, if it doesn't match to commit and it's just not correct, you know, and, and, uh, the, uh, is there, is there justification for the patch, uh, that actually doesn't need to be there, but, and is the code aligned with upstream kernel.org directions? You know, so, um, example of this is, um, recently, actually a few weeks ago. Um, we actually last week, um, uh, we had this patch. Um, I had this patch come past me and I had to reject it, you know, and essentially what this patch did was it did a really nasty hack to a function called, um, alarm suspend, alarm timer suspend. You know, basically it was no hopping out that function, uh, because that function on this particular, on this particular platform, uh, resulted in suspend, suspend behavior, uh, not being what people wanted, uh, or, or could tolerate, uh, for the program. Uh, the suspend architecture of this, this particular device, uh, or platform, uh, was different because Android is running in, in a virtual machine in this, in this, in this case. So having the RTC timer wake up, uh, really wasn't welcome in this, in this, in this particular use case. And, uh, so anyway, they, they know opt, they, they just deleted everything in that function and put a return zero in it. Uh, and I said no. And then I, you know, and I looked more closely and I noticed that, hey, there's a, there's a config macro wrapping that function. You know, I wonder if you could just change that config and get the same, same result, right? You know, and it turns out that's what, that's, that's what happened. You know, so you, you need to look, you know, and that's what I mean by upstream alignment. Um, that sort of thing, that's, that's a fairly good example of an upstream alignment. Another, another example, which I haven't seen in a few years, but it has happened in the past where people will forklift from a older kernel, an entire subsystem to a newer kernel, you know, and just don't say no to those if you can. Sometimes you get forced, um, memory allocation and deallocation. You know, check, check, check for memory leaks, you know, um, uh, look at it, question every global you see, you know, is the global really necessary? Is someone just trying to be lazy and not pass something by value and just put a, you know, wedged in a global? I've seen that happen. Um, you know, and then for every global that you see added, you need to check, you know, you need to interrogate it or question it with respect to, does it need a lock? And it probably does. Um, so globals are a pain. Um, locking, locking is an even bigger pain. Um, so you need to check to see if the locking is sane. Sometimes it won't be. Um, uh, I know it locks, protect data, not code from concurrent access. Um, so, so there's, there's a story that I was, that I, that I, that I tell with this is, um, so locks are kind of like capacitors on hardware, uh, to me a little bit. Um, so at least, at least, uh, non sane walking is a lot like capacitors on hardware. So hardware engineers will come in. So, so a friend of mine who is a hardware, a hardware guy at Intel. Um, you know, I'll show him, this is like years ago when I was making, making some little robot things and was talking about some problems I was having with the board stability, you know, and he told me this story about the cap fairy. So apparently in hardware circles, there's this, there's this phenomena about being visited by the cap fairy. So what this boils down to is you have a board that's not stable and then one morning you come in and the board's better, you know, and, and there's a whole slew of new capacitors sprinkled across the board, you know, and it makes things better, you know, and, and, and, you know, it's, you don't want a lock fairy coming in and sprinkling spin locks around trying to make things better. It, it, it's happened. I've seen it. Haven't seen it happen lately, but it used to happen. Don't let it happen. Don't let, you know, where, where locks are taken need to make sense. And so be careful with your locks. Static analysis, you need to review your static analysis. So we have, we have some, we have some CI tools associated with the patches as they come in and they run, they basically run III and some other things. Some security analysis as well. And, and you need to, as the code reviewer, you need to review this stuff, you know, and make a decision whether or not it needs to be enforced or not. So, you know, oh, go ahead. Not in header, I haven't seen too many in the header files. Most of the security, the static analysis security checkers, you know, the commercial checkers, they tend to give me false alarms on string copies mostly, you know. Yeah. You'll have a lot of false alarms. I know. Yeah, yeah, yep. It's true. That's why you need a human to look at them. Yeah. And that's, that's, yes, that's been my experience with, with that tool as well. Yeah. Yeah. Don't let new compiler warnings in. And I'll talk about, I'll talk about check patch I think next. Okay, about check patch. So, check patch is a annoying thing. If you, if you, if you enforce check patch compliance in a draconian manner through CI tools, you will end up with a lot of ugly looking code, you know. It's, check patch will, will also will sometimes call out errors that you can't work around. So, you need, you need to, you need to review the check patch and use your, and use your head. You know, check patch is great. You need to, you need to run it, but you need to review it, review the output. One case, one case we had with check patch was the, I think the audio team was adding, was adding some back ports, was doing a back port from upstream. So, this is code that was already upstream. They're doing a back port, adding it to some old kernel I was working on. And it had a fairly complicated macro in it. And check patch was flying in this macro. It was being too complex. And, well, you know, it's, it's a change I can't, I, we couldn't fix. We needed that change. And so eventually what we did is we just disabled check patch until we got that patch merged and then we turned check patch on again. It was, it was really, it was really lame. But, you know, so I really prefer human reviewers to review these output of these things, not, not just a robot. And as a developer, if you're, if you're submitting code that you know it's got some check patch issues, be ready, be ready with, get, you know, be ready to be challenged, you know, because we're going to ask it, do you really need that? Or should that be fixed? Why, why is this okay? We'll be what you will be asked. Security scanning, you know, you need to review the security analysis and you need to be very careful before you, you let the patch in, you know, it's, it's, or disposition the issue is false alarm. So, you know, it's, as you get practice looking at this kind of stuff, you get fairly quick at it. But, you know, it can, you know, like you said, with 1500 false alarms, it's like a great, you know, it's, but ask for help before you disposition a possible security issue. And I know this isn't, I don't think I have a slide to talk about this story, but one time, you know, if you see a patch that's clearly doing stuff that's security related, you know, like checking permissions before doing stuff. You know, get, you know, it doesn't hurt to get expert, expert review on it, you know, ask, ask, ask someone who might know better. You know, for example, there was some patch that was ported that was, I forget what it was for. It was, I think it was for changing some C groups, some C, C group properties. But it was, it was, it wasn't using the, it wasn't using the same security check that, that P trace uses, and which is, you know, which is what was, which was what was proper for this, for this particular scenario. And, you know, I saw that patch, it wasn't really sure if it was good. And, you know, I'm lucky enough to work with some, some really, really great upstream people. So I asked Ari and Vendorvin to take a look at it and ask, you know, and he gave me, he told me, oh, that needs to use, use the same check that P trace uses, otherwise it's wrong. You know, it's, it's, it's good to have experts around that you can ask for help. I'll talk about asking for help more later too. Another thing about correctness is, is the code compliant with your business policy, you know, is it in compliance with your business policies and legal, legal and business policies? You know, does it leak confidential information? This confidential information could be a customer name, it could be a code name of a project. You know, you just, you just need to look at the commit comment and ask yourself, is it okay to have this in GPL v2 out in the wild? Just for the comment, commit comments. If you're an IP vendor or, or a silicon provider, you know, is, is this patch touching any undocumented registers that were previously not documented, you know, or using, you know, secret new MSR register magic numbers? That's basically, you're leaking, you're leaking IP if you do. So you need to check to see that everything's approved. Most, I doubt if, I don't expect a lot of people to have to do that in this, in this room other than me. Oh, well, Intel people. You need, you know, do business approvals exist for the IP that the code's for? And another thing you need to check for correctness is where was this, where was the content sourced from? So I'll talk about that more in this next slide. So a common thing that, that people will do when they're developing code is they'll, they'll look around at other trees and they'll harvest, harvest code. And when you harvest this code, it's really, really easy to do it in a way that, that results in you being named as the author of the code. And, and you really don't want to be blame, be, be called out as the person that was stealing someone else's code. Even though it's GPL, you don't want to, you, you want to respect the copyright and the authorship. It's, it's more than just a plagiarism issue. You know, the easy fix to this problem is to use get-amend author and fix it. You know, so you need to make sure your developers know this, this, this get snippet. But, you know, it, it's, it's more than plagiarism. You know, it provides, it provides trust. It, it, it provides a herd mentality to the customer that, oh, okay, this, this patch was taking from, or this code was taking from, taken from, say, the Cyanogen mod Samsung 5X kernel, which I have, have, I do have, I did have an example like that in the internal class. That is just a hideous tree, by the way. It's, it's just, it's got 30, everything squashed into one commit. You know, it's got like 30,000 lines of changes. And it's, it's awful. Anyway, but, but you need to, you need to, you need to call it out so that, so that you can provide a high, a herd mentality a little bit to, to provide more credibility to your patch. Okay, what does a good code review look like? So, basically a good code review helps good code getting quickly. That's the biggest thing. And, you know, you're, of course, you got a review for supportability and correctness. And, you know, in, in an integration environment, you're going to have crappy patches that are, that are, you know, really needed to get in quickly, but they really suck. And you're, you're going to have to, you know, as a, as a, as a code reviewer, you're expected to roll up your sleeves and help fix it, you know. And then try to put in place some follow-up commits, plans for follow-up commits to, to fix the issues. It's really kind of a bitter pill to swallow as a code reviewer, as a kernel maintainer. But you, you kind of need to do that, be able to do this as well. More details about what makes a good code review. You need to provide actionable feedback to the, to the author. You know, you need to make clear what needs to change in order for the patch to get merged. It's not good enough to tell the developer that you make my eyes bleed. You need to actually, you know, give them feedback that they can do something with. You know, so you need to explain why the code needs to be changed. You also need to try to, try to make sure that, yeah, you, you review the whole code one go. You know, because, you know, this, this can be hard to do if the, if the patch doesn't, doesn't, if the patch doesn't get passed, you know, the, the, the static analysis very well and is really a turd. You, you, you, it's really hard for you to do much with it other. But, but you can, you can throw it back and say, make, you know, make, make it pass all the static analysis checks and then I'll, then I'll review it, which is, which is fair. But after it passes all those, those, those items, you need to review the whole patch and attempt to, to get all these, get all the issues enumerated for the, for the developer to fix. And this is important because, you know, well for, for Intel it's important because we have a, you know, a worldwide engineering team and we really can't afford to have a, have a 24 hour turnaround time for every, every bug fix. Oh, three minutes already. Okay. I think I'm pretty close to done. So you don't want to, you don't want to start playing games with the, you know, with, with, with this sort of thing. When discussions stall, you got to reach out to the developer and, and to management. Many times developer will, will decide, you know, I've reviewed patches and I bled all over it and I threw it back at them. And they then I get radio silence in return. You know, so I spent, I spent hours reviewing that patch and then they just blow me off. And what they're doing is they're, that many times they're playing scheduled chicken with me because they go, oh, okay, I'll just wait close, wait till we get close to the milestone. Then I'll send it your way again and see if you change your mind. Because now you're going to have management crawling up your backside to make that, to get that thing in there. Don't, don't, don't let them do that. Talk to their managers and chew out their managers and escalate it. Don't, don't be afraid to ask for help. A lot of good learning you can do. A lot of good growing opportunities when you get something, you know, when you consult some expert. Give examples. Be patient. Key takeaways. This is like my last slide. Hopefully you, you understand how the patches are going to be used by customers. You're going to understand the, the, the importance of reviewing the patches from the point of view of, of that, with the assumption that you're going to have to reuse these patches across different kernel versions and you're going to have to execute a kernel migration at some point. You know, use good prefix conventions. Stop writing bad patches if you're a developer. And if you write bad patches, I certainly won't like you. And I guess that's it and I'm pretty much out of time. So, awesome.