Revert "riscv: Set more data to cacheinfo"

  This reverts commit baf7cbd94b5688f167443a2cc3dcea3300132099.

There are some duplicate cache attributes populations executed
in both ci_leaf_init() and later cache_setup_properties().

Revert the commit baf7cbd94b56 ("riscv: Set more data to cacheinfo")
to setup only the level and type attributes at this early place.

Signed-off-by: Song Shuai <suagrfillet@gmail.com>
---
 arch/riscv/kernel/cacheinfo.c | 66 ++++++++---------------------------
 1 file changed, 15 insertions(+), 51 deletions(-)
  

On Wed, Mar 08, 2023 at 02:47:34PM +0800, Song Shuai wrote:
> This reverts commit baf7cbd94b5688f167443a2cc3dcea3300132099.
> 
> There are some duplicate cache attributes populations executed
> in both ci_leaf_init() and later cache_setup_properties().
> 
> Revert the commit baf7cbd94b56 ("riscv: Set more data to cacheinfo")
> to setup only the level and type attributes at this early place.
>

I've attempted to do a little bit of history diving to figure out what
commit to "blame" in a fixes tag for this.
My initial thought was to blame 03f11f03dbfe ("RISC-V: Parse cpu topology
during boot."), but baf7cbd94b56 ("riscv: Set more data to cacheinfo")
looks like it came along some months later.
I assume you double checked that the stuff in the aux vector is the same
before/after this revert? Looking on lore, that seems to be why Zong did
it this way in the first place:
https://lore.kernel.org/linux-riscv/b6fed9b4c2a3eacb2a9c353c2570d9dc1d0c1c88.1598859038.git.zong.li@sifive.com/

Cheers,
Conor.

> Signed-off-by: Song Shuai <suagrfillet@gmail.com>
> ---
>  arch/riscv/kernel/cacheinfo.c | 66 ++++++++---------------------------
>  1 file changed, 15 insertions(+), 51 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
> index 3a13113f1b29..305ebbdc780d 100644
> --- a/arch/riscv/kernel/cacheinfo.c
> +++ b/arch/riscv/kernel/cacheinfo.c
> @@ -64,53 +64,12 @@ uintptr_t get_cache_geometry(u32 level, enum cache_type type)
>  			   0;
>  }
>  
> -static void ci_leaf_init(struct cacheinfo *this_leaf, enum cache_type type,
> -			 unsigned int level, unsigned int size,
> -			 unsigned int sets, unsigned int line_size)
> +static void ci_leaf_init(struct cacheinfo *this_leaf,
> +			 struct device_node *node,
> +			 enum cache_type type, unsigned int level)
>  {
>  	this_leaf->level = level;
>  	this_leaf->type = type;
> -	this_leaf->size = size;
> -	this_leaf->number_of_sets = sets;
> -	this_leaf->coherency_line_size = line_size;
> -
> -	/*
> -	 * If the cache is fully associative, there is no need to
> -	 * check the other properties.
> -	 */
> -	if (sets == 1)
> -		return;
> -
> -	/*
> -	 * Set the ways number for n-ways associative, make sure
> -	 * all properties are big than zero.
> -	 */
> -	if (sets > 0 && size > 0 && line_size > 0)
> -		this_leaf->ways_of_associativity = (size / sets) / line_size;
> -}
> -
> -static void fill_cacheinfo(struct cacheinfo **this_leaf,
> -			   struct device_node *node, unsigned int level)
> -{
> -	unsigned int size, sets, line_size;
> -
> -	if (!of_property_read_u32(node, "cache-size", &size) &&
> -	    !of_property_read_u32(node, "cache-block-size", &line_size) &&
> -	    !of_property_read_u32(node, "cache-sets", &sets)) {
> -		ci_leaf_init((*this_leaf)++, CACHE_TYPE_UNIFIED, level, size, sets, line_size);
> -	}
> -
> -	if (!of_property_read_u32(node, "i-cache-size", &size) &&
> -	    !of_property_read_u32(node, "i-cache-sets", &sets) &&
> -	    !of_property_read_u32(node, "i-cache-block-size", &line_size)) {
> -		ci_leaf_init((*this_leaf)++, CACHE_TYPE_INST, level, size, sets, line_size);
> -	}
> -
> -	if (!of_property_read_u32(node, "d-cache-size", &size) &&
> -	    !of_property_read_u32(node, "d-cache-sets", &sets) &&
> -	    !of_property_read_u32(node, "d-cache-block-size", &line_size)) {
> -		ci_leaf_init((*this_leaf)++, CACHE_TYPE_DATA, level, size, sets, line_size);
> -	}
>  }
>  
>  int populate_cache_leaves(unsigned int cpu)
> @@ -121,24 +80,29 @@ int populate_cache_leaves(unsigned int cpu)
>  	struct device_node *prev = NULL;
>  	int levels = 1, level = 1;
>  
> -	/* Level 1 caches in cpu node */
> -	fill_cacheinfo(&this_leaf, np, level);
> +	if (of_property_read_bool(np, "cache-size"))
> +		ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level);
> +	if (of_property_read_bool(np, "i-cache-size"))
> +		ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level);
> +	if (of_property_read_bool(np, "d-cache-size"))
> +		ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level);
>  
> -	/* Next level caches in cache nodes */
>  	prev = np;
>  	while ((np = of_find_next_cache_node(np))) {
>  		of_node_put(prev);
>  		prev = np;
> -
>  		if (!of_device_is_compatible(np, "cache"))
>  			break;
>  		if (of_property_read_u32(np, "cache-level", &level))
>  			break;
>  		if (level <= levels)
>  			break;
> -
> -		fill_cacheinfo(&this_leaf, np, level);
> -
> +		if (of_property_read_bool(np, "cache-size"))
> +			ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level);
> +		if (of_property_read_bool(np, "i-cache-size"))
> +			ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level);
> +		if (of_property_read_bool(np, "d-cache-size"))
> +			ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level);
>  		levels = level;
>  	}
>  	of_node_put(np);
> -- 
> 2.20.1
>
  

On Wed, Mar 08, 2023 at 02:47:34PM +0800, Song Shuai wrote:
> This reverts commit baf7cbd94b5688f167443a2cc3dcea3300132099.
> 
> There are some duplicate cache attributes populations executed
> in both ci_leaf_init() and later cache_setup_properties().
> 
> Revert the commit baf7cbd94b56 ("riscv: Set more data to cacheinfo")
> to setup only the level and type attributes at this early place.
>

I had noticed the same and had something similar when we did some rework
around for v6.1 merge window. But there were lot of other issues to be
addressed at the moment and hence deferred this.

So for the change in general, but as Conor responded, it would be good
to do some checking to ensure nothing breaks and all the requirements
this patch(baf7cbd94b56) addressed are already handled in the core.

Acked-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep
  

Hi, Conor & Sudeep :

Sudeep Holla <sudeep.holla@arm.com> 于2023年3月8日周三 11:35写道：
>
> On Wed, Mar 08, 2023 at 02:47:34PM +0800, Song Shuai wrote:
> > This reverts commit baf7cbd94b5688f167443a2cc3dcea3300132099.
> >
> > There are some duplicate cache attributes populations executed
> > in both ci_leaf_init() and later cache_setup_properties().
> >
> > Revert the commit baf7cbd94b56 ("riscv: Set more data to cacheinfo")
> > to setup only the level and type attributes at this early place.
> >
>
> I had noticed the same and had something similar when we did some rework
> around for v6.1 merge window. But there were lot of other issues to be
> addressed at the moment and hence deferred this.
>
> So for the change in general, but as Conor responded, it would be good
> to do some checking to ensure nothing breaks and all the requirements
> this patch(baf7cbd94b56) addressed are already handled in the core.

As you suggested, commit (da29dbcda49d "riscv: Add cache information
in AUX vector")
in the "Get cache information from userland" series should be checked.

The commit da29dbcda49d adds the cacheinfo (read from
ci_cacheinfo(cpu)) in ELF auxiliary vectors,
so process can fetch the cacheinfo through glibc sysconf() after ELF loading.
At the same time, the glibc related support was enabled by its commit
(15b38ffc10 "riscv: Get cache information through sysconf")

With this reverting patch applied, the output of `getconf -a` looks good
in Qemu sifive_u machine and rootfs image with glibc-2.35.

```
LEVEL1_ICACHE_SIZE 32768
LEVEL1_ICACHE_ASSOC 8
LEVEL1_ICACHE_LINESIZE 64
LEVEL1_DCACHE_SIZE 32768
LEVEL1_DCACHE_ASSOC 8
LEVEL1_DCACHE_LINESIZE 64
LEVEL2_CACHE_SIZE 2097152
LEVEL2_CACHE_ASSOC 32
LEVEL2_CACHE_LINESIZE 64
```

>
> Acked-by: Sudeep Holla <sudeep.holla@arm.com>
>
> --
> Regards,
> Sudeep
  

On Thu, Mar 09, 2023 at 07:54:45AM +0000, Song Shuai wrote:
> Hi, Conor & Sudeep :
> 
> Sudeep Holla <sudeep.holla@arm.com> 于2023年3月8日周三 11:35写道：
> >
> > On Wed, Mar 08, 2023 at 02:47:34PM +0800, Song Shuai wrote:
> > > This reverts commit baf7cbd94b5688f167443a2cc3dcea3300132099.
> > >
> > > There are some duplicate cache attributes populations executed
> > > in both ci_leaf_init() and later cache_setup_properties().
> > >
> > > Revert the commit baf7cbd94b56 ("riscv: Set more data to cacheinfo")
> > > to setup only the level and type attributes at this early place.
> > >
> >
> > I had noticed the same and had something similar when we did some rework
> > around for v6.1 merge window. But there were lot of other issues to be
> > addressed at the moment and hence deferred this.
> >
> > So for the change in general, but as Conor responded, it would be good
> > to do some checking to ensure nothing breaks and all the requirements
> > this patch(baf7cbd94b56) addressed are already handled in the core.
> 
> As you suggested, commit (da29dbcda49d "riscv: Add cache information
> in AUX vector")
> in the "Get cache information from userland" series should be checked.
> 
> The commit da29dbcda49d adds the cacheinfo (read from
> ci_cacheinfo(cpu)) in ELF auxiliary vectors,
> so process can fetch the cacheinfo through glibc sysconf() after ELF loading.
> At the same time, the glibc related support was enabled by its commit
> (15b38ffc10 "riscv: Get cache information through sysconf")
> 
> With this reverting patch applied, the output of `getconf -a` looks good
> in Qemu sifive_u machine and rootfs image with glibc-2.35.

If, as I think you're implying, it is unchanged before/after:
Acked-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.
  

Hello:

This patch was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Wed,  8 Mar 2023 14:47:34 +0800 you wrote:
> This reverts commit baf7cbd94b5688f167443a2cc3dcea3300132099.
> 
> There are some duplicate cache attributes populations executed
> in both ci_leaf_init() and later cache_setup_properties().
> 
> Revert the commit baf7cbd94b56 ("riscv: Set more data to cacheinfo")
> to setup only the level and type attributes at this early place.
> 
> [...]

Here is the summary with links:
  - Revert "riscv: Set more data to cacheinfo"
    https://git.kernel.org/riscv/c/6a24915145c9

You are awesome, thank you!