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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Stefano,

> On 15 Aug 2022, at 21:32, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> + 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... */
> 

Agree

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

Agree.
Makes things clearer I think.

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

Removing the copyright would probably require an agreement from the original 
implementer.
To prevent troubles and round of questions I would keep the comment and 
copyright for now.

Cheers
Bertrand

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