Re: [PATCH] add SPDX to arch/arm/*.c

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

+ Xen maintainers and committers


For context, I wrote a patch to introduce SPDX tags starting from
arch/arm/*.c.

Julien rightfully pointed out that it should be added to our coding
style. He is right. Also as I was reading his replies, I realized there
are a couple of minor coding style things to agree as a group first.
I'll highlighted them here and suggested a proposal. I am happy to go
with the preference of the majority.


## comment format // vs /*

In this patch I used:
// SPDX-License-Identifier: GPL-2.0

But our comment format is actually /* xxx */. I think it is fair to
use /* xxx */ as Julien requested:

/* SPDX-License-Identifier: GPL-2.0 */

Unless there are any concerns, I'll change the patch to /* SPDX... */


## blank line after SPDX

In this series, I didn't add a blank line after the new SPDX comment, no
matter if the following line was an #include or another comment. Now I am
thinking it would be best to add a blank line, as follows:

---
/* SPDX-License-Identifier: GPL-2.0 */

#include <xen/bitops.h>
---

Or:

---
/* SPDX-License-Identifier: GPL-2.0 */

/*
 * xen/arch/arm/device.c
 *
---

Let me know if that's OK for you.


## Original copyright text

As we add the new SDPX tag, It makes sense to remove the older copyright
text at the top of the file, e.g.:

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index f03cd943c6..d0a409e4fd 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -1,20 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
 /*
  * alternative runtime patching
  * inspired by the x86 version
  *
  * Copyright (C) 2014-2016 ARM Ltd.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #include <xen/init.h>


Now the question is whether we want to keep what's left:

/*
 * alternative runtime patching
 * inspired by the x86 version
 *
 * Copyright (C) 2014-2016 ARM Ltd.
 */

The Copyright line is not useful and often stale. Also the other comment
is not very interesting in most cases (I am referring to "alternative
runtime patching inspired by the x86 version"), although I realize this
is going to be a on case-by-case basis.

My suggestion is to get rid of it all unless useful (in most cases it is
not useful), leading to:


diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index f03cd943c6..e363176d1f 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -1,21 +1,4 @@
-/*
- * alternative runtime patching
- * inspired by the x86 version
- *
- * Copyright (C) 2014-2016 ARM Ltd.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- */
+/* SPDX-License-Identifier: GPL-2.0 */
 
 #include <xen/init.h>
 #include <xen/types.h>


Do you guys agree?


Cheers,

Stefano


P.S.
Julien, I'll reply to your other points separately to avoid confusion.


On Sat, 13 Aug 2022, Julien Grall wrote:
> Hi Stefano,
> 
> On 13/08/2022 01:59, Stefano Stabellini wrote:
> > Add SPDX license information to all the *.c files under arch/arm.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> > ---
> > 
> > We need to start from somewhere and I thought arch/arm/*.c would be a
> > good place to start.
> 
> Thanks for doing it. This will make easier to understand the license in each
> file. There are a couple of places below where the SDPX tag is incorrect. How
> did you figure out the which license to use?
> 
> Also, I think we should consider to add a section about SPDX in our coding
> style so new files are using it. So we don't end up with a mix in arch/arm/*.
> 
> > 
> > diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> > index f03cd943c6..8115f89408 100644
> > --- a/xen/arch/arm/alternative.c
> > +++ b/xen/arch/arm/alternative.c
> > @@ -1,3 +1,4 @@
> > +// SPDX-License-Identifier: GPL-2.0
> 
> Technically, this is a comment. So this should be /* ... */ to follow Xen
> coding style. Also...
> 
> >   /*
> >    * alternative runtime patching
> >    * inspired by the x86 version
> 
> ... this comment contains information about the license. As you add the SPDX,
> the "long" version should be removed. This would also make easier to verify
> the SPDX you add match existing license.
> 
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index ec81a45de9..7c986ecb18 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -1,3 +1,4 @@
> > +// SPDX-License-Identifier: GPL-2.0
> >   /*
> >    * Early Device Tree
> >    *
> > diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> > index ae649d16ef..887b5426c7 100644
> > --- a/xen/arch/arm/cpuerrata.c
> > +++ b/xen/arch/arm/cpuerrata.c
> > @@ -1,3 +1,4 @@
> > +// SPDX-License-Identifier: GPL-2.0
> 
> This file had no explicit license. I had a look at the 'git log' and AFAICT
> this was either new code and came from Linux. So this looks fine to add GPLv2
> here.
> 
> >   #include <xen/cpu.h>
> >   #include <xen/cpumask.h>
> >   #include <xen/init.h>
> > diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> > index 62d5e1770a..a6253cb57f 100644
> > --- a/xen/arch/arm/cpufeature.c
> > +++ b/xen/arch/arm/cpufeature.c
> > @@ -1,3 +1,4 @@
> > +// SPDX-License-Identifier: GPL-2.0
> >   /*
> >    * Contains CPU feature definitions
> >    *
> > diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
> > index f5f6562600..f586c3d781 100644
> > --- a/xen/arch/arm/decode.c
> > +++ b/xen/arch/arm/decode.c
> > @@ -1,3 +1,4 @@
> > +// SPDX-License-Identifier: GPL-2.0
> 
> This tag doesn't match the license below. It is currently GPLv2+. I don't
> think you can change it without consulting the author. But if it is, then it
> should be mentioned in the commit message.
> 
> I remember we discussed in the past that some files were GPLv2+. But I can't
> remember what was the outcome (I can't find the thread). IIRC GPLv2+ is a lot
> more restrictive than GPLv2 and could prevent some companies to contribute.
> 
> [...]
> 
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 2cd481979c..1a2dac95a9 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -1,3 +1,4 @@
> > +// SPDX-License-Identifier: GPL-2.0
> 
> Same here about GPLv2+. Please go through the rest of the files to confirm the
> license.
> 
> Cheers,
> 
> -- 
> Julien Grall
>