| Message ID | 20230224135844.619054-1-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | RISC-V: Fixup clear_page export when using Zicboz | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Failed to apply to next/pending-fixes or riscv/for-next |
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. Thanks, drew
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.
On 24/02/2023 1:58 pm, 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) Shouldn't this be removed also? Am I missing something?
On Fri, Feb 24, 2023 at 02:44:17PM +0000, Sudip Mukherjee wrote: > > > On 24/02/2023 1:58 pm, 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) > > Shouldn't this be removed also? Am I missing something? Yes, it should be dropped, I've done so while squashing. Thanks, drew
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
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)
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(-)