Hi Quentin, You replied with a long passage. No worries, I finished reading it.
On Tue, May 19, 2026 at 2:29 AM Quentin Schulz <[email protected]> wrote: > > 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: First I had no intention to step into someone's shoes. My intention or goal here is pointing out a double standard on a single patch by the same group of reviewers. Again no intention on pointing to specific people or persons. > > 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. Agree, so in order to follow or change at least as possible why not quote the docs or requirements or even give out examples in the first place? And rather rephrasing yourself in various standards along the same patch version? I guess this is not a schizophrenia behavior right? Also I would consider such behavior as teasing behavior. > > 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? Agree that human form indeed. If the same person works on the same review mindset, with my quoted patches how could it even possible that there will be different standards when reviewing or passing patches that alarmingly mentioned on one patch that the format wordings should be xxxx? I cannot tell this is done by mistake nor not worthy in the first place of standard who mentioned specifically. > > 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. For this it is not unfair to me and it is perfectly fair. Meantime I have no anger in this first place. I explained on the reply and had done what you also mentioned at the end of this reply. By xxx reasons I am going to ignore your mail etc. Why bother? > > 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. Agree! So this works on both not only one side but both sides right? Why not cite the docs or provide examples or even explain better from first place on review in order contributors know what is the standard. Reviewers expect someone to correct something and with expected standards so laid down necessary info for it. > > 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 Good why not do so, if this is really doing U-Boot benefit? > 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): I am not sure I follow you here but I will try. I am not sure how other reviewers work their code. At least I work on real hardware no virtual or even without real hardware testing. At the end of the day U-Boot is to serve not display so real hardware functioning is the goal. At least this could trigger maintainers to investigate or confidently reply this is not the case. > > """ > 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"). Ok the last part of the reply said why you ignore my patches and propose the title naming etc. Again do those have a docs or page that people could reference to? Or can a reviewer simply mark it as change requests and reply one sentence i.e. missing proper patch group title reference [1] to correct etc? At least this reply is really constructive on pointing out what the issue rather than teasing on the header wordings. Thanks for the suggestions. Bests, Brian > > Cheers, > Quentin
