Re: [PATCH] RISC-V: Fixup clear_page export when using Zicboz

From: Andrew Jones <ajones@ventanamicro.com>
To: Ben Dooks <ben.dooks@codethink.co.uk>
Cc: linux-riscv@lists.infradead.org,
	Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
Subject: Re: [PATCH] RISC-V: Fixup clear_page export when using Zicboz
Date: Fri, 24 Feb 2023 16:19:45 +0100	[thread overview]
Message-ID: <20230224151945.od3ye74pk5ye3paz@orel> (raw)
In-Reply-To: <d9944799-7887-8c0b-ace1-7b340dacf9af@codethink.co.uk>

On Fri, Feb 24, 2023 at 02:42:11PM +0000, Ben Dooks wrote:
> On 24/02/2023 14:18, Andrew Jones wrote:
> > On Fri, Feb 24, 2023 at 01:58:44PM +0000, Ben Dooks wrote:
> > > When the clear_page() via Zicboz is enabled, the module build
> > > fails as clear_page() is not marked as a ksym entry. Fix this
> > > by changing the asm code to use <asm-generic/export.h> to add
> > > the correct export.
> > > 
> > > Also remove the weak clear_page() as there's nothing else in
> > > the build defining this symbol, so just make it the entry when
> > > the Zicboz is enabled.
> > > 
> > > Fixes modpost errors such as this:
> > > ERROR: modpost: "clear_page" [drivers/gpu/drm/ttm/ttm.ko] undefined!
> > > 
> > > Reported-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
> > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> > > ---
> > >   arch/riscv/lib/clear_page.S | 6 ++++--
> > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/riscv/lib/clear_page.S b/arch/riscv/lib/clear_page.S
> > > index 7c7fa45b5ab5..4ed5fef52d80 100644
> > > --- a/arch/riscv/lib/clear_page.S
> > > +++ b/arch/riscv/lib/clear_page.S
> > > @@ -6,6 +6,7 @@
> > >   #include <linux/linkage.h>
> > >   #include <asm/asm.h>
> > >   #include <asm/alternative-macros.h>
> > > +#include <asm-generic/export.h>
> > >   #include <asm/hwcap.h>
> > >   #include <asm/insn-def.h>
> > >   #include <asm/page.h>
> > > @@ -17,7 +18,7 @@
> > >   /* void clear_page(void *page) */
> > >   ENTRY(__clear_page)
> > > -WEAK(clear_page)
> > > +SYM_FUNC_START(clear_page)
> > >   	li	a2, PAGE_SIZE
> > >   	/*
> > > @@ -70,4 +71,5 @@ WEAK(clear_page)
> > >   .Lno_zicboz:
> > >   	li	a1, 0
> > >   	tail	__memset
> > > -END(__clear_page)
> > > +SYM_FUNC_END(clear_page)
> > > +EXPORT_SYMBOL(clear_page)
> > > -- 
> > > 2.39.1
> > > 
> > 
> > Hi Ben,
> > 
> > This looks good to me. I agree that I shouldn't have made clear_page weak,
> > but not because there isn't currently anything else defining it, but
> > because it's not something a subsystem is likely to ever define. Exporting
> > for modules is also definitely needed.
> > 
> > Is there any reason I shouldn't just squash this into the patch which
> > introduces clear_page? That patch is still under review, and merging
> > it broken just to immediately fix it would break bisection for no reason.
> 
> Yeah, as long as you note fixes from myself an sudip, then go for it.

I'll mention it in the changelog, but there's not really a good way to
give credit for things like this. There's either nothing (except the
changelog) or a Co-developed-by tag. I don't think this warrants the
latter.

Of course, I'd be happy to pick up T-b's and/or R-b's from you and Sudip,
which would then be applied to the patch(es), if you submit them.

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv