Message-ID: <4c6b5d55-f6a0-f042-ef10-e282e153a358@linaro.org>
Date:   Tue, 29 Nov 2022 16:46:41 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Hector Martin <marcan@...can.st>,
        Ulf Hansson <ulf.hansson@...aro.org>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Sven Peter <sven@...npeter.dev>,
        Alyssa Rosenzweig <alyssa@...enzweig.io>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Stephen Boyd <sboyd@...nel.org>, Marc Zyngier <maz@...nel.org>,
        Mark Kettenis <mark.kettenis@...all.nl>, asahi@...ts.linux.dev,
        linux-arm-kernel@...ts.infradead.org, linux-pm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH v5 2/4] dt-bindings: cpufreq: apple,soc-cpufreq: Add
 binding for Apple SoC cpufreq

On 29/11/2022 16:17, Hector Martin wrote:
> On 29/11/2022 23.34, Krzysztof Kozlowski wrote:
>> On 29/11/2022 15:00, Hector Martin wrote:
>>> On 29/11/2022 20.36, Ulf Hansson wrote:
>>> Please, let's introspect about this for a moment. Something is deeply
>>> broken if people with 25+ years being an arch maintainer can't get a
>>
>> If arch maintainer sends patches which does not build (make
>> dt_binding_check), then what do you exactly expect? Accept them just
>> because it is 25+ years of experience or a maintainer? So we have
>> difference processes - for beginners code should compile. For
>> experienced people, it does not have to build because otherwise they
>> will get discouraged?
> 
> I expect the process to not be so confusing and frustrating that a
> maintainer with 25+ years of experience gives up. That the bindings
> didn't pass the checker is besides the point. People say the Linux
> kernel community is hostile to newbies. This issue proves it's not just
> newbies, the process is failing even experienced folks.
> 
> On that specific issue, any other functional open source project would
> have the binding checks be a CI bot, with a friendly message telling you
> what to do to fix it, and it would re-run when you push to the PR again,
> which is a *much* lower friction action than sending a whole new patch
> series out for review via email (if you don't agree with this, then
> you're not the average contributor - the Linux kernel is by far the
> scariest major open source project to contribute to, and I think most
> people would agree with me on that).

I agree with this entirely. Not only DT checks, but also for driver code
with sparse/smatch/coccinelle/W=1/clang builds.

> I know Rob has a DT checker bot, but its error output is practically
> line noise, and the error email doesn't even mention the
> DT_SCHEMA_FILES= make option (which is the only way to make the check
> not take *forever* to run). Absolutely nobody is going to look at those
> emails without already knowing the intricacies of DT bindings and the
> checker and not find them incredibly frustrating.

Ack

> 
> But it's not just the DT checker. That came after an argument where the
> MFD maintainer complained about the driver while offering no solution
> nor proposed path forward. I had to have an IRC conversation with him to
> work it out, after which he accepted one of the options I'd already
> proposed over email. If you have to change communication mediums to
> resolve an issue, that means your initial medium failed at its job.

Ack

> 
> Not to mention the random drive-by reviews, nitpicks, disagreements
> between maintainers about how to do things, or just plain cases of
> maintainers stubbornly being *wrong* and refusing to listen while
> everyone around them is telling them they're wrong (until someone above
> them in the maintainer tree tells them they're wrong - then they finally
> get it. If it takes someone in a position of authority telling you
> you're wrong for you to accept it, you're doing a poor job at your own
> position.)

Ack

> 
> And then there's the coding style. The rest of the world has
> standardized on formatting tools. Here, every subsystem maintainer has
> their own pet style you have to learn. "Please put your variables in
> reverse christmas tree order".

Ack here as well, and by above Acks I meant - I agree, you are right.  I
don't think we should discuss people's differences, I mean, different
people can give you different review, sometimes less sometimes more
thorough. However at least policies/processes and coding style should be
heavily unified.

> I once got a review comment that
> complained that I re-aligned an existing #define when I added another
> one (to keep them visually aligned, as the new one increased the
> required tab stop). Because "cleanup patches should be separate" (I
> didn't submit a cleanup patch after that, I just left the defines
> misaligned). So now I need to maintain a massive mental map of exactly
> what style conventions and patch breakup conventions every subsystem
> maintainer wants me to use.

Yep. I just have a doc, instead of mental map. :)

> 
> I'm so glad `make rustfmt` is a thing now. Maybe 50 years down the line,
> most of the kernel will have been rewritten in Rust and we'll finally
> fix just one of these problems /s

+1

> 
> Some of these things are, to some extent, a natural result of working
> with other humans. But the kernel community has both a number of humans
> harder to work with than is reasonable in their position, and an overall
> process that multiplies the resulting pain by an order of magnitude for
> everyone involved.
> 
>> While I understand your point about bikeschedding, but I think your
>> previous bindings got pretty nice and fast reviews, so using examples of
>> non-building case is poor choice.
> 
> Yeah, after a while, because I learned how to do DT bindings the hard
> way after having to submit a bunch and getting everything wrong (and
> even then I still get the simplest things wrong, see: v4 here). Which is
> why I'll pick up after rmk's attempt and try again with macsmc at some
> point (probably after 6.1). But I'm not holding my breath that I won't
> need another half dozen rounds of bikeshedding.
> 
> When it takes 10 times more man-hours to upstream a driver than to
> reverse engineer and write it with zero documentation, never mind the
> calendar time it takes, something is very wrong. 

Yes, that's a point. Yet we (community) still have another goal here
than the pure goal of submitter. The submitter wants its stuff upstream.
Therefore this upstreaming effort is for submitter a bit wasted (as I
agree that reverse-engineering should be the biggest task).

However the community (expressed maybe mostly as maintainers) wants
something which will be maintainable and usable also for others, not
only for patch submitters.

Indeed maybe 10x difference between writing code and upstreaming is
something to improve, but please do not forget that the concept is here
to be able to manage this big codebase.

> I actively dread
> submitting new drivers to new subsystems or some existing ones now. How
> much pain will the next one be? Will I be asked to move files around 3
> times? Spend 4 rounds bikeshedding the DT schema? Think it's finally
> ready only for someone to show up and ask to change a major part of the
> design at the last minute?
> 
> And this all when we're actually submitting code of decent quality (I
> think I have enough experience not to submit crap most of the time). Now
> imagine how much worse this all is for a newbie who submits a
> well-intentioned but definitely not up to standard patch. There's a huge
> chance they'll give up before ever getting the submission through.

You say this like any newbie should be able to send a patch and get it
accepted right away, regardless of actually what is in that patch (not
up to standard). It's not newbie's right. No one said it's easy and fast
process... If you want easy and fast, do JavaScript... If you want to
make it easier and faster in the kernel, then we need more reviewers and
maintainers. Somehow many, many people want to send patches, but not
that many want to review them.

> 
> Again, I'll keep pushing and sending out patches, because this is
> important. But this is the reality. This is how bad it is. The process
> is broken, and everyone outside the kernel community can see that and
> has been saying that for ages. Just because some of us are willing to
> put up with the pain anyway doesn't mean it's working as intended.
Best regards,
Krzysztof

Powered by blists - more mailing lists