sparc64: Add preprocessor symbols for PAGE_* pgprot_t values.
PATCH 1/1 sparc64: Add preprocessor symbols for PAGE_* pgprot_t values.
Kernel code assumes that the PAGE_* values are preprocessor symbols, and that therefore arch support can be checked for with #ifdef.
At the moment, sparc64 does not implement any of the symbols checked for, so these checks happen to work.
To prevent potential breakage when another #ifdef check is added or when sparc64 implements another PAGE_ value, make such #ifdef checks work by adding preprocessor symbols on top of the PAGE_* variables.
--- arch/sparc/include/asm/pgtable_64.h | 4 ++++ 1 file changed, 4 insertions(+)
arch/sparc/include/asm/pgtable_64.h +4 -0
@@ -216,9 +216,13 @@ pte_t mk_pte_io(unsigned long, pgprot_t, int, unsigned long);unsigned long pte_sz_bits (unsigned long size);extern pgprot_t PAGE_KERNEL ; +#define PAGE_KERNEL PAGE_KERNELextern pgprot_t PAGE_KERNEL_LOCKED ; +#define PAGE_KERNEL_LOCKED PAGE_KERNEL_LOCKEDextern pgprot_t PAGE_COPY ; +#define PAGE_COPY PAGE_COPYextern pgprot_t PAGE_SHARED ; +#define PAGE_SHARED PAGE_SHARED/* XXX This uglyness is for the atyfb driver's sparc mmap() support. XXX */ extern unsigned long _PAGE_IE;
David Miller
maintainerFrom: Clemens Ladisch <[email protected]>
Date: Sun, 02 Nov 2014 21:15:12 +0100
Quoted 11 lines
Kernel code assumes that the PAGE_* values are preprocessor symbols, and that therefore arch support can be checked for with #ifdef.
At the moment, sparc64 does not implement any of the symbols checked for, so these checks happen to work.
To prevent potential breakage when another #ifdef check is added or when sparc64 implements another
PAGE_value, make such #ifdef checks work by adding preprocessor symbols on top of the PAGE_* variables.Signed-off-by Clemens Ladisch <[email protected]>
"This change actually doesn't have any effect, and doesn't matter,
so let's make this change."
I really don't buy this.
I'd also rather the kernel use Kconfig based symbols to detect for arch availability of feature X or Y, assuming things are CPP symbols is very fragile at best.
I'm not applying this, sorry. I mean, you didn't even bother to mention what symbol this does matter for.
Quoted 10 lines · David Miller
Quoted 10 lines
Kernel code assumes that the PAGE_* values are preprocessor symbols, and that therefore arch support can be checked for with #ifdef.
At the moment, sparc64 does not implement any of the symbols checked for, so these checks happen to work.
To prevent potential breakage when another #ifdef check is added or when sparc64 implements another
PAGE_value, make such #ifdef checks work by adding preprocessor symbols on top of the PAGE_* variables.
Quoted 6 lines
Quoted 1 line
Signed-off-by Clemens Ladisch <[email protected]>"This change actually doesn't have any effect, and doesn't matter,
so let's make this change."I really don't buy this.
At the moment, sparc64 does not support symbols such as PAGE_KERNEL_RO, and has no mechanism for arch-independent code to detect this. Some code tries anyway:
$ git grep '#ifn\?def PAGE_KERNEL_' drivers/base/firmware_class.c:#ifndef PAGE_KERNEL_RO drivers/staging/comedi/comedi_buf.c:#ifdef PAGE_KERNEL_NOCACHE mm/nommu.c:#ifndef PAGE_KERNEL_EXEC mm/vmalloc.c:#ifndef PAGE_KERNEL_EXEC
I don't want to add more such code to the kernel without a guarantee that it actually works, or without replacing it with some other kind of check that does work.
Quoted 3 lines
I'd also rather the kernel use
Kconfigbased symbols to detect for arch availability of feature X or Y, assuming things are CPP symbols is very fragile at best.
It is certainly possible to use Kconfig-based symbols. However, this would have its own fragility: adding, e.g., PAGE_KERNEL_SO to an arch requires that one remembers to update Kconfig, and if one forgets, a check like this:
#ifndef CONFIG_ARCH_HAS_PAGE_KERNEL_RO
#define PAGE_KERNEL_RO PAGE_KERNEL /* fallback for this code */ #endif
will not detect the error on sparc64 (if PAGE_KERNEL_RO were a CPP symbol, the compiler would complain about the redefinition).
So even if PAGE_KERNEL_RO is a variable, making it into a CPP symbol is beneficial. And once it is a CPP symbol, introducing a separate Kconfig-based symbol is not necessary and just increases the chances of an error.
Regards,
Clemens
David Miller
maintainerFrom: Clemens Ladisch <[email protected]>
Date: Mon, 03 Nov 2014 10:44:40 +0100
Quoted 14 lines
Quoted 3 lines · David Miller
I'd also rather the kernel use
Kconfigbased symbols to detect for arch availability of feature X or Y, assuming things are CPP symbols is very fragile at best.It is certainly possible to use Kconfig-based symbols. However, this would have its own fragility: adding, e.g.,
PAGE_KERNEL_SOto an arch requires that one remembers to updateKconfig, and if one forgets, a check like this:#ifndef
CONFIG_ARCH_HAS_PAGE_KERNEL_RO
#definePAGE_KERNEL_ROPAGE_KERNEL/* fallback for this code */ #endif
Quoted 7 lines
will not detect the error on sparc64 (if
PAGE_KERNEL_ROwere a CPP symbol, the compiler would complain about the redefinition).So even if
PAGE_KERNEL_ROis a variable, making it into a CPP symbol is beneficial. And once it is a CPP symbol, introducing a separate Kconfig-based symbol is not necessary and just increases the chances of an error.
I'd rather code then use the symbol unconditionally, and we declare that it's something every architecture must provide in some shape or form.
This CPP check business is fragile as hell and I'm not going to promote it in the architectures I maintainer by applying patches like your's.
Sorry.