Re: [PATCH] dump_stack: convert generic dump_stack into a weak symbol

[Greentime Hu]

2018-03-07 17:09 GMT+08:00 Petr Mladek <pmladek@xxxxxxxx>:
> Ah, there was a mid-air collision. Arnd already answered most of my
> questions and even more.
>
> On Wed 2018-03-07 09:46:27, Arnd Bergmann wrote:
>> On Wed, Mar 7, 2018 at 3:21 AM, Sergey Senozhatsky
>> <sergey.senozhatsky.work@xxxxxxxxx> wrote:
>> > On (03/06/18 14:27), Arnd Bergmann wrote:
>> >> 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.
>> >
>> > Honestly, I kind of find __weak less confusing than EXPORT_SYMBOL(dump_stack)
>> > in 3 different places. __weak hints that the symbol likely will be overridden
>> > somewhere, while EXPORT_SYMBOL() does not (at least not to me). Dunno.
>>
>> It's not either/or, they are both wrong ;-)
>>
>> The EXPORT_SYMBOL() is not the thing that makes it work. The duplicate
>> declaration today only works because lib/dump_stack.o is listed as lib-y
>> in the Makefile instead of obj-y. On blackfin and nds32, this causes the entire
>> file to just not be included in the final vmlinux, because there are no
>> references to it.
>>
>> With your patch, I would actually expect the lib/dump_stack.o file
>> to still not be picked up, so now you have a missing EXPORT_SYMBOL()
>> on the two unusual architectures until the point when you add another
>> (referenced) symbol to it.
>
> Great catch! We should change it from lib-y to obj-y. Of course,
> the best solution would be if nds32 uses the generic implementation
> and we could avoid adding __weak.
>

I agree too.
I think I can add a patch to remove the dump_stack() implementation in
nds32 and use the generic one.

diff --git a/arch/nds32/kernel/traps.c b/arch/nds32/kernel/traps.c
index 8828b4aeb72b..6e34eb9824a4 100644
--- a/arch/nds32/kernel/traps.c
+++ b/arch/nds32/kernel/traps.c
@@ -156,18 +156,6 @@ static void __dump(struct task_struct *tsk,
unsigned long *base_reg)
        pr_emerg("\n");
 }

-void dump_stack(void)
-{
-       unsigned long *base_reg;
-       if (!IS_ENABLED(CONFIG_FRAME_POINTER))
-               __asm__ __volatile__("\tori\t%0, $sp, #0\n":"=r"(base_reg));
-       else
-               __asm__ __volatile__("\tori\t%0, $fp, #0\n":"=r"(base_reg));
-       __dump(NULL, base_reg);
-}
-
-EXPORT_SYMBOL(dump_stack);
-