On 10/03/2021 03.30, Chris Down wrote:
> ---
> MAINTAINERS | 5 +
> arch/arm/kernel/entry-v7m.S | 2 +-
> arch/arm/lib/backtrace-clang.S | 2 +-
> arch/arm/lib/backtrace.S | 2 +-
> arch/arm/mach-rpc/io-acorn.S | 2 +-
> arch/arm/vfp/vfphw.S | 6 +-
> arch/ia64/include/uapi/asm/cmpxchg.h | 4 +-
> arch/openrisc/kernel/entry.S | 6 +-
> arch/powerpc/kernel/head_fsl_booke.S | 2 +-
> arch/um/include/shared/user.h | 3 +-
> arch/x86/kernel/head_32.S | 2 +-
> fs/seq_file.c | 21 +++
> include/asm-generic/vmlinux.lds.h | 13 ++
> include/linux/module.h | 6 +
> include/linux/printk.h | 72 ++++++++++-
> include/linux/seq_file.h | 1 +
> include/linux/string_helpers.h | 2 +
> init/Kconfig | 14 ++
> kernel/module.c | 14 +-
> kernel/printk/Makefile | 1 +
> kernel/printk/index.c | 183 +++++++++++++++++++++++++++
> kernel/printk/printk.c | 20 ++-
> lib/string_helpers.c | 29 ++++-
> lib/test-string_helpers.c | 6 +
> 24 files changed, 386 insertions(+), 32 deletions(-)
> create mode 100644 kernel/printk/index.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3353de0c4bc8..328b3e822223 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14314,6 +14314,11 @@ S: Maintained
> F: include/linux/printk.h
> F: kernel/printk/
>
> +PRINTK INDEXING
> +R: Chris Down <ch...@chrisdown.name>
> +S: Maintained
> +F: kernel/printk/index.c
> +
> PRISM54 WIRELESS DRIVER
> M: Luis Chamberlain <mcg...@kernel.org>
> L: linux-wirel...@vger.kernel.org
> diff --git a/arch/arm/kernel/entry-v7m.S b/arch/arm/kernel/entry-v7m.S
> index d0e898608d30..7bde93c10962 100644
> --- a/arch/arm/kernel/entry-v7m.S
> +++ b/arch/arm/kernel/entry-v7m.S
> @@ -23,7 +23,7 @@ __invalid_entry:
> adr r0, strerr
> mrs r1, ipsr
> mov r2, lr
> - bl printk
> + bl _printk
I think it's pointless renaming the symbol to _printk, with all the
churn and reduced readability that involves (especially when reading
assembly "why are we calling _printk and not printk here?"). There's
nothing wrong with providing a macro wrapper by the same name
#define printk(bla bla) ({ do_stuff; printk(bla bla); })
Only two places would need to be updated to surround the word printk in
parentheses to suppress macro expansion: The declaration and the
definition of printk. I.e.
int (printk)(const char *s, ...)
>
> +struct module;
> +
> +#ifdef CONFIG_PRINTK_INDEX
> +extern void pi_sec_store(struct module *mod);
> +extern void pi_sec_remove(struct module *mod);
> +
> +struct pi_object {
> + const char *fmt;
> + const char *func;
> + const char *file;
> + unsigned int line;
> +};
> +
> +extern struct pi_object __start_printk_index[];
> +extern struct pi_object __stop_printk_index[];
Do you need these declarations to be visible to the whole kernel? Can't
they live in printk/index.c?
> +
> +#define pi_sec_elf_embed(_p_func, _fmt, ...) \
> + ({ \
> + int _p_ret; \
> + \
> + if (__builtin_constant_p(_fmt)) { \
> + /*
> + * The compiler may not be able to eliminate this, so
> + * we need to make sure that it doesn't see any
> + * hypothetical assignment for non-constants even
> + * though this is already inside the
> + * __builtin_constant_p guard.
> + */ \
> + static struct pi_object _pi \
static const struct pi_object?
> + __section(".printk_index") = { \
> + .fmt = __builtin_constant_p(_fmt) ? (_fmt) :
> NULL, \
> + .func = __func__, \
> + .file = __FILE__, \
> + .line = __LINE__, \
> + }; \
> + _p_ret = _p_func(_pi.fmt, ##__VA_ARGS__); \
Is the use of _pi.fmt here a trick to prevent gcc from eliding the _pi
object, so it is seen as "used"? That seems a bit fragile, especially if
the compiler ends up generating the same code in .text - that means gcc
does not load the format string from the _pi object (which it
shouldn't), but then I don't see why it (or the next version of gcc)
couldn't realize that _pi is indeed unused.
There's the __used attribute precisely for this kind of thing. Then you
could also eliminate
> + } else \
> + _p_ret = _p_func(_fmt, ##__VA_ARGS__); \
> + \
this and the _p_ret variable
> + _p_ret; \
and just end the ({}) with _p_func(_fmt, ##__VA_ARGS__);
That would also allow you to more easily wrap, say, dev_printk(), which
returns void - it seems that by not handling dev_printk and friends
you're missing quite a few format strings.
Rasmus
