| Message ID | 20221006070818.3616-2-jszhang@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: improve boot time isa extensions handling | expand |
On Thu, Oct 06, 2022 at 03:08:11PM +0800, Jisheng Zhang wrote: > It's a bit wired to call riscv_noncoherent_supported() once when s/wired/weird/ s/once/each time/ > insmod a module. Move the calling out of feature patch func. > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > --- > arch/riscv/kernel/cpufeature.c | 7 +------ > arch/riscv/kernel/setup.c | 4 ++++ > 2 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index 3b5583db9d80..03611b3ef45e 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -272,12 +272,7 @@ static bool __init_or_module cpufeature_probe_zicbom(unsigned int stage) > case RISCV_ALTERNATIVES_EARLY_BOOT: > return false; > default: > - if (riscv_isa_extension_available(NULL, ZICBOM)) { > - riscv_noncoherent_supported(); > - return true; > - } else { > - return false; > - } > + return riscv_isa_extension_available(NULL, ZICBOM); > } > #endif > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > index 2dfc463b86bb..1a055c3f5d9d 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -299,6 +299,10 @@ void __init setup_arch(char **cmdline_p) > riscv_init_cbom_blocksize(); I think we can move this riscv_init_cbom_blocksize() down to be above the new #ifdef in order to keep like calls grouped. It doesn't matter though. > riscv_fill_hwcap(); > apply_boot_alternatives(); > +#ifdef CONFIG_RISCV_DMA_NONCOHERENT > + if (riscv_isa_extension_available(NULL, ZICBOM)) > + riscv_noncoherent_supported(); > +#endif > } > > static int __init topology_init(void) > -- > 2.37.2 Otherwise, Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
On Thu, Oct 06, 2022 at 03:10:33PM +0200, Andrew Jones wrote: > On Thu, Oct 06, 2022 at 03:08:11PM +0800, Jisheng Zhang wrote: > > It's a bit wired to call riscv_noncoherent_supported() once when > > s/wired/weird/ > > s/once/each time/ > > > insmod a module. Move the calling out of feature patch func. > > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > > --- > > arch/riscv/kernel/cpufeature.c | 7 +------ > > arch/riscv/kernel/setup.c | 4 ++++ > > 2 files changed, 5 insertions(+), 6 deletions(-) > > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > index 3b5583db9d80..03611b3ef45e 100644 > > --- a/arch/riscv/kernel/cpufeature.c > > +++ b/arch/riscv/kernel/cpufeature.c > > @@ -272,12 +272,7 @@ static bool __init_or_module cpufeature_probe_zicbom(unsigned int stage) > > case RISCV_ALTERNATIVES_EARLY_BOOT: > > return false; > > default: > > - if (riscv_isa_extension_available(NULL, ZICBOM)) { > > - riscv_noncoherent_supported(); > > - return true; > > - } else { > > - return false; > > - } > > + return riscv_isa_extension_available(NULL, ZICBOM); > > } > > #endif > > > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > > index 2dfc463b86bb..1a055c3f5d9d 100644 > > --- a/arch/riscv/kernel/setup.c > > +++ b/arch/riscv/kernel/setup.c > > @@ -299,6 +299,10 @@ void __init setup_arch(char **cmdline_p) > > riscv_init_cbom_blocksize(); > > I think we can move this riscv_init_cbom_blocksize() down to be above the > new #ifdef in order to keep like calls grouped. It doesn't matter though. Please ignore this comment. I see commit 8f7e001e0325 ("RISC-V: Clean up the Zicbom block size probing") specifically moved riscv_fill_hwcap() below riscv_init_cbom_blocksize(). Thanks, drew > > > riscv_fill_hwcap(); > > apply_boot_alternatives(); > > +#ifdef CONFIG_RISCV_DMA_NONCOHERENT > > + if (riscv_isa_extension_available(NULL, ZICBOM)) > > + riscv_noncoherent_supported(); > > +#endif > > } > > > > static int __init topology_init(void) > > -- > > 2.37.2 > > Otherwise, > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Hi, Am Donnerstag, 6. Oktober 2022, 09:08:11 CEST schrieb Jisheng Zhang: > It's a bit wired to call riscv_noncoherent_supported() once when > insmod a module. Move the calling out of feature patch func. > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > --- > arch/riscv/kernel/cpufeature.c | 7 +------ > arch/riscv/kernel/setup.c | 4 ++++ > 2 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index 3b5583db9d80..03611b3ef45e 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -272,12 +272,7 @@ static bool __init_or_module cpufeature_probe_zicbom(unsigned int stage) > case RISCV_ALTERNATIVES_EARLY_BOOT: > return false; > default: > - if (riscv_isa_extension_available(NULL, ZICBOM)) { > - riscv_noncoherent_supported(); > - return true; > - } else { > - return false; > - } > + return riscv_isa_extension_available(NULL, ZICBOM); > } > #endif > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > index 2dfc463b86bb..1a055c3f5d9d 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -299,6 +299,10 @@ void __init setup_arch(char **cmdline_p) > riscv_init_cbom_blocksize(); > riscv_fill_hwcap(); > apply_boot_alternatives(); > +#ifdef CONFIG_RISCV_DMA_NONCOHERENT > + if (riscv_isa_extension_available(NULL, ZICBOM)) > + riscv_noncoherent_supported(); > +#endif The nice thing about doing this in cpufeature_probe_zicbom is that you keep all the "ifs" in one place, where now you have 2 places that check the existence of the extension. The overhead is one "x = true" setting on each call to _probe() and with this change things are now also handled differently between the main implementation and the deviants. Though I guess, I'll let others do the judgement call on what is the desired way to go ;-) . Heiko
On Thu, Oct 06, 2022 at 03:08:11PM +0800, Jisheng Zhang wrote: > It's a bit wired to call riscv_noncoherent_supported() once when > insmod a module. Move the calling out of feature patch func. > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > --- > arch/riscv/kernel/cpufeature.c | 7 +------ > arch/riscv/kernel/setup.c | 4 ++++ > 2 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index 3b5583db9d80..03611b3ef45e 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -272,12 +272,7 @@ static bool __init_or_module cpufeature_probe_zicbom(unsigned int stage) > case RISCV_ALTERNATIVES_EARLY_BOOT: > return false; > default: > - if (riscv_isa_extension_available(NULL, ZICBOM)) { > - riscv_noncoherent_supported(); > - return true; > - } else { > - return false; > - } > + return riscv_isa_extension_available(NULL, ZICBOM); > } > #endif > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > index 2dfc463b86bb..1a055c3f5d9d 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -299,6 +299,10 @@ void __init setup_arch(char **cmdline_p) > riscv_init_cbom_blocksize(); > riscv_fill_hwcap(); > apply_boot_alternatives(); > +#ifdef CONFIG_RISCV_DMA_NONCOHERENT > + if (riscv_isa_extension_available(NULL, ZICBOM)) > + riscv_noncoherent_supported(); > +#endif I have a personal bias against ifdefs where possible, maybe @Heiko remembers why riscv_noncoherent_supported() was not defined as something like `void riscv_noncoherent_support(void){}` for when that CONFIG is not enabled? If it was this could become a an IS_ENABLED & we wouldn't have to be so careful about wrapping it's usage in ifdefs. Your change in isolation makes sense to me though, so: Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Thanks, Conor. > } > > static int __init topology_init(void) > -- > 2.37.2 >
On Sat, Oct 08, 2022 at 02:06:00PM +0100, Conor Dooley wrote: > On Thu, Oct 06, 2022 at 03:08:11PM +0800, Jisheng Zhang wrote: > > It's a bit wired to call riscv_noncoherent_supported() once when > > insmod a module. Move the calling out of feature patch func. > > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > > --- > > arch/riscv/kernel/cpufeature.c | 7 +------ > > arch/riscv/kernel/setup.c | 4 ++++ > > 2 files changed, 5 insertions(+), 6 deletions(-) > > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > index 3b5583db9d80..03611b3ef45e 100644 > > --- a/arch/riscv/kernel/cpufeature.c > > +++ b/arch/riscv/kernel/cpufeature.c > > @@ -272,12 +272,7 @@ static bool __init_or_module cpufeature_probe_zicbom(unsigned int stage) > > case RISCV_ALTERNATIVES_EARLY_BOOT: > > return false; > > default: > > - if (riscv_isa_extension_available(NULL, ZICBOM)) { > > - riscv_noncoherent_supported(); > > - return true; > > - } else { > > - return false; > > - } > > + return riscv_isa_extension_available(NULL, ZICBOM); > > } > > #endif > > > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > > index 2dfc463b86bb..1a055c3f5d9d 100644 > > --- a/arch/riscv/kernel/setup.c > > +++ b/arch/riscv/kernel/setup.c > > @@ -299,6 +299,10 @@ void __init setup_arch(char **cmdline_p) > > riscv_init_cbom_blocksize(); > > riscv_fill_hwcap(); > > apply_boot_alternatives(); > > +#ifdef CONFIG_RISCV_DMA_NONCOHERENT > > + if (riscv_isa_extension_available(NULL, ZICBOM)) > > + riscv_noncoherent_supported(); > > +#endif > > I have a personal bias against ifdefs where possible, maybe @Heiko > remembers why riscv_noncoherent_supported() was not defined as something > like `void riscv_noncoherent_support(void){}` for when that CONFIG is > not enabled? If it was this could become a an IS_ENABLED & we wouldn't > have to be so careful about wrapping it's usage in ifdefs. Good idea. Will do in newer version. > > Your change in isolation makes sense to me though, so: > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > > Thanks, > Conor. > > > } > > > > static int __init topology_init(void) > > -- > > 2.37.2 > >
On 8 October 2022 14:59:37 IST, Jisheng Zhang <jszhang@kernel.org> wrote: >On Sat, Oct 08, 2022 at 02:06:00PM +0100, Conor Dooley wrote: >> On Thu, Oct 06, 2022 at 03:08:11PM +0800, Jisheng Zhang wrote: >> > It's a bit wired to call riscv_noncoherent_supported() once when >> > insmod a module. Move the calling out of feature patch func. >> > >> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> >> > --- >> > arch/riscv/kernel/cpufeature.c | 7 +------ >> > arch/riscv/kernel/setup.c | 4 ++++ >> > 2 files changed, 5 insertions(+), 6 deletions(-) >> > >> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c >> > index 3b5583db9d80..03611b3ef45e 100644 >> > --- a/arch/riscv/kernel/cpufeature.c >> > +++ b/arch/riscv/kernel/cpufeature.c >> > @@ -272,12 +272,7 @@ static bool __init_or_module cpufeature_probe_zicbom(unsigned int stage) >> > case RISCV_ALTERNATIVES_EARLY_BOOT: >> > return false; >> > default: >> > - if (riscv_isa_extension_available(NULL, ZICBOM)) { >> > - riscv_noncoherent_supported(); >> > - return true; >> > - } else { >> > - return false; >> > - } >> > + return riscv_isa_extension_available(NULL, ZICBOM); >> > } >> > #endif >> > >> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c >> > index 2dfc463b86bb..1a055c3f5d9d 100644 >> > --- a/arch/riscv/kernel/setup.c >> > +++ b/arch/riscv/kernel/setup.c >> > @@ -299,6 +299,10 @@ void __init setup_arch(char **cmdline_p) >> > riscv_init_cbom_blocksize(); >> > riscv_fill_hwcap(); >> > apply_boot_alternatives(); >> > +#ifdef CONFIG_RISCV_DMA_NONCOHERENT >> > + if (riscv_isa_extension_available(NULL, ZICBOM)) >> > + riscv_noncoherent_supported(); >> > +#endif >> >> I have a personal bias against ifdefs where possible, maybe @Heiko >> remembers why riscv_noncoherent_supported() was not defined as something >> like `void riscv_noncoherent_support(void){}` for when that CONFIG is >> not enabled? If it was this could become a an IS_ENABLED & we wouldn't >> have to be so careful about wrapping it's usage in ifdefs. > >Good idea. Will do in newer version. Given this comment and the LKP report I've marked the series as changes requested in patchwork FYI. Thanks, Conor. > >> >> Your change in isolation makes sense to me though, so: >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com> >> >> Thanks, >> Conor. >> >> > } >> > >> > static int __init topology_init(void) >> > -- >> > 2.37.2 >> >
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index 3b5583db9d80..03611b3ef45e 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -272,12 +272,7 @@ static bool __init_or_module cpufeature_probe_zicbom(unsigned int stage) case RISCV_ALTERNATIVES_EARLY_BOOT: return false; default: - if (riscv_isa_extension_available(NULL, ZICBOM)) { - riscv_noncoherent_supported(); - return true; - } else { - return false; - } + return riscv_isa_extension_available(NULL, ZICBOM); } #endif diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c index 2dfc463b86bb..1a055c3f5d9d 100644 --- a/arch/riscv/kernel/setup.c +++ b/arch/riscv/kernel/setup.c @@ -299,6 +299,10 @@ void __init setup_arch(char **cmdline_p) riscv_init_cbom_blocksize(); riscv_fill_hwcap(); apply_boot_alternatives(); +#ifdef CONFIG_RISCV_DMA_NONCOHERENT + if (riscv_isa_extension_available(NULL, ZICBOM)) + riscv_noncoherent_supported(); +#endif } static int __init topology_init(void)
It's a bit wired to call riscv_noncoherent_supported() once when insmod a module. Move the calling out of feature patch func. Signed-off-by: Jisheng Zhang <jszhang@kernel.org> --- arch/riscv/kernel/cpufeature.c | 7 +------ arch/riscv/kernel/setup.c | 4 ++++ 2 files changed, 5 insertions(+), 6 deletions(-)