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)) ```
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
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
Dear Joe,
Am Sonntag, den 19.03.2017, 01:31 -0700 schrieb Joe Perches:
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
Yes, for x86 (with `CONFIG_X86_32`) they are.
``` $ git grep asmlinkage | grep regparm arch/x86/include/asm/linkage.h:#def ine asmlinkage CPP_ASMLINKAGE __attribute__((regparm(0))) $ nl -ba arch/x86/include/asm/linkage.h | head -11 1 #ifndef _ASM_X86_LINKAGE_H 2 #define _ASM_X86_LINKAGE_H 3 4 #include <linux/stringify.h> 5 6 #undef notrace 7 #define notrace __attribute__((no_instrument_function)) 8 9 #ifdef CONFIG_X86_32 10 #define asmlinkage CPP_ASMLINKAGE __attribute__((regparm(0))) 11 #endif /* CONFIG_X86_32 */ ```
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
-
Joe Perches -
Paul Menzel