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