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
