U-Boot patch submit standard and requirement

[Quentin Schulz]

Hi Brian,

On 5/14/26 3:51 AM, Sune Brian wrote:
> Hi Tom,
> 
> Sorry to bother you.
> 
> I am curious that for me myself I had no issue to follow
> the requirements [1] as long as all patches that are
> passing the review stage do follow the rules in [1].
> However based on most recent commits and reviews
> most of those are not even close to what [1] mentioned.
> 
> So at the end, reviewers in U-Boot just made their own
> standard and requested contributors to follow?
> 
> Rather the U-Boot itself should all follow the docs rules?
> 
> [1] https://docs.u-boot.org/en/latest/develop/sending_patches.html#sending-updated-patch-versions
> 

If you feel like other people's patches aren't properly formatted 
according to the docs, you have two options:

- review them on the mailing list and tell them to follow the format as 
specified in the docs, like Simon did for your patch,
- suggest updates (or send patches!) to the docs to better match what is 
followed by the project,
- if they aren't about the commit log, you can send follow-up patches to 
fix the source code to better match what we document, (but you cannot on 
commit logs as those cannot be modified once merged),

If you do not understand why someone is requesting changes, ask them to 
explain or point at the documentation. If you disagree with the reasons, 
you can *politely* tell them that by providing arguments. However, 
maintainers are free to go the "agree-to-disagree" way and refuse your 
patch. You have to work with them to get your patches merged, sometimes 
it means being really patient.

Maintainers are famously overworked in any open-source project you can 
think of. By following the rules (which sometimes are implicit), you 
make their life (and the life of reviewers) easier as it's less effort 
for them to review as it follows some patterns they are used to.

Maintainers are people, like you, like me, they also make mistakes or 
give some leeway because it's the 15th version of a patch and required 
changes aren't worth going back and forth with the contributor. This is 
the *maintainer*'s call, and they're free to decide to do it for one 
patch of one person or not. It's a fallacy to say "this patch didn't 
follow a rule and got merged so I'll not follow this rule". At that 
point, why even write a commit log? Why even have comments? Why even 
care about the code compiling without warnings? Why care about security 
holes in the patch since other patches got merged with security issues?

I've learned over the years to not answer when I'm getting angry because 
someone doesn't understand what I'm writing or they have unreasonable 
requests. You wait a few hours, or a few days, and you read again the 
mail with a cold head and you usually read with a different perspective 
and you can now stay polite with them (and sometimes you understand what 
you wrote was confusing because it could be understood another way). 
Getting angry, especially at maintainers, will be a sure way to get 
people to ignore you. Since they are not your colleagues they perfectly 
can decide they don't want to work with you. Yes, it's unfair.

Remember also that people are not in your head, so providing context in 
your patches or emails is really important. The more information and 
context you can provide, the less the person reading your mail will have 
to think or guess before they can review your patch.

For example:

- 8f41874f4631 ("fix socfpga GEN5 handoff script path"). When was src 
variable removed? If I had reviewed that patch, I would have looked for 
a commit that removed that variable and it may not be obvious so it'll 
take me time that you likely have already spent yourself.
- bb1c2b463265 ("update GCC version check after Kbuild bump"). What 
errors did you see? Did you test any other toolchain than GCC 10.0.1?
- 5b1fe6ef6b81 ("Altera SoCFpga Boot Stall Fix"). Where does it stops in 
the boot process? What's the boot log? "After testing", what kinds of 
tests? There's no explanation as to *why* this patch works, only that 
it's imported from downstream fork from Altera (that you say is 
"official"). I can understand this as "I don't understand what this is 
doing but it works", which some maintainers may accept, some may not.
- e291277689f6 ("sync socfpga common u-boot dts". The Device Tree node 
would be present in U-Boot proper. So the issue is that you're missing 
L2 and memory controllers in xPL stages. Why is it important? What kind 
of issues does it fix?
- 3ba9b1f7bd7e ("Cyclone V Board handsoff script"). No explanation as 
what this does. Why is it better than the current implementation? What 
does it bring? From which version of "official" downstream fork of 
Altera did you import this script?
- 1feebc36e588 ("FPGA2SDRAM setup fix"). When does it stall? What's the 
symptom? What are the bootlogs? What is stalling "from distro"? "This 
patch fix the issue", but why/how? This sounds like "I tried something 
and it worked, I don't understand why" which we typically do not like.

Were *I* maintainer, I would have accepted none of those patches in that 
form and instead asked you to provide more information. By not providing 
information, you're stripping users the ability to figure out if an 
issue they experience in an older U-Boot has been fixed in newer 
versions without having to compile the new version and testing by 
themselves. They could just look at commit logs (or do a search on the 
Internet since our mailing list is publicly archived) and see if someone 
had the same issue and it was patched. This is *not* a theoretical 
benefit, it happens often in the Linux kernel. With additional 
information, it helps us figuring out if we can refactor or remove code 
a few years from now because we could hope to reproduce the problem you 
fixed and see if it's still fixed after refactoring/deleting code. All 
that to say, your patches also benefited from not exactly following 
rules. An obvious one being (specifically the last sentence):

"""
Keep EVERY line under 72 characters. That is, your message should be 
line-wrapped with line-feeds. However, don’t get carried away and wrap 
it too short either since this also looks funny.
"""

They also do not adhere (according to me; see the list of questions I 
would have had on now-merged commits) to:

"""
Detail level: The audience of the commit log message that you should 
cater to is those familiar with the underlying source code you are 
modifying, but who are _not_ familiar with the patch you are submitting. 
They should be able to determine what is being changed and why.
"""

I've noted multiple times that your patches typically do not follow the 
naming scheme where there could at least be one prefix like "socfpga: ", 
c.f.:

"""
If applicable, prefix the summary line with a word describing what area 
of code is being affected followed by a colon. This is a standard 
adopted by both U-Boot and Linux. [...] The best thing to do is look at 
the “git log <file>” output to see what others have done so you don’t 
break conventions.
"""

Though to be fair, some merged socfpga patches don't and it's not always 
easy to find an appropriate prefix. For 
arch/arm/mach-socfpga/misc_gen5.c, it could have been "arm: socfgpa: ". 
For arch/arm/config.mk, "arch: arm: " or "arch: arm: build:" for 
example. For cmd/Kconfig, "cmd: " at the very least. For the new boards, 
something like "arm: socfpga: " for example. This is important because 
people visually filter what they want to review or not. After looking 
into your patch submissions in the past, I understood that you mostly do 
stuff related to socfpga which I have no interested in, but since the 
commit titles don't match what I'm expecting, I now simply ignore all 
your patches even though I could and likely would have reviewed 
f26db83ca964 ("fix PL330 CMD supported target") and bb1c2b463265 
("update GCC version check after Kbuild bump").

Cheers,
Quentin