Hi Stefano, > On 15 Aug 2022, at 21:32, Stefano Stabellini <sstabell...@kernel.org> 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.stabell...@amd.com> >>> --- >>> >>> 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 >>
