Re: [PATCH] x86: fixmap: use CONFIG_NR_CPUS instead of NR_CPUS

From: Randy Dunlap <rdunlap@infradead.org>
To: Ingo Molnar <mingo@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	x86@kernel.org, Mel Gorman <mgorman@techsingularity.net>,
	Tom Rix <trix@redhat.com>
Subject: Re: [PATCH] x86: fixmap: use CONFIG_NR_CPUS instead of NR_CPUS
Date: Wed, 26 May 2021 10:59:51 -0700	[thread overview]
Message-ID: <59676378-1b52-cae7-7944-adeffd27190e@infradead.org> (raw)
In-Reply-To: <YK3vrIB7cWop+UIW@gmail.com>

On 5/25/21 11:50 PM, Ingo Molnar wrote:
> 
> * Randy Dunlap <rdunlap@infradead.org> wrote:
> 
>> Use CONFIG_NR_CPUS instead of NR_CPUS for an enum entry item.
>> (Alternatively, #include <linux/threads.h> unconditionally instead of
>> conditionally.)
>>
>> This fixes 100+ build errors like so:
>>
>> In file included from ../include/asm-generic/early_ioremap.h:6:0,
>>                  from ./arch/x86/include/generated/asm/early_ioremap.h:1,
>>                  from ../arch/x86/include/asm/io.h:44,
>>                  from ../include/linux/io.h:13,
>>                  from ../mm/early_ioremap.c:13:
>> ../arch/x86/include/asm/fixmap.h:103:48: error: ‘NR_CPUS’ undeclared here (not in a function); did you mean ‘NR_OPEN’?
>>   FIX_KMAP_END = FIX_KMAP_BEGIN + (KM_MAX_IDX * NR_CPUS) - 1,
>>
>> Fixes: e972c2511967 ("mm/early_ioremap: add prototype for early_memremap_pgprot_adjust")
> 
> I believe this patch is in the -mm tree, not the x86 tree.
> 
>> @@ -100,7 +100,7 @@ enum fixed_addresses {
>>  #endif
>>  #ifdef CONFIG_KMAP_LOCAL
>>  	FIX_KMAP_BEGIN,	/* reserved pte's for temporary kernel mappings */
>> -	FIX_KMAP_END = FIX_KMAP_BEGIN + (KM_MAX_IDX * NR_CPUS) - 1,
>> +	FIX_KMAP_END = FIX_KMAP_BEGIN + (KM_MAX_IDX * CONFIG_NR_CPUS) - 1,
>>  #ifdef CONFIG_PCI_MMCONFIG
>>  	FIX_PCIE_MCFG,
>>  #endif
> 
> Please resolve this bug properly:
> 
>  - Don't sprinkle low level headers with random CONFIG_NR_CPUS conversions.
> 
>  - <asm/io.h> currently includes <asm/early_ioremap.h>, but this seems 
>    unjustified.

Deleting it causes build errors.

>  - Once early_ioremap.h is gone from io.h, it's potentially possible to 
>    include <linux/threads.h>. More work to resolve dependencies might be 
>    needed though.

Yes, my first patch for this (unsent) just included <linux/threads.h>
in fixmap.h unconditionally instead of conditionally.

> Frankly, I'd prefer if such a low level header dependencies change came in 
> via the x86 tree so we can properly review it, test it, and keep it 
> working. Right now I can only guess what is needed here...

Sure, makes sense.

Mel, do you have any patch suggestions here?  re:

commit e972c2511967181d955f74181d74438e26b2e797
Author: Mel Gorman <mgorman@techsingularity.net>
Date:   Fri May 21 10:40:56 2021 +1000

    mm/early_ioremap: add prototype for early_memremap_pgprot_adjust

thanks.
-- 
~Randy