Re: [PATCH] sparc64: Add preprocessor symbols for PAGE_* pgprot_t values.

[Clemens Ladisch]

David Miller wrote:
>> 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 <clemens@xxxxxxxxxx>
>
> "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.

> 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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/