On 2022-04-18 at 09:04:33 -0700, Reinette Chatre wrote:
> Hi Pengfei,
>
> On 4/16/2022 12:52 AM, Pengfei Xu wrote:
> > On 2022-03-15 at 09:44:25 -0700, Reinette Chatre wrote:
> >> Some selftests depend on information provided by the CPUID instruction.
> >> To support this dependency the selftests implement private wrappers for
> >> CPUID.
> >>
> >> Duplication of the CPUID wrappers should be avoided.
> >>
> >> Both gcc and clang/LLVM provide __cpuid_count() macros but neither
> >> the macro nor its header file are available in all the compiler
> >> versions that need to be supported by the selftests. __cpuid_count()
> >> as provided by gcc is available starting with gcc v4.4, so it is
> >> not available if the latest tests need to be run in all the
> >> environments required to support kernels v4.9 and v4.14 that
> >> have the minimal required gcc v3.2.
> >>
> >> Provide a centrally defined macro for __cpuid_count() to help
> >> eliminate the duplicate CPUID wrappers while continuing to
> >> compile in older environments.
> >>
> >> Suggested-by: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>
> >> Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> >> ---
> >> Note to maintainers:
> >> - Macro is identical to the one provided by gcc, but not liked by
> >> checkpatch.pl with message "Macros with complex values should
> >> be enclosed in parentheses". Similar style is used in kernel,
> >> for example in arch/x86/kernel/fpu/xstate.h.
> >>
> >> tools/testing/selftests/kselftest.h | 15 +++++++++++++++
> >> 1 file changed, 15 insertions(+)
> >>
> >> diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
> >> index f1180987492c..898d7b2fac6c 100644
> >> --- a/tools/testing/selftests/kselftest.h
> >> +++ b/tools/testing/selftests/kselftest.h
> >> @@ -52,6 +52,21 @@
> >> + * have __cpuid_count().
> >> + */
> >> +#ifndef __cpuid_count
> >> +#define __cpuid_count(level, count, a, b, c, d) \
> >> + __asm__ __volatile__ ("cpuid\n\t" \
> >> + : "=a" (a), "=b" (b), "=c" (c), "=d" (d) \
> >> + : "0" (level), "2" (count))
> >> +#endif
> > Linux C check tool "scripts/checkpatch.pl" shows an error:
> > "
> > ERROR: Macros with complex values should be enclosed in parentheses
>
> I encountered this also and that is why this patch contains the "Note to
> maintainers" above. It is not clear to me whether you considered the note
> since your response does not acknowledge it.
>
Sorry, I just made a suggestion to fix this problem mentioned by the script.
I didn't notice and reply for the note.
> > ...
> > +#define __cpuid_count(level, count, a, b, c, d) \
> > + __asm__ __volatile__ ("cpuid\n\t" \
> > + : "=a" (a), "=b" (b), "=c" (c), "=d" (d) \
> > + : "0" (level), "2" (count))
> > "
> > Googling:
> > https://www.google.com/search?q=Macros+with+complex+values+should+be+enclosed+in+parentheses&rlz=1C1GCEB_enUS884US884&oq=Macros+with+complex+values+should+be+enclosed+in+parentheses&aqs=chrome.0.69i59j0i5i30l2.313j0j7&sourceid=chrome&ie=UTF-8
> > -> https://stackoverflow.com/questions/8142280/why-do-we-need-parentheses-around-block-macro
>
> More information available in
> https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html#Statement-Exprs
> but from what I understand it does not apply to this macro. Even so, I do
> not know what checkpatch.pl uses to determine that this is a "Macro with
> complex values".
>
Checked checkpatch.pl and it seems to suggest using ({ }) for any asm macro
definition.
> >
> > Could we fix it as follow, shall we?
> > "
> > #ifndef __cpuid_count
> > #define __cpuid_count(level, count, a, b, c, d) ({ \
> > __asm__ __volatile__ ("cpuid\n\t" \
> > : "=a" (a), "=b" (b), "=c" (c), "=d" (d) \
> > : "0" (level), "2" (count)) \
> > })
> > #endif
> > "
>
> Sure, I can do so.
>
I just made a suggestion to fix the problem reported by the checkpatch.pl.
But I didn't think deeply enough before: I'm not sure is there any real
improvment or help after the fix.
Thanks!
--Pengfei
> Reinette
