linux-kernel - Re: [PATCH 03/15] dt-bindings: clock: qcom,dispcc-sm6125: Require GCC PLL0 DIV clock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 26 Jun 2023 19:47:37 +0200
From:   Marijn Suijten <marijn.suijten@...ainline.org>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     Konrad Dybcio <konrad.dybcio@...aro.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Rob Clark <robdclark@...il.com>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Sean Paul <sean@...rly.run>, David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Krishna Manikandan <quic_mkrishn@...cinc.com>,
        ~postmarketos/upstreaming@...ts.sr.ht,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Martin Botka <martin.botka@...ainline.org>,
        Jami Kettunen <jami.kettunen@...ainline.org>,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Krzysztof Kozlowski <krzk@...nel.org>,
        linux-clk@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        freedreno@...ts.freedesktop.org, Lux Aliaga <they@...t.lgbt>
Subject: Re: [PATCH 03/15] dt-bindings: clock: qcom,dispcc-sm6125: Require
 GCC PLL0 DIV clock

On 2023-06-26 18:15:13, Krzysztof Kozlowski wrote:
> On 26/06/2023 16:26, Marijn Suijten wrote:
> > On 2023-06-26 11:43:39, Konrad Dybcio wrote:
> >> On 25.06.2023 21:48, Marijn Suijten wrote:
> >>> On 2023-06-24 03:45:02, Konrad Dybcio wrote:
> >>>> On 24.06.2023 02:41, Marijn Suijten wrote:
> >>>>> The "gcc_disp_gpll0_div_clk_src" clock is consumed by the driver, will
> >>>>> be passed from DT, and should be required by the bindings.
> >>>>>
> >>>>> Fixes: 8397c9c0c26b ("dt-bindings: clock: add QCOM SM6125 display clock bindings")
> >>>>> Signed-off-by: Marijn Suijten <marijn.suijten@...ainline.org>
> >>>>> ---
> >>>> Ideally, you'd stick it at the bottom of the list, as the items: order
> >>>> is part of the ABI
> >>>
> >>> This isn't an ABI break, as this driver nor its bindings require/declare
> >>> a fixed order: they declare a relation between clocks and clock-names.
> >> Bindings describe the ABI, drivers implement compliant code flow.
> > 
> > That is how bindings are supposed to be...  However typically the driver
> > is written/ported first and then the bindings are simply created to
> 
> Your development process does not matter for the bindings. Whatever you
> decide to do "typically" is your choice, although of course I understand
> why you do it like that. You can argument the same that "I never create
> bindings in my process, so the driver defines the ABI".

This is not my process, nor my choice.

> > reflect this, and sometimes (as is the case with this patch)
> > incorrectly.
> > 
> > That, together with a lack of DTS and known-working device with it
> > (which is why I'm submitting driver+bindings+dts in one series now!)
> > makes us shoot ourselves in the foot by locking everyone into an ABI
> > that makes no sense.
> 
> No one is locked into the ABI. SoC maintainer decides on this. However
> unjustified ABI breaking or not caring about it at all is not the way to
> go. It is not the correct process.

It definitely sounds like it.

> >>> This orders the GCC clock just like other dispccs.  And the previous
> >>> patch dropped the unused cfg_ahb_clk from the bindings, so all bets are
> >>> off anyway.
> >> Thinking about it again, the binding has not been consumed by any upstream
> >> DT to date, so it should (tm) be fine to let it slide..
> > 
> > Exactly, I hope/doubt anyone was already using these incomplete
> > bindings.  And again: the ABI here is the name->phandle mapping, the
> > order Does Not Matter™.
> 
> No, it's not. Your one driver does not define the ABI. There are many
> different drivers, many different operating systems and other software
> components.

You missed the point entirely.

The point is that someone contributed a dt-bindings patch that does not
represent the hardware (hence not the driver for that hardware either).
It was taken without objection.  So now what?  We are stuck with a
broken ABI that does not allow us to describe the hardware?

If there are many different drivers and other OSes, why are we solely
responsible for describing broken bindings?  Why were there no
objections elsewhere that this does not allow us to describe the
hardware in question?

Note that the person signing off on and sending that initial dt-bindings
patch for qcom,dispcc-sm6125.yaml is me, by the way.

- Marijn