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

[Rob Herring]

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).

Rob