<div dir="ltr">I am not sure if this happened in coreboot upstream: on some other projects <a href="http://checkpatch.pl">checkpatch.pl</a> was recently updated to the latest Linux kernel version, and some more stupid warnings/errors started popping out.<div><br></div><div>There is a fine controlling <a href="http://checkpatch.pl">checkpatch.pl</a> behavior, <span style="color:rgb(0,0,0)">.checkpatch.conf in the coreboot root directory. That file can be modified to quiet down <a href="http://checkpatch.pl">checkpatch.pl</a> complaints which are not applicable to coreboot. </span></div><div><span style="color:rgb(0,0,0)"><br></span></div><div><span style="color:rgb(0,0,0)">--vb</span></div><div><span style="color:rgb(0,0,0)"><br></span></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jul 25, 2016 at 11:46 PM, Zeh, Werner <span dir="ltr"><<a href="mailto:werner.zeh@siemens.com" target="_blank">werner.zeh@siemens.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="DE" link="blue" vlink="purple">
<div class="m_2445479446219378522WordSection1">
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">Hi
</span><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">community</span><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Arial","sans-serif""><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">I recently ran into a blocked commit due to warnings reported by <a href="http://checkpatch.pl" target="_blank">checkpatch.pl</a>.<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">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<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">which reflects the per specification needed layout of this table entry.
<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">I did that in the same style how other structures are defined in the same file (src/arch/x86/include/arch/acpi.h):<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif""><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">typedef struct dmar_atsr_entry {<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">
</span><span lang="ES" style="font-size:10.0pt;font-family:"Arial","sans-serif"">u16 type;<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="ES" style="font-size:10.0pt;font-family:"Arial","sans-serif""> u16 length;<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="ES" style="font-size:10.0pt;font-family:"Arial","sans-serif""> u8 flags;<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="ES" style="font-size:10.0pt;font-family:"Arial","sans-serif"">
</span><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">u8 reserved;<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif""> u16 segment;<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">} __attribute__ ((packed)) dmar_atsr_entry_t;<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif""><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">This change resulted in two warnings caused by util/lint/<a href="http://checkpatch.pl" target="_blank">checkpatch.pl</a> when I was trying to do a git commit:<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif""><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">WARNING: do not add new typedefs<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">#34: FILE: src/arch/x86/include/arch/acpi.h:288:<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">+typedef struct dmar_atsr_entry {<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif""><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">WARNING: __packed is preferred over __attribute__((packed))<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">#40: FILE: src/arch/x86/include/arch/acpi.h:294:<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">+} __attribute__ ((packed)) dmar_atsr_entry_t;<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif""><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">So the first warning tells me that I should not use “typedef” in my code.<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif""><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">The second one tells me not to use __attribute__ ((packed) but instead __packed!<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">If one take a closer look to the coreboot tree, one will not find a macro definition called __packed at all.<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif""><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">After a short discussion on IRC with Stefan it was clear that although we have this <a href="http://checkpatch.pl" target="_blank">checkpatch.pl</a> script in the tree and the git hook “pre-commit”<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">activates it, the script does not perfectly match current coreboot coding style.<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif""><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">So in the end there needs to be a change somewhere:<u></u><u></u></span></p>
<p class="m_2445479446219378522MsoListParagraph"><u></u><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif""><span>1.<span style="font:7.0pt "Times New Roman"">
</span></span></span><u></u><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">We can adapt the script to match current coreboot coding style<u></u><u></u></span></p>
<p class="m_2445479446219378522MsoListParagraph"><u></u><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif""><span>2.<span style="font:7.0pt "Times New Roman"">
</span></span></span><u></u><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">We can start to refactor the coreboot tree to match the demands of <a href="http://checkpatch.pl" target="_blank">checkpatch.pl</a><u></u><u></u></span></p>
<p class="m_2445479446219378522MsoListParagraph"><u></u><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif""><span>3.<span style="font:7.0pt "Times New Roman"">
</span></span></span><u></u><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">We can align only new patches to the demands of the script<u></u><u></u></span></p>
<p class="m_2445479446219378522MsoListParagraph"><u></u><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif""><span>4.<span style="font:7.0pt "Times New Roman"">
</span></span></span><u></u><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">We can show warnings from the script but prevent them from aborting the git commit<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif""><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">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.<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">2. will lead to a lot of changes in the tree while the reason/goal is still questionable (at least to me).<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">3. 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.<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">4. can be a short term solution while we are seeking for the best way to deal with this issue.<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif""><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">There may be other ways to go, too, which I have overseen now.<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif""><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">I just want to start this discussion officially as this is not the first time I ran into issues with <a href="http://checkpatch.pl" target="_blank">checkpatch.pl</a>.<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">What do you think about it, what would be the best way to handle this case?<u></u><u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif""><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">Your ideas and thoughts are appreciated.<span class="HOEnZb"><font color="#888888"><u></u><u></u></font></span></span></p><span class="HOEnZb"><font color="#888888">
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif""><u></u> <u></u></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial","sans-serif"">Werner<u></u><u></u></span></p>
</font></span></div>
</div>
<br>--<br>
coreboot mailing list: <a href="mailto:coreboot@coreboot.org">coreboot@coreboot.org</a><br>
<a href="https://www.coreboot.org/mailman/listinfo/coreboot" rel="noreferrer" target="_blank">https://www.coreboot.org/mailman/listinfo/coreboot</a><br></blockquote></div><br></div>