Messages in this thread | | | Date | Thu, 23 Mar 2023 11:51:25 +0300 | From | Dmitry Rokosov <> | Subject | Re: [PATCH v3] checkpatch: add missing bindings license check |
| |
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
-- Thank you, Dmitry
| |