| Message ID | 20220812154010.18280-1-palmer@rivosinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] RISC-V: Clean up the Zicbom block size probing | expand |
On 12/08/2022 16:40, Palmer Dabbelt wrote: > This fixes two issues: I truncated the warning's hart ID when porting to > the 64-bit hart ID code, and the original code's warning handling could > fire on an uninitialized hart ID. > > The biggest change here is that riscv_cbom_block_size is no longer > initialized, as IMO the default isn't sane: there's nothing in the ISA > that mandates any specific cache block size, so falling back to one will > just silently produce the wrong answer on some systems. This also > changes the probing order so the cache block size is known before > enabling Zicbom support. > > Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant") > Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension") > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> I just manually edited riscv/defconfig to add enable the zicbom config option, but I get the below with clang-15: /stuff/linux/arch/riscv/mm/dma-noncoherent.c:104:6: warning: format specifies type 'int' but the argument has type 'unsigned long' [-Wformat] cbom_hartid, hartid); ^~~~~~~~~~~ /stuff/linux/include/linux/printk.h:517:37: note: expanded from macro 'pr_warn' printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ /stuff/linux/include/linux/printk.h:464:60: note: expanded from macro 'printk' #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ /stuff/linux/include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap' _p_func(_fmt, ##__VA_ARGS__); \ ~~~~ ^~~~~~~~~~~ /stuff/linux/arch/riscv/mm/dma-noncoherent.c:104:6: warning: variable 'cbom_hartid' is uninitialized when used here [-Wuninitialized] cbom_hartid, hartid); ^~~~~~~~~~~ /stuff/linux/include/linux/printk.h:517:37: note: expanded from macro 'pr_warn' printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__) ^~~~~~~~~~~ /stuff/linux/include/linux/printk.h:464:60: note: expanded from macro 'printk' #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__) ^~~~~~~~~~~ /stuff/linux/include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap' _p_func(_fmt, ##__VA_ARGS__); \ ^~~~~~~~~~~ /stuff/linux/arch/riscv/mm/dma-noncoherent.c:87:36: note: initialize the variable 'cbom_hartid' to silence this warning unsigned long hartid, cbom_hartid; ^ = 0 /stuff/linux/arch/riscv/mm/dma-noncoherent.c:116:10: error: too many arguments provided to function-like macro invocation "Non-coherent DMA support enabled without a block size\n"); ^ /stuff/linux/include/asm-generic/bug.h:121:9: note: macro 'WARN_ON' defined here #define WARN_ON(condition) ({ \ ^ /stuff/linux/arch/riscv/mm/dma-noncoherent.c:115:2: error: use of undeclared identifier 'WARN_ON' WARN_ON(!riscv_cbom_block_size, ^ 2 warnings and 2 errors generated. make[5]: *** [/stuff/linux/scripts/Makefile.build:250: arch/riscv/mm/dma-noncoherent.o] Error 1 make[4]: *** [/stuff/linux/scripts/Makefile.build:525: arch/riscv/mm] Error 2 > > --- > > Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>: > > * Everything but the unsigned long cbom_hartid. > --- > arch/riscv/kernel/setup.c | 2 +- > arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++---------- > 2 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > index 95ef6e2bf45c..2dfc463b86bb 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p) > setup_smp(); > #endif > > - riscv_fill_hwcap(); > riscv_init_cbom_blocksize(); > + riscv_fill_hwcap(); > apply_boot_alternatives(); > } > > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c > index cd2225304c82..3aa3572715d6 100644 > --- a/arch/riscv/mm/dma-noncoherent.c > +++ b/arch/riscv/mm/dma-noncoherent.c > @@ -12,7 +12,7 @@ > #include <linux/of_device.h> > #include <asm/cacheflush.h> > > -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES; > +static unsigned int riscv_cbom_block_size; > static bool noncoherent_supported; > > void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, > @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void) > { > struct device_node *node; > int ret; > - u32 val; > + u32 val, probed_block_size; > > + probed_block_size = 0; > for_each_of_cpu_node(node) { > - unsigned long hartid; > - int cbom_hartid; > + unsigned long hartid, cbom_hartid; > > ret = riscv_of_processor_hartid(node, &hartid); > if (ret) > continue; > > - if (hartid < 0) > - continue; > - > /* set block-size for cbom extension if available */ > ret = of_property_read_u32(node, "riscv,cbom-block-size", &val); > if (ret) > continue; > > - if (!riscv_cbom_block_size) { > - riscv_cbom_block_size = val; > + if (!probed_block_size) { > + probed_block_size = val; > cbom_hartid = hartid; > } else { > - if (riscv_cbom_block_size != val) > + if (probed_block_size != val) > pr_warn("cbom-block-size mismatched between harts %d and %lu\n", > cbom_hartid, hartid); > } > } > + > + if (probed_block_size) > + riscv_cbom_block_size = probed_block_size; > } > #endif > > void riscv_noncoherent_supported(void) > { > + WARN_ON(!riscv_cbom_block_size, > + "Non-coherent DMA support enabled without a block size\n"); > noncoherent_supported = true; > }
On Fri, Aug 12, 2022 at 9:06 AM Palmer Dabbelt <palmer@rivosinc.com> wrote: > > This fixes two issues: I truncated the warning's hart ID when porting to > the 64-bit hart ID code, and the original code's warning handling could > fire on an uninitialized hart ID. > > The biggest change here is that riscv_cbom_block_size is no longer > initialized, as IMO the default isn't sane: there's nothing in the ISA > that mandates any specific cache block size, so falling back to one will > just silently produce the wrong answer on some systems. This also > changes the probing order so the cache block size is known before > enabling Zicbom support. > > Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant") > Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension") > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> > > --- > > Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>: > > * Everything but the unsigned long cbom_hartid. > --- > arch/riscv/kernel/setup.c | 2 +- > arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++---------- > 2 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > index 95ef6e2bf45c..2dfc463b86bb 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p) > setup_smp(); > #endif > > - riscv_fill_hwcap(); > riscv_init_cbom_blocksize(); > + riscv_fill_hwcap(); > apply_boot_alternatives(); > } > > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c > index cd2225304c82..3aa3572715d6 100644 > --- a/arch/riscv/mm/dma-noncoherent.c > +++ b/arch/riscv/mm/dma-noncoherent.c > @@ -12,7 +12,7 @@ > #include <linux/of_device.h> > #include <asm/cacheflush.h> > > -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES; > +static unsigned int riscv_cbom_block_size; What is the expected behavior if the block size is zero in CMO operations ? As per my understanding, it will be equivalent to a nop. Let me know if I am wrong. If that is the case, this is misleading as well. Maybe we should just disable CMO extension altogether if it can't find the DT property. > static bool noncoherent_supported; > > void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, > @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void) > { > struct device_node *node; > int ret; > - u32 val; > + u32 val, probed_block_size; > > + probed_block_size = 0; > for_each_of_cpu_node(node) { > - unsigned long hartid; > - int cbom_hartid; > + unsigned long hartid, cbom_hartid; > > ret = riscv_of_processor_hartid(node, &hartid); > if (ret) > continue; > > - if (hartid < 0) > - continue; > - > /* set block-size for cbom extension if available */ > ret = of_property_read_u32(node, "riscv,cbom-block-size", &val); > if (ret) > continue; > > - if (!riscv_cbom_block_size) { > - riscv_cbom_block_size = val; > + if (!probed_block_size) { > + probed_block_size = val; > cbom_hartid = hartid; > } else { > - if (riscv_cbom_block_size != val) > + if (probed_block_size != val) > pr_warn("cbom-block-size mismatched between harts %d and %lu\n", > cbom_hartid, hartid); > } > } > + > + if (probed_block_size) > + riscv_cbom_block_size = probed_block_size; > } > #endif > > void riscv_noncoherent_supported(void) > { > + WARN_ON(!riscv_cbom_block_size, > + "Non-coherent DMA support enabled without a block size\n"); > noncoherent_supported = true; > } > -- > 2.34.1 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Fri, 12 Aug 2022 10:43:02 PDT (-0700), atishp@atishpatra.org wrote: > On Fri, Aug 12, 2022 at 9:06 AM Palmer Dabbelt <palmer@rivosinc.com> wrote: >> >> This fixes two issues: I truncated the warning's hart ID when porting to >> the 64-bit hart ID code, and the original code's warning handling could >> fire on an uninitialized hart ID. >> >> The biggest change here is that riscv_cbom_block_size is no longer >> initialized, as IMO the default isn't sane: there's nothing in the ISA >> that mandates any specific cache block size, so falling back to one will >> just silently produce the wrong answer on some systems. This also >> changes the probing order so the cache block size is known before >> enabling Zicbom support. >> >> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant") >> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension") >> Reported-by: kernel test robot <lkp@intel.com> >> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> >> >> --- >> >> Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>: >> >> * Everything but the unsigned long cbom_hartid. >> --- >> arch/riscv/kernel/setup.c | 2 +- >> arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++---------- >> 2 files changed, 13 insertions(+), 11 deletions(-) >> >> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c >> index 95ef6e2bf45c..2dfc463b86bb 100644 >> --- a/arch/riscv/kernel/setup.c >> +++ b/arch/riscv/kernel/setup.c >> @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p) >> setup_smp(); >> #endif >> >> - riscv_fill_hwcap(); >> riscv_init_cbom_blocksize(); >> + riscv_fill_hwcap(); >> apply_boot_alternatives(); >> } >> >> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c >> index cd2225304c82..3aa3572715d6 100644 >> --- a/arch/riscv/mm/dma-noncoherent.c >> +++ b/arch/riscv/mm/dma-noncoherent.c >> @@ -12,7 +12,7 @@ >> #include <linux/of_device.h> >> #include <asm/cacheflush.h> >> >> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES; >> +static unsigned int riscv_cbom_block_size; > > What is the expected behavior if the block size is zero in CMO > operations ? As per my understanding, it will be equivalent to a nop. > Let me know if I am wrong. > > If that is the case, this is misleading as well. Maybe we should just > disable CMO extension altogether if it can't find the DT property. That seems reasonable to me, even if having a 0 block size is allowed by the spec it seems way more likely to have been a mistake. I'll send a v3, after puttting together a toolchain that actually builds this (assuming that's why I'm not getting the failures/warnings). > >> static bool noncoherent_supported; >> >> void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, >> @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void) >> { >> struct device_node *node; >> int ret; >> - u32 val; >> + u32 val, probed_block_size; >> >> + probed_block_size = 0; >> for_each_of_cpu_node(node) { >> - unsigned long hartid; >> - int cbom_hartid; >> + unsigned long hartid, cbom_hartid; >> >> ret = riscv_of_processor_hartid(node, &hartid); >> if (ret) >> continue; >> >> - if (hartid < 0) >> - continue; >> - >> /* set block-size for cbom extension if available */ >> ret = of_property_read_u32(node, "riscv,cbom-block-size", &val); >> if (ret) >> continue; >> >> - if (!riscv_cbom_block_size) { >> - riscv_cbom_block_size = val; >> + if (!probed_block_size) { >> + probed_block_size = val; >> cbom_hartid = hartid; >> } else { >> - if (riscv_cbom_block_size != val) >> + if (probed_block_size != val) >> pr_warn("cbom-block-size mismatched between harts %d and %lu\n", >> cbom_hartid, hartid); >> } >> } >> + >> + if (probed_block_size) >> + riscv_cbom_block_size = probed_block_size; >> } >> #endif >> >> void riscv_noncoherent_supported(void) >> { >> + WARN_ON(!riscv_cbom_block_size, >> + "Non-coherent DMA support enabled without a block size\n"); >> noncoherent_supported = true; >> } >> -- >> 2.34.1 >> >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi Palmer, On Fri, Aug 12, 2022 at 08:40:10AM -0700, Palmer Dabbelt wrote: > This fixes two issues: I truncated the warning's hart ID when porting to > the 64-bit hart ID code, and the original code's warning handling could > fire on an uninitialized hart ID. > > The biggest change here is that riscv_cbom_block_size is no longer > initialized, as IMO the default isn't sane: there's nothing in the ISA > that mandates any specific cache block size, so falling back to one will > just silently produce the wrong answer on some systems. This also > changes the probing order so the cache block size is known before > enabling Zicbom support. > > Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant") > Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension") > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> > > --- > > Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>: > > * Everything but the unsigned long cbom_hartid. > --- > arch/riscv/kernel/setup.c | 2 +- > arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++---------- > 2 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > index 95ef6e2bf45c..2dfc463b86bb 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p) > setup_smp(); > #endif > > - riscv_fill_hwcap(); > riscv_init_cbom_blocksize(); > + riscv_fill_hwcap(); > apply_boot_alternatives(); > } > > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c > index cd2225304c82..3aa3572715d6 100644 > --- a/arch/riscv/mm/dma-noncoherent.c > +++ b/arch/riscv/mm/dma-noncoherent.c > @@ -12,7 +12,7 @@ > #include <linux/of_device.h> > #include <asm/cacheflush.h> > > -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES; > +static unsigned int riscv_cbom_block_size; > static bool noncoherent_supported; > > void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, > @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void) > { > struct device_node *node; > int ret; > - u32 val; > + u32 val, probed_block_size; > > + probed_block_size = 0; > for_each_of_cpu_node(node) { > - unsigned long hartid; > - int cbom_hartid; > + unsigned long hartid, cbom_hartid; > > ret = riscv_of_processor_hartid(node, &hartid); > if (ret) > continue; > > - if (hartid < 0) > - continue; > - > /* set block-size for cbom extension if available */ > ret = of_property_read_u32(node, "riscv,cbom-block-size", &val); > if (ret) > continue; > > - if (!riscv_cbom_block_size) { > - riscv_cbom_block_size = val; > + if (!probed_block_size) { > + probed_block_size = val; > cbom_hartid = hartid; > } else { > - if (riscv_cbom_block_size != val) > + if (probed_block_size != val) > pr_warn("cbom-block-size mismatched between harts %d and %lu\n", ^ %lu? > cbom_hartid, hartid); > } > } > + > + if (probed_block_size) > + riscv_cbom_block_size = probed_block_size; > } > #endif > > void riscv_noncoherent_supported(void) > { > + WARN_ON(!riscv_cbom_block_size, > + "Non-coherent DMA support enabled without a block size\n"); > noncoherent_supported = true; > } > -- > 2.34.1 For what it's worth, while this should address the uninitialized cbom_hartid at runtime (from the quick glance I gave it), it doesn't address the compile time warning. I am not sure how to make it clear to clang that the if statement will be executed during the first loop iteration because probed_block_size is initialized to zero... Additionally, it appears that WARN() is the right macro, not WARN_ON() and an '#include <asm/bug.h>' is needed. arch/riscv/mm/dma-noncoherent.c:104:6: error: variable 'cbom_hartid' is uninitialized when used here [-Werror,-Wuninitialized] cbom_hartid, hartid); ^~~~~~~~~~~ include/linux/printk.h:517:37: note: expanded from macro 'pr_warn' printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__) ^~~~~~~~~~~ include/linux/printk.h:464:60: note: expanded from macro 'printk' #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__) ^~~~~~~~~~~ include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap' _p_func(_fmt, ##__VA_ARGS__); \ ^~~~~~~~~~~ arch/riscv/mm/dma-noncoherent.c:87:36: note: initialize the variable 'cbom_hartid' to silence this warning unsigned long hartid, cbom_hartid; ^ = 0 arch/riscv/mm/dma-noncoherent.c:116:10: error: too many arguments provided to function-like macro invocation "Non-coherent DMA support enabled without a block size\n"); ^ include/asm-generic/bug.h:121:9: note: macro 'WARN_ON' defined here #define WARN_ON(condition) ({ \ ^ arch/riscv/mm/dma-noncoherent.c:115:2: error: use of undeclared identifier 'WARN_ON' WARN_ON(!riscv_cbom_block_size, ^ 3 errors generated. Cheers, Nathan
> On 15 Aug 2022, at 16:40, Nathan Chancellor <nathan@kernel.org> wrote: > > Hi Palmer, > > On Fri, Aug 12, 2022 at 08:40:10AM -0700, Palmer Dabbelt wrote: >> This fixes two issues: I truncated the warning's hart ID when porting to >> the 64-bit hart ID code, and the original code's warning handling could >> fire on an uninitialized hart ID. >> >> The biggest change here is that riscv_cbom_block_size is no longer >> initialized, as IMO the default isn't sane: there's nothing in the ISA >> that mandates any specific cache block size, so falling back to one will >> just silently produce the wrong answer on some systems. This also >> changes the probing order so the cache block size is known before >> enabling Zicbom support. >> >> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant") >> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension") >> Reported-by: kernel test robot <lkp@intel.com> >> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> >> >> --- >> >> Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>: >> >> * Everything but the unsigned long cbom_hartid. >> --- >> arch/riscv/kernel/setup.c | 2 +- >> arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++---------- >> 2 files changed, 13 insertions(+), 11 deletions(-) >> >> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c >> index 95ef6e2bf45c..2dfc463b86bb 100644 >> --- a/arch/riscv/kernel/setup.c >> +++ b/arch/riscv/kernel/setup.c >> @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p) >> setup_smp(); >> #endif >> >> - riscv_fill_hwcap(); >> riscv_init_cbom_blocksize(); >> + riscv_fill_hwcap(); >> apply_boot_alternatives(); >> } >> >> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c >> index cd2225304c82..3aa3572715d6 100644 >> --- a/arch/riscv/mm/dma-noncoherent.c >> +++ b/arch/riscv/mm/dma-noncoherent.c >> @@ -12,7 +12,7 @@ >> #include <linux/of_device.h> >> #include <asm/cacheflush.h> >> >> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES; >> +static unsigned int riscv_cbom_block_size; >> static bool noncoherent_supported; >> >> void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, >> @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void) >> { >> struct device_node *node; >> int ret; >> - u32 val; >> + u32 val, probed_block_size; >> >> + probed_block_size = 0; >> for_each_of_cpu_node(node) { >> - unsigned long hartid; >> - int cbom_hartid; >> + unsigned long hartid, cbom_hartid; >> >> ret = riscv_of_processor_hartid(node, &hartid); >> if (ret) >> continue; >> >> - if (hartid < 0) >> - continue; >> - >> /* set block-size for cbom extension if available */ >> ret = of_property_read_u32(node, "riscv,cbom-block-size", &val); >> if (ret) >> continue; >> >> - if (!riscv_cbom_block_size) { >> - riscv_cbom_block_size = val; >> + if (!probed_block_size) { >> + probed_block_size = val; >> cbom_hartid = hartid; >> } else { >> - if (riscv_cbom_block_size != val) >> + if (probed_block_size != val) >> pr_warn("cbom-block-size mismatched between harts %d and %lu\n", > > ^ %lu? > >> cbom_hartid, hartid); >> } >> } >> + >> + if (probed_block_size) >> + riscv_cbom_block_size = probed_block_size; >> } >> #endif >> >> void riscv_noncoherent_supported(void) >> { >> + WARN_ON(!riscv_cbom_block_size, >> + "Non-coherent DMA support enabled without a block size\n"); >> noncoherent_supported = true; >> } >> -- >> 2.34.1 > > For what it's worth, while this should address the uninitialized > cbom_hartid at runtime (from the quick glance I gave it), it doesn't > address the compile time warning. I am not sure how to make it clear to > clang that the if statement will be executed during the first loop > iteration because probed_block_size is initialized to zero... The warnings are correct; the variables are declared inside the body, as I pointed out on IRC when people were discussing the function. Jess > Additionally, it appears that WARN() is the right macro, not WARN_ON() > and an '#include <asm/bug.h>' is needed. > > arch/riscv/mm/dma-noncoherent.c:104:6: error: variable 'cbom_hartid' is uninitialized when used here [-Werror,-Wuninitialized] > cbom_hartid, hartid); > ^~~~~~~~~~~ > include/linux/printk.h:517:37: note: expanded from macro 'pr_warn' > printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__) > ^~~~~~~~~~~ > include/linux/printk.h:464:60: note: expanded from macro 'printk' > #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__) > ^~~~~~~~~~~ > include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap' > _p_func(_fmt, ##__VA_ARGS__); \ > ^~~~~~~~~~~ > arch/riscv/mm/dma-noncoherent.c:87:36: note: initialize the variable 'cbom_hartid' to silence this warning > unsigned long hartid, cbom_hartid; > ^ > = 0 > arch/riscv/mm/dma-noncoherent.c:116:10: error: too many arguments provided to function-like macro invocation > "Non-coherent DMA support enabled without a block size\n"); > ^ > include/asm-generic/bug.h:121:9: note: macro 'WARN_ON' defined here > #define WARN_ON(condition) ({ \ > ^ > arch/riscv/mm/dma-noncoherent.c:115:2: error: use of undeclared identifier 'WARN_ON' > WARN_ON(!riscv_cbom_block_size, > ^ > 3 errors generated. > > Cheers, > Nathan > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Mon, Aug 15, 2022 at 06:36:14PM +0100, Jessica Clarke wrote: > > On 15 Aug 2022, at 16:40, Nathan Chancellor <nathan@kernel.org> wrote: > > > > Hi Palmer, > > > > On Fri, Aug 12, 2022 at 08:40:10AM -0700, Palmer Dabbelt wrote: > >> This fixes two issues: I truncated the warning's hart ID when porting to > >> the 64-bit hart ID code, and the original code's warning handling could > >> fire on an uninitialized hart ID. > >> > >> The biggest change here is that riscv_cbom_block_size is no longer > >> initialized, as IMO the default isn't sane: there's nothing in the ISA > >> that mandates any specific cache block size, so falling back to one will > >> just silently produce the wrong answer on some systems. This also > >> changes the probing order so the cache block size is known before > >> enabling Zicbom support. > >> > >> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant") > >> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension") > >> Reported-by: kernel test robot <lkp@intel.com> > >> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> > >> > >> --- > >> > >> Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>: > >> > >> * Everything but the unsigned long cbom_hartid. > >> --- > >> arch/riscv/kernel/setup.c | 2 +- > >> arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++---------- > >> 2 files changed, 13 insertions(+), 11 deletions(-) > >> > >> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > >> index 95ef6e2bf45c..2dfc463b86bb 100644 > >> --- a/arch/riscv/kernel/setup.c > >> +++ b/arch/riscv/kernel/setup.c > >> @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p) > >> setup_smp(); > >> #endif > >> > >> - riscv_fill_hwcap(); > >> riscv_init_cbom_blocksize(); > >> + riscv_fill_hwcap(); > >> apply_boot_alternatives(); > >> } > >> > >> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c > >> index cd2225304c82..3aa3572715d6 100644 > >> --- a/arch/riscv/mm/dma-noncoherent.c > >> +++ b/arch/riscv/mm/dma-noncoherent.c > >> @@ -12,7 +12,7 @@ > >> #include <linux/of_device.h> > >> #include <asm/cacheflush.h> > >> > >> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES; > >> +static unsigned int riscv_cbom_block_size; > >> static bool noncoherent_supported; > >> > >> void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, > >> @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void) > >> { > >> struct device_node *node; > >> int ret; > >> - u32 val; > >> + u32 val, probed_block_size; > >> > >> + probed_block_size = 0; > >> for_each_of_cpu_node(node) { > >> - unsigned long hartid; > >> - int cbom_hartid; > >> + unsigned long hartid, cbom_hartid; > >> > >> ret = riscv_of_processor_hartid(node, &hartid); > >> if (ret) > >> continue; > >> > >> - if (hartid < 0) > >> - continue; > >> - > >> /* set block-size for cbom extension if available */ > >> ret = of_property_read_u32(node, "riscv,cbom-block-size", &val); > >> if (ret) > >> continue; > >> > >> - if (!riscv_cbom_block_size) { > >> - riscv_cbom_block_size = val; > >> + if (!probed_block_size) { > >> + probed_block_size = val; > >> cbom_hartid = hartid; > >> } else { > >> - if (riscv_cbom_block_size != val) > >> + if (probed_block_size != val) > >> pr_warn("cbom-block-size mismatched between harts %d and %lu\n", > > > > ^ %lu? > > > >> cbom_hartid, hartid); > >> } > >> } > >> + > >> + if (probed_block_size) > >> + riscv_cbom_block_size = probed_block_size; > >> } > >> #endif > >> > >> void riscv_noncoherent_supported(void) > >> { > >> + WARN_ON(!riscv_cbom_block_size, > >> + "Non-coherent DMA support enabled without a block size\n"); > >> noncoherent_supported = true; > >> } > >> -- > >> 2.34.1 > > > > For what it's worth, while this should address the uninitialized > > cbom_hartid at runtime (from the quick glance I gave it), it doesn't > > address the compile time warning. I am not sure how to make it clear to > > clang that the if statement will be executed during the first loop > > iteration because probed_block_size is initialized to zero... > > The warnings are correct; the variables are declared inside the body, > as I pointed out on IRC when people were discussing the function. Ugh, I don't know how I missed that :/ guess that's what I get for replying to emails before I am fully awake... Cheers, Nathan > > Additionally, it appears that WARN() is the right macro, not WARN_ON() > > and an '#include <asm/bug.h>' is needed. > > > > arch/riscv/mm/dma-noncoherent.c:104:6: error: variable 'cbom_hartid' is uninitialized when used here [-Werror,-Wuninitialized] > > cbom_hartid, hartid); > > ^~~~~~~~~~~ > > include/linux/printk.h:517:37: note: expanded from macro 'pr_warn' > > printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__) > > ^~~~~~~~~~~ > > include/linux/printk.h:464:60: note: expanded from macro 'printk' > > #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__) > > ^~~~~~~~~~~ > > include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap' > > _p_func(_fmt, ##__VA_ARGS__); \ > > ^~~~~~~~~~~ > > arch/riscv/mm/dma-noncoherent.c:87:36: note: initialize the variable 'cbom_hartid' to silence this warning > > unsigned long hartid, cbom_hartid; > > ^ > > = 0 > > arch/riscv/mm/dma-noncoherent.c:116:10: error: too many arguments provided to function-like macro invocation > > "Non-coherent DMA support enabled without a block size\n"); > > ^ > > include/asm-generic/bug.h:121:9: note: macro 'WARN_ON' defined here > > #define WARN_ON(condition) ({ \ > > ^ > > arch/riscv/mm/dma-noncoherent.c:115:2: error: use of undeclared identifier 'WARN_ON' > > WARN_ON(!riscv_cbom_block_size, > > ^ > > 3 errors generated. > > > > Cheers, > > Nathan > > > > _______________________________________________ > > linux-riscv mailing list > > linux-riscv@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv >
On 13/08/2022 00:07, Palmer Dabbelt wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Fri, 12 Aug 2022 10:43:02 PDT (-0700), atishp@atishpatra.org wrote: >> On Fri, Aug 12, 2022 at 9:06 AM Palmer Dabbelt <palmer@rivosinc.com> wrote: >>> >>> This fixes two issues: I truncated the warning's hart ID when porting to >>> the 64-bit hart ID code, and the original code's warning handling could >>> fire on an uninitialized hart ID. >>> >>> The biggest change here is that riscv_cbom_block_size is no longer >>> initialized, as IMO the default isn't sane: there's nothing in the ISA >>> that mandates any specific cache block size, so falling back to one will >>> just silently produce the wrong answer on some systems. This also >>> changes the probing order so the cache block size is known before >>> enabling Zicbom support. >>> >>> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant") >>> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension") >>> Reported-by: kernel test robot <lkp@intel.com> >>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> >>> >>> --- >>> >>> Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>: >>> >>> * Everything but the unsigned long cbom_hartid. >>> --- >>> arch/riscv/kernel/setup.c | 2 +- >>> arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++---------- >>> 2 files changed, 13 insertions(+), 11 deletions(-) >>> >>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c >>> index 95ef6e2bf45c..2dfc463b86bb 100644 >>> --- a/arch/riscv/kernel/setup.c >>> +++ b/arch/riscv/kernel/setup.c >>> @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p) >>> setup_smp(); >>> #endif >>> >>> - riscv_fill_hwcap(); >>> riscv_init_cbom_blocksize(); >>> + riscv_fill_hwcap(); >>> apply_boot_alternatives(); >>> } >>> >>> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c >>> index cd2225304c82..3aa3572715d6 100644 >>> --- a/arch/riscv/mm/dma-noncoherent.c >>> +++ b/arch/riscv/mm/dma-noncoherent.c >>> @@ -12,7 +12,7 @@ >>> #include <linux/of_device.h> >>> #include <asm/cacheflush.h> >>> >>> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES; >>> +static unsigned int riscv_cbom_block_size; >> >> What is the expected behavior if the block size is zero in CMO >> operations ? As per my understanding, it will be equivalent to a nop. >> Let me know if I am wrong. >> >> If that is the case, this is misleading as well. Maybe we should just >> disable CMO extension altogether if it can't find the DT property. > > That seems reasonable to me, even if having a 0 block size is allowed by > the spec it seems way more likely to have been a mistake. I'll send a > v3, after puttting together a toolchain that actually builds this > (assuming that's why I'm not getting the failures/warnings). What's the plan here with v3? I folded the following into this version locally to clear both the original warning & the build error with this patch on clang-15: diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c index 3aa3572715d6..8a49ea5ba01d 100644 --- a/arch/riscv/mm/dma-noncoherent.c +++ b/arch/riscv/mm/dma-noncoherent.c @@ -79,12 +79,13 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, void riscv_init_cbom_blocksize(void) { struct device_node *node; - int ret; + unsigned long cbom_hartid; u32 val, probed_block_size; + int ret; probed_block_size = 0; for_each_of_cpu_node(node) { - unsigned long hartid, cbom_hartid; + unsigned long hartid; ret = riscv_of_processor_hartid(node, &hartid); if (ret) @@ -100,7 +101,7 @@ void riscv_init_cbom_blocksize(void) cbom_hartid = hartid; } else { if (probed_block_size != val) - pr_warn("cbom-block-size mismatched between harts %d and %lu\n", + pr_warn("cbom-block-size mismatched between harts %lu and %lu\n", cbom_hartid, hartid); } } @@ -112,7 +113,7 @@ void riscv_init_cbom_blocksize(void) void riscv_noncoherent_supported(void) { - WARN_ON(!riscv_cbom_block_size, - "Non-coherent DMA support enabled without a block size\n"); + WARN(!riscv_cbom_block_size, + "Non-coherent DMA support enabled without a block size\n"); noncoherent_supported = true; } > >> >>> static bool noncoherent_supported; >>> >>> void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, >>> @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void) >>> { >>> struct device_node *node; >>> int ret; >>> - u32 val; >>> + u32 val, probed_block_size; >>> >>> + probed_block_size = 0; >>> for_each_of_cpu_node(node) { >>> - unsigned long hartid; >>> - int cbom_hartid; >>> + unsigned long hartid, cbom_hartid; >>> >>> ret = riscv_of_processor_hartid(node, &hartid); >>> if (ret) >>> continue; >>> >>> - if (hartid < 0) >>> - continue; >>> - >>> /* set block-size for cbom extension if available */ >>> ret = of_property_read_u32(node, "riscv,cbom-block-size", &val); >>> if (ret) >>> continue; >>> >>> - if (!riscv_cbom_block_size) { >>> - riscv_cbom_block_size = val; >>> + if (!probed_block_size) { >>> + probed_block_size = val; >>> cbom_hartid = hartid; >>> } else { >>> - if (riscv_cbom_block_size != val) >>> + if (probed_block_size != val) >>> pr_warn("cbom-block-size mismatched between harts %d and %lu\n", >>> cbom_hartid, hartid); >>> } >>> } >>> + >>> + if (probed_block_size) >>> + riscv_cbom_block_size = probed_block_size; >>> } >>> #endif >>> >>> void riscv_noncoherent_supported(void) >>> { >>> + WARN_ON(!riscv_cbom_block_size, >>> + "Non-coherent DMA support enabled without a block size\n"); >>> noncoherent_supported = true; >>> } >>> -- >>> 2.34.1 >>> >>>
Am Freitag, 12. August 2022, 17:40:10 CEST schrieb Palmer Dabbelt: > This fixes two issues: I truncated the warning's hart ID when porting to > the 64-bit hart ID code, and the original code's warning handling could > fire on an uninitialized hart ID. > > The biggest change here is that riscv_cbom_block_size is no longer > initialized, as IMO the default isn't sane: there's nothing in the ISA > that mandates any specific cache block size, so falling back to one will > just silently produce the wrong answer on some systems. This also > changes the probing order so the cache block size is known before > enabling Zicbom support. > > Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant") > Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension") > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> With Conor's fixes folded in, this compiles and breaks the T-Head CMOs as they rely on that default value :-) . Can we do the following: (1) pick Anup's patch moving the block-size init over to cacheflush [0] (2) apply this patch (with Conor's fixes and adapted to the changed location) and add this one additional line: diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c index d4b1526538ad..67fa078f303f 100644 --- a/arch/riscv/errata/thead/errata.c +++ b/arch/riscv/errata/thead/errata.c @@ -42,6 +42,7 @@ static bool errata_probe_cmo(unsigned int stage, if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) return false; + riscv_cbom_block_size = L1_CACHE_BYTES; riscv_noncoherent_supported(); return true; } With that done everything works (again) and looks great, so would be Reviewed-by: Heiko Stuebner <heiko@sntech.de> Tested-by: Heiko Stuebner <heiko@sntech.de> Heiko [0] https://lore.kernel.org/r/20220830044642.566769-3-apatel@ventanamicro.com
On 01/09/2022 16:57, Heiko Stübner wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am Freitag, 12. August 2022, 17:40:10 CEST schrieb Palmer Dabbelt: >> This fixes two issues: I truncated the warning's hart ID when porting to >> the 64-bit hart ID code, and the original code's warning handling could >> fire on an uninitialized hart ID. >> >> The biggest change here is that riscv_cbom_block_size is no longer >> initialized, as IMO the default isn't sane: there's nothing in the ISA >> that mandates any specific cache block size, so falling back to one will >> just silently produce the wrong answer on some systems. This also >> changes the probing order so the cache block size is known before >> enabling Zicbom support. >> >> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant") >> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension") >> Reported-by: kernel test robot <lkp@intel.com> >> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> > > With Conor's fixes folded in, this compiles and breaks the T-Head CMOs > as they rely on that default value :-) . > > Can we do the following: > > (1) pick Anup's patch moving the block-size init over to cacheflush [0] > (2) apply this patch (with Conor's fixes and adapted to the changed > location) and add this one additional line: > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > index d4b1526538ad..67fa078f303f 100644 > --- a/arch/riscv/errata/thead/errata.c > +++ b/arch/riscv/errata/thead/errata.c > @@ -42,6 +42,7 @@ static bool errata_probe_cmo(unsigned int stage, > if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > return false; > > + riscv_cbom_block_size = L1_CACHE_BYTES; > riscv_noncoherent_supported(); > return true; > } > > > With that done everything works (again) and looks great, so would be > Reviewed-by: Heiko Stuebner <heiko@sntech.de> > Tested-by: Heiko Stuebner <heiko@sntech.de> With all of the above, it'd also be: Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > > > Heiko > > [0] https://lore.kernel.org/r/20220830044642.566769-3-apatel@ventanamicro.com > >
On Fri, Sep 02, 2022 at 09:55:27AM +0000, Conor.Dooley@microchip.com wrote: > On 01/09/2022 16:57, Heiko Stübner wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > Am Freitag, 12. August 2022, 17:40:10 CEST schrieb Palmer Dabbelt: > >> This fixes two issues: I truncated the warning's hart ID when porting to > >> the 64-bit hart ID code, and the original code's warning handling could > >> fire on an uninitialized hart ID. > >> > >> The biggest change here is that riscv_cbom_block_size is no longer > >> initialized, as IMO the default isn't sane: there's nothing in the ISA > >> that mandates any specific cache block size, so falling back to one will > >> just silently produce the wrong answer on some systems. This also > >> changes the probing order so the cache block size is known before > >> enabling Zicbom support. > >> > >> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant") > >> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension") > >> Reported-by: kernel test robot <lkp@intel.com> > >> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> > > > > With Conor's fixes folded in, this compiles and breaks the T-Head CMOs > > as they rely on that default value :-) . > > > > Can we do the following: > > > > (1) pick Anup's patch moving the block-size init over to cacheflush [0] > > (2) apply this patch (with Conor's fixes and adapted to the changed > > location) and add this one additional line: > > > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > > index d4b1526538ad..67fa078f303f 100644 > > --- a/arch/riscv/errata/thead/errata.c > > +++ b/arch/riscv/errata/thead/errata.c > > @@ -42,6 +42,7 @@ static bool errata_probe_cmo(unsigned int stage, > > if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > > return false; > > > > + riscv_cbom_block_size = L1_CACHE_BYTES; > > riscv_noncoherent_supported(); > > return true; > > } > > > > > > With that done everything works (again) and looks great, so would be > > Reviewed-by: Heiko Stuebner <heiko@sntech.de> > > Tested-by: Heiko Stuebner <heiko@sntech.de> > > With all of the above, it'd also be: > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> I needed these fixes for a patch series enabling KVM guests to use Zicbom so I've gone ahead and made Conor's and Heiko's changes [1]. I rebased Anup's moving patch on top of that [2]. I'll go ahead and post this patch separately in case that makes things easier. [1] https://github.com/jones-drew/linux/commit/af361283ec3129846307f787a3ebb19bd4a9c421 [2] https://github.com/jones-drew/linux/commit/61c404299c63c8706c129d2a67071f5aae94594f (I forgot Conor's r-b on this patch in the branch, but I'll pick it up now while posting.) Thanks, drew > > > > > > > Heiko > > > > [0] https://lore.kernel.org/r/20220830044642.566769-3-apatel@ventanamicro.com > > > > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c index 95ef6e2bf45c..2dfc463b86bb 100644 --- a/arch/riscv/kernel/setup.c +++ b/arch/riscv/kernel/setup.c @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p) setup_smp(); #endif - riscv_fill_hwcap(); riscv_init_cbom_blocksize(); + riscv_fill_hwcap(); apply_boot_alternatives(); } diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c index cd2225304c82..3aa3572715d6 100644 --- a/arch/riscv/mm/dma-noncoherent.c +++ b/arch/riscv/mm/dma-noncoherent.c @@ -12,7 +12,7 @@ #include <linux/of_device.h> #include <asm/cacheflush.h> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES; +static unsigned int riscv_cbom_block_size; static bool noncoherent_supported; void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void) { struct device_node *node; int ret; - u32 val; + u32 val, probed_block_size; + probed_block_size = 0; for_each_of_cpu_node(node) { - unsigned long hartid; - int cbom_hartid; + unsigned long hartid, cbom_hartid; ret = riscv_of_processor_hartid(node, &hartid); if (ret) continue; - if (hartid < 0) - continue; - /* set block-size for cbom extension if available */ ret = of_property_read_u32(node, "riscv,cbom-block-size", &val); if (ret) continue; - if (!riscv_cbom_block_size) { - riscv_cbom_block_size = val; + if (!probed_block_size) { + probed_block_size = val; cbom_hartid = hartid; } else { - if (riscv_cbom_block_size != val) + if (probed_block_size != val) pr_warn("cbom-block-size mismatched between harts %d and %lu\n", cbom_hartid, hartid); } } + + if (probed_block_size) + riscv_cbom_block_size = probed_block_size; } #endif void riscv_noncoherent_supported(void) { + WARN_ON(!riscv_cbom_block_size, + "Non-coherent DMA support enabled without a block size\n"); noncoherent_supported = true; }
This fixes two issues: I truncated the warning's hart ID when porting to the 64-bit hart ID code, and the original code's warning handling could fire on an uninitialized hart ID. The biggest change here is that riscv_cbom_block_size is no longer initialized, as IMO the default isn't sane: there's nothing in the ISA that mandates any specific cache block size, so falling back to one will just silently produce the wrong answer on some systems. This also changes the probing order so the cache block size is known before enabling Zicbom support. Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant") Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension") Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> --- Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>: * Everything but the unsigned long cbom_hartid. --- arch/riscv/kernel/setup.c | 2 +- arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++---------- 2 files changed, 13 insertions(+), 11 deletions(-)