Re: [PATCH v3] checkpatch: add missing bindings license check

[Dmitry Rokosov]

Rob, Krzysztof, Andrew,

Sorry for one more ping. I don't understand the status for this
patchset. If you don't mind, let's discuss solution which will be okay
for all maintainers.

What we have for now:
    - Krzysztof acked v3 patchset
    - Andrew applied it to mm-unstable
    - Rob didn't agree with the current approach.

I would be grateful if you can share your opinion. And we will be on the
same page.

On Wed, Mar 22, 2023 at 05:56:20PM +0300, Dmitry Rokosov wrote:
> On Wed, Mar 22, 2023 at 09:36:40AM -0500, Rob Herring wrote:
> > On Wed, Mar 22, 2023 at 9:15 AM Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxx> wrote:
> > >
> > > On Wed, Mar 22, 2023 at 08:40:21AM -0500, Rob Herring wrote:
> > > > On Wed, Mar 22, 2023 at 5:26 AM Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > Hello Rob, thank you for the comments. Please find my thoughts below.
> > > > >
> > > > > On Tue, Mar 21, 2023 at 04:53:37PM -0500, Rob Herring wrote:
> > > > > > On Mon, Mar 20, 2023 at 11:33:50PM +0300, Dmitry Rokosov wrote:
> > > > > > > All headers from 'include/dt-bindings/' must be verified by checkpatch
> > > > > > > together with Documentation bindings, because all of them are part of
> > > > > > > the whole DT bindings system.
> > > > > > >
> > > > > > > The requirement is dual licensed and matching pattern:
> > > > > > >     /GPL-2\.0(?:-only|-or-later|\+)? (?:OR|or) BSD-2-Clause/
> > > > > >
> > > > > > This is not correct. The headers can and should be licensed like the dts
> > > > > > files which are (unfortunately) all over the place and differ from the
> > > > > > bindings.
> > > > > >
> > > > > > Also, GPL-2.0-or-later is neither desired nor encouraged.
> > > > >
> > > > > Sorry, I'm little bit confused. Let's discuss correct way.
> > > > >
> > > > > We had such discussion in another review.
> > > > >
> > > > > https://lore.kernel.org/all/20230313201259.19998-4-ddrokosov@xxxxxxxxxxxxxx/
> > > > >
> > > > > Krzysztof has mentioned that Documentation yaml bindings schemas and
> > > > > include bindings headers should have the same license by default.
> > > >
> > > > By default is the key. Logically, headers are part of the binding
> > > > definition. However, they are included by dts files, so IMO their
> > > > license should align with dts files. If you don't yet have any dts
> > > > files, then yes, "GPL-2.0-only OR BSD-2-Clause" is what you should
> > > > use.
> > > >
> > > > > And checkpath must check not only Documentation schema (previous
> > > > > implementation), but 'include bindings' as well:
> > > > >
> > > > > From Krzysztof at https://lore.kernel.org/all/9d176288-cd7c-7107-e180-761e372a2b6e@xxxxxxxxxx/:
> > > >
> > > > Checkpatch has no way of knowing about the dts file part, so it can't
> > > > tell you what license.
> > > >
> > > > Even as-is, checkpatch is wrong sometimes. If you convert a binding
> > > > (that defaulted to GPL-2.0-only) to schema, you can't just relicense
> > > > it dual licensed.
> > > >
> > > > >
> > > > > ---
> > > > > >>>>> @@ -0,0 +1,20 @@
> > > > > >>>>> +/* SPDX-License-Identifier: GPL-2.0+ */
> > > > > >>>>
> > > > > >>>> I found in changelog:
> > > > > >>>> "fix license issue, it's GPL-2.0+ only in the current version"
> > > > > >>>> and I do not understand.
> > > > > >>>>
> > > > > >>>> The license is wrong, so what did you fix?
> > > > > >>>>
> > > > > >>>
> > > > > >>> Sorry don't get you. Why is it wrong?
> > > > > >>
> > > > > >> Run checkpatch - it will tell you why wrong. The license is not correct.
> > > > > >> This is part of binding and should be the same as binding.
> > > > > >>
> > > > > >
> > > > > > I always run checkpatch before sending the next patch series. Checkpatch
> > > > > > doesn't highlight this problem:
> > > > > >
> > > > > > --------------
> > > > > > $ rg SPDX a1_clkc_v10/v10-0003-dt-bindings-clock-meson-add-A1-PLL-and-Periphera.patch
> > > > > > 32:+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > > > 111:+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > > > 188:+/* SPDX-License-Identifier: GPL-2.0+ */
> > > > > > 294:+/* SPDX-License-Identifier: GPL-2.0+ */
> > > > > >
> > > > > > $ ./scripts/checkpatch.pl --strict a1_clkc_v10/v10-0003-dt-bindings-clock-meson-add-A1-PLL-and-Periphera.patch
> > > > > > total: 0 errors, 0 warnings, 0 checks, 259 lines checked
> > > > >
> > > > > Hmm, my bad, that's something to fix/improve in checkpatch.
> > > > > ---
> > > > >
> > > > > Actually, I agree with Krzysztof that checkpatch should verify 'include
> > > > > bindings', but looks like there is misunderstanding which license pattern
> > > > > we have to use.
> > > > >
> > > > > Rob, could you please share your thoughts if possible? Which one pattern
> > > > > we have to base on? GPL-2.0-only without 'later' suffix? Or you totally
> > > > > disagree that checkpatch is responsible for 'include bindings'
> > > > > verification?
> > > >
> > > > I think we could do this:
> > > >
> > > > Schemas should be: GPL-2.0-only OR BSD-2-Clause
> > > > Headers should be: GPL-2.0-only OR .*
> > > >
> > > > Perhaps the 2nd term can be constrained to "(MIT|BSD-[23]-Clause)",
> > > > but I haven't looked at what variations exist in the headers. It may
> > > > be too varied that we can only check for "OR". We don't want to
> > > > encourage folks to blindly relicense things because checkpatch says
> > > > so. If you are copying an existing header and modifying it, then you
> > > > keep the original license (unless you have rights to change it).
> > >
> > > Yes, if we are thinking in the such terms, when bindings are part of
> > > device tree source, it's one option to make the same license for both of
> > > them. But usually developer creates bindings definition in the first.
> > 
> > No, most often they are copied from something else. Any tool can't
> > know what the source (and its license) is and actively telling users
> > to do something different is bad.
> > 
> > I imagine writing the schema is the last thing because upstream
> > requires it and downstream doesn't.
> > 
> 
> Maybe checkpatch strict rules would allow developers to double confirm
> licenses in the copied files...
> 
> > > After that, developer or other contributor creates device tree nodes.
> > > Also different device tree sources (for differnt boards as an example)
> > > can have different licenses.
> > 
> > I'm sure there are combinations of dts files and headers with
> > incompatible licenses. A tool to check that would be nice. Just need
> > to generate a list of all input files perhaps with the preprocessor
> > dependency generation and then get the licenses for all the files.
> > 
> 
> Are you talking about some make rule like 'dt_bindings_check' or part of
> 'dtb_check'?
> 
> > > Maybe it's better option to make license dependency between dts and
> > > bindings when bindings have a first priority and dts should have the
> > > same license or dual license, because bindings are the primary from the
> > > git history point of view.
> > 
> > dts files are too far gone to define any rule in checkpatch. Binding
> > files are not because there's really only 2 variations since all the
> > existing bindings are just kernel default license (GPL-2.0-only).
> > 
> > > OR
> > >
> > > Make default value of bindings as suggested in the patchset (maybe
> > > without +/or-later) and show notice log from the checkpatch, like:
> > >
> > > '''
> > > DT binding documents should be licensed (GPL-2.0-only OR BSD-2-Clause)
> > > For special cases ask 'devicetree@xxxxxxxxxxxxxxx' directly
> > 
> > For special cases, ask your lawyer...
> > 
> > > '''
> > >
> > > And handle all exceptions during LKML review, as Krzysztof suggested
> > > before.
> > >
> > > What do you think about above approaches?
> > 
> > I laid out what the options are already.
> > 
> 
> I don't get your position, sorry. By adding GPL-2.0 OR .* pattern rule
> we are just checking GPL licensed of bindings, it's not enough.
> 
> Different licenses in the *new* yaml schemas and *new* bindings are bad
> idea, aren't?
> 
> If we introduce strict rules (read as 'suggestion') to checkpatch, the
> world will be better. Because new bindings will be aligned with schemas
> by license. The dual license, incompatible licenses with dts files are
> already existed, it doens't solve this problem. But as a next step we
> can expand dtb_check make rule and analyse dts license issues in
> preprocessor execution time.
> 
> > >
> > > Krzysztof, please share your opinion as well.
> > >
> > > For sure, current checkpatch behaviour is wrong, it doesn't help to
> > > understand all mentioned interlacements.
> > 
> > checkpatch is suggestions or possible issues in many cases. It's not
> > absolute nor completely accurate to begin with.
> > 
> > Rob
> 
> -- 
> Thank you,
> Dmitry

-- 
Thank you,
Dmitry