Hi Pengfei,
On 4/18/2022 9:31 PM, Pengfei Xu wrote:
> 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.
In this case I would prefer to not implicitly follow the checkpatch.pl without
understanding what the concern is.
The goal of this change is to make the __cpuid_count() macro available
within kselftest and it does so by duplicating gcc's __cpuid_count() macro.
The macro style is not unique and you would, for example, encounter the same
checkpatch.pl complaint if you run:
./scripts/checkpatch.pl -f arch/x86/kernel/fpu/xstate.h
Reinette
