Message-ID: <1489912261.13953.22.camel@perches.com>
Subject: Re: checkpatch: Question regarding asmlinkage and storage class
From: Joe Perches <joe@perches.com>
To: Paul Menzel <paulepanter@users.sourceforge.net>,
        Andy Whitcroft <apw@canonical.com>
Cc: linux-kernel@vger.kernel.org, coreboot@coreboot.org
Date: Sun, 19 Mar 2017 01:31:01 -0700
In-Reply-To: <1489839338.9183.68.camel@users.sourceforge.net>
References: <1489839338.9183.68.camel@users.sourceforge.net>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2134
Lines: 69

On Sat, 2017-03-18 at 13:15 +0100, Paul Menzel wrote:
> Dear checkpatch developers,
> 
> 
> The coreboot project started using checkpatch.pl, and now some effort
> is going into fixing issues pointed out by `checkpatch.pl`.
> 
> The file `src/arch/x86/acpi_s3.c` in coreboot contains the code below.
> 
> ```
>    205	void (*acpi_do_wakeup)(uintptr_t vector, u32 backup_source, u32 backup_target,
>    206		u32 backup_size) asmlinkage = (void *)WAKEUP_BASE;
> ```
> 
> The warning is
> 
> > WARNING: storage class should be at the beginning of the declaration
> 
> which raised the question below [2].
> 
> > And I am waiting for someone to answer why checkpatch.pl claims
> > asmlinkage as a storage-class in the first place.
[]
> In coreboot the macro is defined similarly to Linux.
> 
> ```
> #define asmlinkage __attribute__((regparm(0)))
> #define alwaysinline inline __attribute__((always_inline))
> ```

Are they similar?

$ git grep -i "define.*ASMLINKAGE\b" include
include/linux/linkage.h:#define CPP_ASMLINKAGE extern "C"
include/linux/linkage.h:#define CPP_ASMLINKAGE
include/linux/linkage.h:#define asmlinkage CPP_ASMLINKAGE

I believe asmlinkage is defined just to avoid
possible asm/c++ symbol decorations.

> In Linux, commit 9c0ca6f9 (update checkpatch.pl to version 0.10) seems
> to have introduced the check. The commit message contains “asmlinkage
> is also a storage type”.
> 
> Furthermore, `checkpatch.pl` doesn’t seem to warn about the code below.
> 
> ```
> void __attribute__((weak)) mainboard_suspend_resume(void)
> ```
> 
> This raises the question below.
> 
> > It appears coreboot proper mostly followed this placement for
> > function attributes before. It would be nice if we were consistent,
> > specially if checkpatch starts to complaint about these.
> 
> Is there another reason, besides not having that implemented?
> 
> I am looking forward to your answers.
> 
> 
> Kind regards,
> 
> Paul
> 
> 
> [1] https://review.coreboot.org/#/c/18865/1/src/arch/x86/acpi_s3.c@205
> [2] https://review.coreboot.org/18865/
> [3] https://review.coreboot.org/#/c/18865/1/src/arch/x86/acpi_s3.c@244