From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 05F53C6FD1C for ; Thu, 23 Mar 2023 08:51:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230304AbjCWIvg (ORCPT ); Thu, 23 Mar 2023 04:51:36 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50362 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229563AbjCWIvd (ORCPT ); Thu, 23 Mar 2023 04:51:33 -0400 Received: from mx.sberdevices.ru (mx.sberdevices.ru [45.89.227.171]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4BDD44489 for ; Thu, 23 Mar 2023 01:51:31 -0700 (PDT) Received: from s-lin-edge02.sberdevices.ru (localhost [127.0.0.1]) by mx.sberdevices.ru (Postfix) with ESMTP id 6DBD85FD0B; Thu, 23 Mar 2023 11:51:28 +0300 (MSK) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sberdevices.ru; s=mail; t=1679561488; bh=YDvbblAgwTRBmzIxLGewYzkOUOnDQBgkWvoHPBkQDtA=; h=Date:From:To:Subject:Message-ID:MIME-Version:Content-Type; b=ebJFJlmdXCYScBMmCSo6WTrLtZBjRMoCO+u0CMOoM802u3SFIULROeR+RFnWn1hf1 rtOiGFyZZjk7p2l8k7FNDc63F6JRfGFLbCqjngcoUxFw+83ollCASR4sFoTNhyAFs1 Mek81Rddp5ezIdfI20OI27TVRY7Lm1ZlXmauPKc6UzTXwA10xL/cB+XGFaoULDq5nC 03gkEam+HGVQHUa7HZsfmQQ/M5904DJluiwUiHwERd9UiMFiKV0+i9Nzqit4plDxeu wFIYzazDtsSQpWxP7+napp6UmrOFVyXIgfd1X3j/3H9rbhOaZiwqB5UhNoFgxUKUqb rIKsWWU8mMEwg== Received: from S-MS-EXCH01.sberdevices.ru (S-MS-EXCH01.sberdevices.ru [172.16.1.4]) by mx.sberdevices.ru (Postfix) with ESMTP; Thu, 23 Mar 2023 11:51:27 +0300 (MSK) Date: Thu, 23 Mar 2023 11:51:25 +0300 From: Dmitry Rokosov To: Rob Herring , , CC: , , , , , , Subject: Re: [PATCH v3] checkpatch: add missing bindings license check Message-ID: <20230323085125.e2evhw262fru4i4z@CAB-WSD-L081021> References: <20230320203350.13696-1-ddrokosov@sberdevices.ru> <20230321215337.GA1656483-robh@kernel.org> <20230322102607.5rac7lmy5w653jen@CAB-WSD-L081021> <20230322141444.abwirejgfbeer7lr@CAB-WSD-L081021> <20230322145620.ksia76spsi7f6wxc@CAB-WSD-L081021> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20230322145620.ksia76spsi7f6wxc@CAB-WSD-L081021> User-Agent: NeoMutt/20220415 X-Originating-IP: [172.16.1.6] X-ClientProxiedBy: S-MS-EXCH02.sberdevices.ru (172.16.1.5) To S-MS-EXCH01.sberdevices.ru (172.16.1.4) X-KSMG-Rule-ID: 4 X-KSMG-Message-Action: clean X-KSMG-AntiSpam-Status: not scanned, disabled by settings X-KSMG-AntiSpam-Interceptor-Info: not scanned X-KSMG-AntiPhishing: not scanned, disabled by settings X-KSMG-AntiVirus: Kaspersky Secure Mail Gateway, version 1.1.2.30, bases: 2023/03/23 05:59:00 #20997558 X-KSMG-AntiVirus-Status: Clean, skipped Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 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