Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 21 Sep 2019 03:07:19 -0700 (PDT)
From:   Paul Walmsley <paul.walmsley@sifive.com>
To:     Christoph Hellwig <hch@infradead.org>
cc:     linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] riscv: resolve most warnings from sparse
In-Reply-To: <20190919173142.GA26224@infradead.org>
Message-ID: <alpine.DEB.2.21.9999.1909210301470.2030@viisi.sifive.com>
References: <alpine.DEB.2.21.9999.1909190125400.13510@viisi.sifive.com> <20190919173142.GA26224@infradead.org>
User-Agent: Alpine 2.21.9999 (DEB 301 2018-08-15)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Christoph,

On Thu, 19 Sep 2019, Christoph Hellwig wrote:

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

I agree that it's hard to review as it is, and will split it up into a few 
smaller patches.

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

If you have another suggestion, please let me know.  Adding this 
prototypes is perhaps not ideal, but I personally don't see any downside, 
aside from aesthetics.  Several other architectures have either asm/ or 
kernel/entry.h, and a few others have head.h, so it's at least in line 
with existing practice.

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

Thanks, will review the above issues.

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

Once you can get the ack from the EDAC maintainers, I'll most likely apply 
your previous patch.  In the meantime I'm planning to get the sparse fixes 
in early, since they are long overdue.


Thanks for the review,

- Paul