Re: [PATCH v8 2/3] x86/cpu: Check if feature string is non-zero

[Maciej Wieczor-Retman]

On 2026-03-09 at 23:23:26 -0700, Sohil Mehta wrote:
>On 3/2/2026 7:25 AM, Maciej Wieczor-Retman wrote:
>
>> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
>> index 9aa11224a038..b60269174d95 100644
>> --- a/arch/x86/kernel/cpu/common.c
>> +++ b/arch/x86/kernel/cpu/common.c
>> @@ -675,6 +675,7 @@ cpuid_dependent_features[] = {
>>  static void filter_cpuid_features(struct cpuinfo_x86 *c, bool warn)
>>  {
>>  	const struct cpuid_dependent_feature *df;
>> +	char feature_buf[16];
>>  
>
>The usage of number 16 isn't intuitive here. Might be useful to use a
>macro here. See below..
>
...
>
>It would be useful to have a comment here because the function is called
>from multiple files. You can probably reuse the one from the deleted
>x86_feature_name().

Sure, that's a good idea.

>
>/*
> * Return the feature "name" if available, otherwise return
> * the X86_FEATURE_* numerals to make it easier to identify
> * the feature.
> */
>
>> +const char *x86_cap_name(unsigned int bit, char *buf)
>> +{
>> +	unsigned int word = bit >> 5;
>> +	const char *name = NULL;
>> +
>> +	if (likely(word < NCAPINTS))
>> +		name = x86_cap_flags[bit];
>> +	else if (likely(word < NCAPINTS + NBUGINTS))
>> +		name = x86_bug_flags[bit - 32 * NCAPINTS];
>> +
>> +	if (name)
>> +		return name;
>> +
>> +	snprintf(buf, 16, "%u:%u", word, bit & 31);
>
>The buffer size of 16 is expected to be in sync with the callers and
>easy to mess up. Also, the callers wouldn't know that it needs to be 16
>because of this snprintf(). Should we have a simple define for it?
>
>Maybe something like,
>
>#define X86_CAP_BUF_SIZE 16

Yes, thanks, I probably should've done that frome the start :)

>
>> +	return buf;
>> +}
>> +
>>  /*
>>   * This does the hard work of actually picking apart the CPU stuff...
>>   */
>> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
>> index 146f6f8b0650..dfd79b06ab7b 100644
>> --- a/arch/x86/kernel/cpu/cpuid-deps.c
>> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
>> @@ -2,6 +2,7 @@
>>  #include <linux/kernel.h>
>>  #include <linux/init.h>
>>  #include <linux/module.h>
>
>Nit:
>
>You can consider adding a newline here.

Okay.

>> +#include <asm/cpu.h>
>>  #include <asm/cpufeature.h>
>>  

-- 
Kind regards
Maciej Wieczór-Retman