[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190919173142.GA26224@infradead.org>
Date: Thu, 19 Sep 2019 10:31:42 -0700
From: Christoph Hellwig <hch@...radead.org>
To: Paul Walmsley <paul.walmsley@...ive.com>
Cc: linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] riscv: resolve most warnings from sparse
On Thu, Sep 19, 2019 at 01:26:38AM -0700, Paul Walmsley wrote:
>
> Resolve most of the warnings emitted by sparse. The objective here is
> to keep arch/riscv as clean as possible with regards to sparse warnings,
> and to maintain this bar for subsequent patches.
I think this patch does just way to many different things and needs
to be split up into one patch per issue / code module.
> --- /dev/null
> +++ b/arch/riscv/include/asm/entry.h
For example adding this file should be a patch on its own. It can
also move to arch/riscv/kernel/ instead of polluting the <asm/*.h>
namespace. That being said I'm not sure I like this and the
head.h patches. Just adding a header for entry points used from
aseembly only seems rather pointless, I wonder if there is a way
to just shut up sparse on them. Same for most of head.h.
> @@ -61,6 +61,9 @@
>
> #define PAGE_TABLE __pgprot(_PAGE_TABLE)
>
> +extern pgd_t swapper_pg_dir[];
> +extern pgd_t trampoline_pg_dir[];
> +extern pgd_t early_pg_dir[];
> extern pgd_t swapper_pg_dir[];
This seems to add a duplicate definition of swapper_pg_dir.
> +extern asmlinkage void __init smp_callin(void);
No nee for the extern.
> index 905372d7eeb8..d0d980d99019 100644
> --- a/arch/riscv/include/asm/thread_info.h
> +++ b/arch/riscv/include/asm/thread_info.h
> @@ -58,6 +58,8 @@ struct thread_info {
> .addr_limit = KERNEL_DS, \
> }
>
> +extern int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
This really needs to move to a header outside of arch/. Also no need
for the extern and as-is this adds a line > 80 chars.
> +#ifdef CONFIG_PROFILING
> /* Unsupported */
> int setup_profiling_timer(unsigned int multiplier)
> {
> return -EINVAL;
> }
> +#endif
Yikes. All architectures either just return 0 or -EINVAL here,
and the caller has a spurious extern for it. Please just remove
this arch hook and add a Kconfig variable that the few architectures
currently returning 0 select insted.
> +static void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
This adds an > 80 char line.
> -pmd_t early_pmd[PTRS_PER_PMD * NUM_EARLY_PMDS] __initdata __aligned(PAGE_SIZE);
> +static pmd_t early_pmd[PTRS_PER_PMD * NUM_EARLY_PMDS] __initdata __aligned(PAGE_SIZE);
Another one.
> --- a/arch/riscv/mm/sifive_l2_cache.c
> +++ b/arch/riscv/mm/sifive_l2_cache.c
> @@ -142,7 +142,7 @@ static irqreturn_t l2_int_handler(int irq, void *device)
> return IRQ_HANDLED;
> }
>
> -int __init sifive_l2_init(void)
> +static int __init sifive_l2_init(void)
> {
> struct device_node *np;
> struct resource res;
And this needs to be applied after this file moves to the right place
and isn't completely bogusly built into every RISC-V kernel. Not all
the world is a SiFive..
Powered by blists - more mailing lists