[v3] checkpatch: add missing bindings license check

  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/

The issue was found during patch review:
https://lore.kernel.org/all/20230313201259.19998-4-ddrokosov@sberdevices.ru/

Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
---
Changes v3 since v2 at [2]:
    - replace 'not =~' expression with '!~' to be aligned with other
      checkpatch lines

Changes v2 since v1 at [1]:
    - include/dt-bindings check is aligned to open parens
    - introduce more strict pattern for bindings license:
      /GPL-2\.0(?:-only|-or-later|\+)? (?:OR|or) BSD-2-Clause/

Links:
    [1] https://lore.kernel.org/all/20230317201621.15518-1-ddrokosov@sberdevices.ru/
    [2] https://lore.kernel.org/all/20230320100027.27788-1-ddrokosov@sberdevices.ru/
---
 scripts/checkpatch.pl | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
  

On Mon, 2023-03-20 at 23:33 +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/
> 
> The issue was found during patch review:
> https://lore.kernel.org/all/20230313201259.19998-4-ddrokosov@sberdevices.ru/
> 
> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> ---

Assuming the devicetree people are good with this change:

Acked-by: Joe Perches <joe@perches.com>

> Changes v3 since v2 at [2]:
>     - replace 'not =~' expression with '!~' to be aligned with other
>       checkpatch lines
> 
> Changes v2 since v1 at [1]:
>     - include/dt-bindings check is aligned to open parens
>     - introduce more strict pattern for bindings license:
>       /GPL-2\.0(?:-only|-or-later|\+)? (?:OR|or) BSD-2-Clause/
> 
> Links:
>     [1] https://lore.kernel.org/all/20230317201621.15518-1-ddrokosov@sberdevices.ru/
>     [2] https://lore.kernel.org/all/20230320100027.27788-1-ddrokosov@sberdevices.ru/
> ---
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl

> @@ -3709,8 +3709,9 @@ sub process {
>  						WARN("SPDX_LICENSE_TAG",
>  						     "'$spdx_license' is not supported in LICENSES/...\n" . $herecurr);
>  					}
> -					if ($realfile =~ m@^Documentation/devicetree/bindings/@ &&
> -					    not $spdx_license =~ /GPL-2\.0.*BSD-2-Clause/) {
> +					if (($realfile =~ m@^Documentation/devicetree/bindings/@ ||
> +					     $realfile =~ m@^include/dt-bindings/@) &&
> +					    $spdx_license !~ /GPL-2\.0(?:-only|-or-later|\+)? (?:OR|or) BSD-2-Clause/) {
>  						my $msg_level = \&WARN;
>  						$msg_level = \&CHK if ($file);
>  						if (&{$msg_level}("SPDX_LICENSE_TAG",
  

On 20/03/2023 21:33, 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/
> 
> The issue was found during patch review:
> https://lore.kernel.org/all/20230313201259.19998-4-ddrokosov@sberdevices.ru/
> 
> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> ---
> Changes v3 since v2 at [2]:

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
  

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. 

Rob
  

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@sberdevices.ru/

Krzysztof has mentioned that Documentation yaml bindings schemas and
include bindings headers should have the same license by default.
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@linaro.org/:

---
>>>>> @@ -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?
  

On Wed, Mar 22, 2023 at 5:26 AM Dmitry Rokosov <ddrokosov@sberdevices.ru> 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@sberdevices.ru/
>
> 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@linaro.org/:

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
  

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@sberdevices.ru> 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@sberdevices.ru/
> >
> > 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@linaro.org/:
> 
> 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.
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.

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.

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@vger.kernel.org' directly
'''

And handle all exceptions during LKML review, as Krzysztof suggested
before.

What do you think about above approaches?

Krzysztof, please share your opinion as well.

For sure, current checkpatch behaviour is wrong, it doesn't help to
understand all mentioned interlacements.
  

On Wed, Mar 22, 2023 at 9:15 AM Dmitry Rokosov <ddrokosov@sberdevices.ru> 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@sberdevices.ru> 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@sberdevices.ru/
> > >
> > > 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@linaro.org/:
> >
> > 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.

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

> 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@vger.kernel.org' 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.

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

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@sberdevices.ru> 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@sberdevices.ru> 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@sberdevices.ru/
> > > >
> > > > 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@linaro.org/:
> > >
> > > 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@vger.kernel.org' 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
  

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@sberdevices.ru> 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@sberdevices.ru> 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@sberdevices.ru/
> > > > >
> > > > > 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@linaro.org/:
> > > >
> > > > 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@vger.kernel.org' 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