Re: [PATCH] riscv: resolve most warnings from sparse

From: Christoph Hellwig
Date: Thu Sep 19 2019 - 13:31:48 EST


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