Re: [coreboot] What is the best way to treat warnings reported by checkpatch.pl

[Vadim Bendebury]

I am not sure if this happened in coreboot upstream: on some other projects
checkpatch.pl was recently updated to the latest Linux kernel version, and
some more stupid warnings/errors started popping out.

There is a fine controlling checkpatch.pl behavior, .checkpatch.conf in the
coreboot root directory.  That file can be modified to quiet down
checkpatch.pl complaints which are not applicable to coreboot.

--vb

On Mon, Jul 25, 2016 at 11:46 PM, Zeh, Werner 
wrote:
> Hi community.
>
> I recently ran into a blocked commit due to warnings reported by
> checkpatch.pl.
>
> In this particular case I wanted to add the “ATSR” structure to the DMAR
> tables. To do that, I need to define a new structure
>
> which reflects the per specification needed layout of this table entry.
>
> I did that in the same style how other structures are defined in the same
> file (src/arch/x86/include/arch/acpi.h):
>
> typedef struct dmar_atsr_entry {
>         u16 type;
>
>         u16 length;
>
>         u8 flags;
>
>         u8 reserved;
>
>         u16 segment;
>
> } __attribute__ ((packed)) dmar_atsr_entry_t;
>
> This change resulted in two warnings caused by util/lint/checkpatch.pl
> when I was trying to do a git commit:
>
> WARNING: do not add new typedefs
>
> #34: FILE: src/arch/x86/include/arch/acpi.h:288:
>
> +typedef struct dmar_atsr_entry {
>
> WARNING: __packed is preferred over __attribute__((packed))
>
> #40: FILE: src/arch/x86/include/arch/acpi.h:294:
>
> +} __attribute__ ((packed)) dmar_atsr_entry_t;
>
> So the first warning tells me that I should not use “typedef” in my code.
>
> The second one tells me not to use __attribute__ ((packed) but instead
> __packed!
>
> If one take a closer look to the coreboot tree, one will not find a macro
> definition called __packed at all.
>
> After a short discussion on IRC with Stefan it was clear that although we
> have this checkpatch.pl script in the tree and the git hook “pre-commit”
>
> activates it, the script does not perfectly match current coreboot coding
> style.
>
> So in the end there needs to be a change somewhere:
> We can adapt the script to match current coreboot coding style
>
> We can start to refactor the coreboot tree to match the demands of
>
> checkpatch.pl
> We can align only new patches to the demands of the script
>
> We can show warnings from the script but prevent them from
>
> aborting the git commit
>
> While 1. would be the best, straight forward approach for the project, it
> might cause high effort for a person who knows the code base very well.
> will lead to a lot of changes in the tree while the reason/goal is
>
> still questionable (at least to me).
> will lead to different code style all over the tree (even in the same
>
> file) and in the end will result in fragmented code. I personally would not
> like it.
> can be a short term solution while we are seeking for the best way to
>
> deal with this issue.
>
> There may be other ways to go, too, which I have overseen now.
>
> I just want to start this discussion officially as this is not the first
> time I ran into issues with checkpatch.pl.
>
> What do you think about it, what would be the best way to handle this case?
>
> Your ideas and thoughts are appreciated.
>
> Werner