On Mon, Mar 5, 2018 at 6:37 AM, Sergey Senozhatsky
<sergey.senozhatsky.w...@gmail.com> wrote:
> We want to move dump_stack related functions out of printk C
> code and consolidate them in lib/dump_stack file. The reason why
> dump_stack_print_info()/etc ended up in printk.c was a "generic"
> (dummy) lib dump_stack() function, which archs can override.
> For example, blackfin and nds32, define their own EXPORT_SYMBOL
> dump_stack() functions.
>
> In case of blackfin that arch-specific dump_stack() symbol invokes
> a global dump_stack_print_info() function. So we can't easily move
> dump_stack_print_info() to lib/dump_stack, because this way we will
> link with lib/dump_stack.o file, which will bring in a generic
> dump_stack() symbol with it, causing a multiple definitions error:
> - one dump_stack() from arch/blackfin/dumpstack
> - the other one from lib/dump_stack
>
> Convert generic dump_stack() into a weak symbol. So we will be
> able link with lib/dump_stack and at the same time archs will
> be able to override dump_stack(). It also opens up a way for us
> to move dump_stack_set_arch_desc(), dump_stack_print_info() and
> show_regs_print_info() to lib/dump_stack.
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhat...@gmail.com>
> ---
> arch/blackfin/kernel/dumpstack.c | 1 -
> arch/nds32/kernel/traps.c | 2 --
> lib/dump_stack.c | 4 ++--
> 3 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/arch/blackfin/kernel/dumpstack.c
> b/arch/blackfin/kernel/dumpstack.c
> index 3c992c1f8ef2..61af017130cd 100644
> --- a/arch/blackfin/kernel/dumpstack.c
> +++ b/arch/blackfin/kernel/dumpstack.c
> @@ -174,4 +174,3 @@ void dump_stack(void)
> show_stack(current, &stack);
> trace_buffer_restore(tflags);
> }
> -EXPORT_SYMBOL(dump_stack);
As we are now removing blackfin, based on the latest discussion, this
part should no longer be necessary.
> diff --git a/arch/nds32/kernel/traps.c b/arch/nds32/kernel/traps.c
> index 8828b4aeb72b..455bb0787367 100644
> --- a/arch/nds32/kernel/traps.c
> +++ b/arch/nds32/kernel/traps.c
> @@ -166,8 +166,6 @@ void dump_stack(void)
> __dump(NULL, base_reg);
> }
>
> -EXPORT_SYMBOL(dump_stack);
> -
> void show_stack(struct task_struct *tsk, unsigned long *sp)
> {
> unsigned long *base_reg;
nds32 currently only exists in linux-next, not in the mainline kernel.
If it's the only architecture that does something different from everyone
else, I think we should change nds32.
I looked at the nds32 show_stack() implementation now and it
seems to me that it is completely unnecessary, as the implementation
from lib/dump_stack.c does basically the same thing (by calling
show_stack(NULL, NULL)).
> diff --git a/lib/dump_stack.c b/lib/dump_stack.c
> index c5edbedd364d..02a8ad163760 100644
> --- a/lib/dump_stack.c
> +++ b/lib/dump_stack.c
> @@ -25,7 +25,7 @@ static void __dump_stack(void)
> #ifdef CONFIG_SMP
> static atomic_t dump_lock = ATOMIC_INIT(-1);
>
> -asmlinkage __visible void dump_stack(void)
> +asmlinkage __weak __visible void dump_stack(void)
> {
> unsigned long flags;
> int was_locked;
> @@ -58,7 +58,7 @@ asmlinkage __visible void dump_stack(void)
> local_irq_restore(flags);
> }
> #else
> -asmlinkage __visible void dump_stack(void)
> +asmlinkage __weak __visible void dump_stack(void)
> {
> __dump_stack();
> }
Weak symbols are generally discouraged in the kernel. We have
them in a couple of places, but I find them rather confusing as they
make it harder to figure out what is actually going on.
Arnd
