diff mbox series

[v2,-next,3/4] RISC-V: cacheflush: Initialize CBO variables on ACPI systems

Message ID 20230927170015.295232-4-sunilvl@ventanamicro.com
State New
Headers
Series RISC-V: ACPI improvements |

Commit Message

Sunil V L Sept. 27, 2023, 5 p.m. UTC 
  Using new interface to get the CBO block size information in RHCT,
initialize the variables on ACPI platforms.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/mm/cacheflush.c | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

Comments

Conor Dooley Oct. 2, 2023, 3:50 p.m. UTC | #1 
On Wed, Sep 27, 2023 at 10:30:14PM +0530, Sunil V L wrote:
> Using new interface to get the CBO block size information in RHCT,
> initialize the variables on ACPI platforms.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>

Otherwise,
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.
> ---
>  arch/riscv/mm/cacheflush.c | 37 +++++++++++++++++++++++++++++++------
>  1 file changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> index f1387272a551..8e59644e473c 100644
> --- a/arch/riscv/mm/cacheflush.c
> +++ b/arch/riscv/mm/cacheflush.c
> @@ -3,7 +3,9 @@
>   * Copyright (C) 2017 SiFive
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/of.h>
> +#include <asm/acpi.h>
>  #include <asm/cacheflush.h>
>  
>  #ifdef CONFIG_SMP
> @@ -124,15 +126,38 @@ void __init riscv_init_cbo_blocksizes(void)
>  	unsigned long cbom_hartid, cboz_hartid;
>  	u32 cbom_block_size = 0, cboz_block_size = 0;
>  	struct device_node *node;
> +	struct acpi_table_header *rhct;
> +	acpi_status status;
> +	unsigned int cpu;
> +
> +	if (!acpi_disabled) {
> +		status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct);
> +		if (ACPI_FAILURE(status))
> +			return;
> +	}
>  
> -	for_each_of_cpu_node(node) {
> -		/* set block-size for cbom and/or cboz extension if available */
> -		cbo_get_block_size(node, "riscv,cbom-block-size",
> -				   &cbom_block_size, &cbom_hartid);
> -		cbo_get_block_size(node, "riscv,cboz-block-size",
> -				   &cboz_block_size, &cboz_hartid);
> +	for_each_possible_cpu(cpu) {
> +		if (acpi_disabled) {
> +			node = of_cpu_device_node_get(cpu);
> +			if (!node) {
> +				pr_warn("Unable to find cpu node\n");
> +				continue;
> +			}
> +
> +			/* set block-size for cbom and/or cboz extension if available */
> +			cbo_get_block_size(node, "riscv,cbom-block-size",
> +					   &cbom_block_size, &cbom_hartid);
> +			cbo_get_block_size(node, "riscv,cboz-block-size",
> +					   &cboz_block_size, &cboz_hartid);
> +		} else {
> +			acpi_get_cbo_block_size(rhct, cpu, &cbom_block_size,
> +						&cboz_block_size, NULL);
> +		}
>  	}
>  
> +	if (!acpi_disabled && rhct)
> +		acpi_put_table((struct acpi_table_header *)rhct);
> +
>  	if (cbom_block_size)
>  		riscv_cbom_block_size = cbom_block_size;
>  
> -- 
> 2.39.2
>
Samuel Holland Oct. 3, 2023, 7:50 p.m. UTC | #2 
On 2023-09-27 12:00 PM, Sunil V L wrote:
> Using new interface to get the CBO block size information in RHCT,
> initialize the variables on ACPI platforms.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  arch/riscv/mm/cacheflush.c | 37 +++++++++++++++++++++++++++++++------
>  1 file changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> index f1387272a551..8e59644e473c 100644
> --- a/arch/riscv/mm/cacheflush.c
> +++ b/arch/riscv/mm/cacheflush.c
> @@ -3,7 +3,9 @@
>   * Copyright (C) 2017 SiFive
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/of.h>
> +#include <asm/acpi.h>
>  #include <asm/cacheflush.h>
>  
>  #ifdef CONFIG_SMP
> @@ -124,15 +126,38 @@ void __init riscv_init_cbo_blocksizes(void)
>  	unsigned long cbom_hartid, cboz_hartid;
>  	u32 cbom_block_size = 0, cboz_block_size = 0;
>  	struct device_node *node;
> +	struct acpi_table_header *rhct;
> +	acpi_status status;
> +	unsigned int cpu;
> +
> +	if (!acpi_disabled) {
> +		status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct);
> +		if (ACPI_FAILURE(status))
> +			return;
> +	}
>  
> -	for_each_of_cpu_node(node) {
> -		/* set block-size for cbom and/or cboz extension if available */
> -		cbo_get_block_size(node, "riscv,cbom-block-size",
> -				   &cbom_block_size, &cbom_hartid);
> -		cbo_get_block_size(node, "riscv,cboz-block-size",
> -				   &cboz_block_size, &cboz_hartid);
> +	for_each_possible_cpu(cpu) {
> +		if (acpi_disabled) {
> +			node = of_cpu_device_node_get(cpu);
> +			if (!node) {
> +				pr_warn("Unable to find cpu node\n");
> +				continue;
> +			}
> +
> +			/* set block-size for cbom and/or cboz extension if available */
> +			cbo_get_block_size(node, "riscv,cbom-block-size",
> +					   &cbom_block_size, &cbom_hartid);
> +			cbo_get_block_size(node, "riscv,cboz-block-size",
> +					   &cboz_block_size, &cboz_hartid);

This leaks a reference to the device node.

> +		} else {
> +			acpi_get_cbo_block_size(rhct, cpu, &cbom_block_size,
> +						&cboz_block_size, NULL);

This function loops through the whole RHCT already. Why do we need to call it
for each CPU? Can't we just call it once, and have it do the same consistency
checks as cbo_get_block_size()?

In that case, the DT path could keep the for_each_of_cpu_node() loop.

Regards,
Samuel

> +		}
>  	}
>  
> +	if (!acpi_disabled && rhct)
> +		acpi_put_table((struct acpi_table_header *)rhct);
> +
>  	if (cbom_block_size)
>  		riscv_cbom_block_size = cbom_block_size;
>
Sunil V L Oct. 4, 2023, 4:22 a.m. UTC | #3 
On Tue, Oct 03, 2023 at 02:50:02PM -0500, Samuel Holland wrote:
> On 2023-09-27 12:00 PM, Sunil V L wrote:
> > Using new interface to get the CBO block size information in RHCT,
> > initialize the variables on ACPI platforms.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  arch/riscv/mm/cacheflush.c | 37 +++++++++++++++++++++++++++++++------
> >  1 file changed, 31 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> > index f1387272a551..8e59644e473c 100644
> > --- a/arch/riscv/mm/cacheflush.c
> > +++ b/arch/riscv/mm/cacheflush.c
> > @@ -3,7 +3,9 @@
> >   * Copyright (C) 2017 SiFive
> >   */
> >  
> > +#include <linux/acpi.h>
> >  #include <linux/of.h>
> > +#include <asm/acpi.h>
> >  #include <asm/cacheflush.h>
> >  
> >  #ifdef CONFIG_SMP
> > @@ -124,15 +126,38 @@ void __init riscv_init_cbo_blocksizes(void)
> >  	unsigned long cbom_hartid, cboz_hartid;
> >  	u32 cbom_block_size = 0, cboz_block_size = 0;
> >  	struct device_node *node;
> > +	struct acpi_table_header *rhct;
> > +	acpi_status status;
> > +	unsigned int cpu;
> > +
> > +	if (!acpi_disabled) {
> > +		status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct);
> > +		if (ACPI_FAILURE(status))
> > +			return;
> > +	}
> >  
> > -	for_each_of_cpu_node(node) {
> > -		/* set block-size for cbom and/or cboz extension if available */
> > -		cbo_get_block_size(node, "riscv,cbom-block-size",
> > -				   &cbom_block_size, &cbom_hartid);
> > -		cbo_get_block_size(node, "riscv,cboz-block-size",
> > -				   &cboz_block_size, &cboz_hartid);
> > +	for_each_possible_cpu(cpu) {
> > +		if (acpi_disabled) {
> > +			node = of_cpu_device_node_get(cpu);
> > +			if (!node) {
> > +				pr_warn("Unable to find cpu node\n");
> > +				continue;
> > +			}
> > +
> > +			/* set block-size for cbom and/or cboz extension if available */
> > +			cbo_get_block_size(node, "riscv,cbom-block-size",
> > +					   &cbom_block_size, &cbom_hartid);
> > +			cbo_get_block_size(node, "riscv,cboz-block-size",
> > +					   &cboz_block_size, &cboz_hartid);
> 
> This leaks a reference to the device node.
> 
Yep!. I missed of_node_put(). Let me add in next revision. Thanks!

> > +		} else {
> > +			acpi_get_cbo_block_size(rhct, cpu, &cbom_block_size,
> > +						&cboz_block_size, NULL);
> 
> This function loops through the whole RHCT already. Why do we need to call it
> for each CPU? Can't we just call it once, and have it do the same consistency
> checks as cbo_get_block_size()?
> 
> In that case, the DT path could keep the for_each_of_cpu_node() loop.
> 
I kept the same logic as DT. Basically, by passing the cpu node, we
will fetch the exact CPU's CBO property from RHCT. It is not clear to me
why we overwrite the same variable with value from another cpu and
whether we can return as soon as we get the CBO size for one CPU.

Drew, can we exit the loop if we get the CBO size for one CPU?

Thanks!
Sunil
Andrew Jones Oct. 4, 2023, 8:33 a.m. UTC | #4 
On Wed, Oct 04, 2023 at 09:52:23AM +0530, Sunil V L wrote:
> On Tue, Oct 03, 2023 at 02:50:02PM -0500, Samuel Holland wrote:
> > On 2023-09-27 12:00 PM, Sunil V L wrote:
> > > Using new interface to get the CBO block size information in RHCT,
> > > initialize the variables on ACPI platforms.
> > > 
> > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > > ---
> > >  arch/riscv/mm/cacheflush.c | 37 +++++++++++++++++++++++++++++++------
> > >  1 file changed, 31 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> > > index f1387272a551..8e59644e473c 100644
> > > --- a/arch/riscv/mm/cacheflush.c
> > > +++ b/arch/riscv/mm/cacheflush.c
> > > @@ -3,7 +3,9 @@
> > >   * Copyright (C) 2017 SiFive
> > >   */
> > >  
> > > +#include <linux/acpi.h>
> > >  #include <linux/of.h>
> > > +#include <asm/acpi.h>
> > >  #include <asm/cacheflush.h>
> > >  
> > >  #ifdef CONFIG_SMP
> > > @@ -124,15 +126,38 @@ void __init riscv_init_cbo_blocksizes(void)
> > >  	unsigned long cbom_hartid, cboz_hartid;
> > >  	u32 cbom_block_size = 0, cboz_block_size = 0;
> > >  	struct device_node *node;
> > > +	struct acpi_table_header *rhct;
> > > +	acpi_status status;
> > > +	unsigned int cpu;
> > > +
> > > +	if (!acpi_disabled) {
> > > +		status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct);
> > > +		if (ACPI_FAILURE(status))
> > > +			return;
> > > +	}
> > >  
> > > -	for_each_of_cpu_node(node) {
> > > -		/* set block-size for cbom and/or cboz extension if available */
> > > -		cbo_get_block_size(node, "riscv,cbom-block-size",
> > > -				   &cbom_block_size, &cbom_hartid);
> > > -		cbo_get_block_size(node, "riscv,cboz-block-size",
> > > -				   &cboz_block_size, &cboz_hartid);
> > > +	for_each_possible_cpu(cpu) {
> > > +		if (acpi_disabled) {
> > > +			node = of_cpu_device_node_get(cpu);
> > > +			if (!node) {
> > > +				pr_warn("Unable to find cpu node\n");
> > > +				continue;
> > > +			}
> > > +
> > > +			/* set block-size for cbom and/or cboz extension if available */
> > > +			cbo_get_block_size(node, "riscv,cbom-block-size",
> > > +					   &cbom_block_size, &cbom_hartid);
> > > +			cbo_get_block_size(node, "riscv,cboz-block-size",
> > > +					   &cboz_block_size, &cboz_hartid);
> > 
> > This leaks a reference to the device node.
> > 
> Yep!. I missed of_node_put(). Let me add in next revision. Thanks!
> 
> > > +		} else {
> > > +			acpi_get_cbo_block_size(rhct, cpu, &cbom_block_size,
> > > +						&cboz_block_size, NULL);
> > 
> > This function loops through the whole RHCT already. Why do we need to call it
> > for each CPU? Can't we just call it once, and have it do the same consistency
> > checks as cbo_get_block_size()?
> > 
> > In that case, the DT path could keep the for_each_of_cpu_node() loop.
> > 
> I kept the same logic as DT. Basically, by passing the cpu node, we
> will fetch the exact CPU's CBO property from RHCT. It is not clear to me
> why we overwrite the same variable with value from another cpu and
> whether we can return as soon as we get the CBO size for one CPU.
> 
> Drew, can we exit the loop if we get the CBO size for one CPU?

We want to compare the values for each CPU with the first one we find in
order to ensure they are consistent. I think Samuel is suggesting that
we leave the DT path here the same, i.e. keep the for_each_of_cpu_node()
loop, and then change acpi_get_cbo_block_size() to *not* take a cpu as
input, but rather follow the same pattern as DT, which is to loop over
all cpus doing a consistency check against the first cpu's CBO info.

Thanks,
drew
Sunil V L Oct. 4, 2023, 10:13 a.m. UTC | #5 
On Wed, Oct 04, 2023 at 10:33:31AM +0200, Andrew Jones wrote:
> On Wed, Oct 04, 2023 at 09:52:23AM +0530, Sunil V L wrote:
> > On Tue, Oct 03, 2023 at 02:50:02PM -0500, Samuel Holland wrote:
> > > On 2023-09-27 12:00 PM, Sunil V L wrote:
> > > > Using new interface to get the CBO block size information in RHCT,
> > > > initialize the variables on ACPI platforms.
> > > > 
> > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > > > ---
> > > >  arch/riscv/mm/cacheflush.c | 37 +++++++++++++++++++++++++++++++------
> > > >  1 file changed, 31 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> > > > index f1387272a551..8e59644e473c 100644
> > > > --- a/arch/riscv/mm/cacheflush.c
> > > > +++ b/arch/riscv/mm/cacheflush.c
> > > > @@ -3,7 +3,9 @@
> > > >   * Copyright (C) 2017 SiFive
> > > >   */
> > > >  
> > > > +#include <linux/acpi.h>
> > > >  #include <linux/of.h>
> > > > +#include <asm/acpi.h>
> > > >  #include <asm/cacheflush.h>
> > > >  
> > > >  #ifdef CONFIG_SMP
> > > > @@ -124,15 +126,38 @@ void __init riscv_init_cbo_blocksizes(void)
> > > >  	unsigned long cbom_hartid, cboz_hartid;
> > > >  	u32 cbom_block_size = 0, cboz_block_size = 0;
> > > >  	struct device_node *node;
> > > > +	struct acpi_table_header *rhct;
> > > > +	acpi_status status;
> > > > +	unsigned int cpu;
> > > > +
> > > > +	if (!acpi_disabled) {
> > > > +		status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct);
> > > > +		if (ACPI_FAILURE(status))
> > > > +			return;
> > > > +	}
> > > >  
> > > > -	for_each_of_cpu_node(node) {
> > > > -		/* set block-size for cbom and/or cboz extension if available */
> > > > -		cbo_get_block_size(node, "riscv,cbom-block-size",
> > > > -				   &cbom_block_size, &cbom_hartid);
> > > > -		cbo_get_block_size(node, "riscv,cboz-block-size",
> > > > -				   &cboz_block_size, &cboz_hartid);
> > > > +	for_each_possible_cpu(cpu) {
> > > > +		if (acpi_disabled) {
> > > > +			node = of_cpu_device_node_get(cpu);
> > > > +			if (!node) {
> > > > +				pr_warn("Unable to find cpu node\n");
> > > > +				continue;
> > > > +			}
> > > > +
> > > > +			/* set block-size for cbom and/or cboz extension if available */
> > > > +			cbo_get_block_size(node, "riscv,cbom-block-size",
> > > > +					   &cbom_block_size, &cbom_hartid);
> > > > +			cbo_get_block_size(node, "riscv,cboz-block-size",
> > > > +					   &cboz_block_size, &cboz_hartid);
> > > 
> > > This leaks a reference to the device node.
> > > 
> > Yep!. I missed of_node_put(). Let me add in next revision. Thanks!
> > 
> > > > +		} else {
> > > > +			acpi_get_cbo_block_size(rhct, cpu, &cbom_block_size,
> > > > +						&cboz_block_size, NULL);
> > > 
> > > This function loops through the whole RHCT already. Why do we need to call it
> > > for each CPU? Can't we just call it once, and have it do the same consistency
> > > checks as cbo_get_block_size()?
> > > 
> > > In that case, the DT path could keep the for_each_of_cpu_node() loop.
> > > 
> > I kept the same logic as DT. Basically, by passing the cpu node, we
> > will fetch the exact CPU's CBO property from RHCT. It is not clear to me
> > why we overwrite the same variable with value from another cpu and
> > whether we can return as soon as we get the CBO size for one CPU.
> > 
> > Drew, can we exit the loop if we get the CBO size for one CPU?
> 
> We want to compare the values for each CPU with the first one we find in
> order to ensure they are consistent. I think Samuel is suggesting that
> we leave the DT path here the same, i.e. keep the for_each_of_cpu_node()
> loop, and then change acpi_get_cbo_block_size() to *not* take a cpu as
> input, but rather follow the same pattern as DT, which is to loop over
> all cpus doing a consistency check against the first cpu's CBO info.
> 
Ahh OK. Thanks Drew and Samuel. Let me update as you suggested.

Thanks!
Sunil
diff mbox series

Patch

diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
index f1387272a551..8e59644e473c 100644
--- a/arch/riscv/mm/cacheflush.c
+++ b/arch/riscv/mm/cacheflush.c
@@ -3,7 +3,9 @@ 
  * Copyright (C) 2017 SiFive
  */
 
+#include <linux/acpi.h>
 #include <linux/of.h>
+#include <asm/acpi.h>
 #include <asm/cacheflush.h>
 
 #ifdef CONFIG_SMP
@@ -124,15 +126,38 @@  void __init riscv_init_cbo_blocksizes(void)
 	unsigned long cbom_hartid, cboz_hartid;
 	u32 cbom_block_size = 0, cboz_block_size = 0;
 	struct device_node *node;
+	struct acpi_table_header *rhct;
+	acpi_status status;
+	unsigned int cpu;
+
+	if (!acpi_disabled) {
+		status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct);
+		if (ACPI_FAILURE(status))
+			return;
+	}
 
-	for_each_of_cpu_node(node) {
-		/* set block-size for cbom and/or cboz extension if available */
-		cbo_get_block_size(node, "riscv,cbom-block-size",
-				   &cbom_block_size, &cbom_hartid);
-		cbo_get_block_size(node, "riscv,cboz-block-size",
-				   &cboz_block_size, &cboz_hartid);
+	for_each_possible_cpu(cpu) {
+		if (acpi_disabled) {
+			node = of_cpu_device_node_get(cpu);
+			if (!node) {
+				pr_warn("Unable to find cpu node\n");
+				continue;
+			}
+
+			/* set block-size for cbom and/or cboz extension if available */
+			cbo_get_block_size(node, "riscv,cbom-block-size",
+					   &cbom_block_size, &cbom_hartid);
+			cbo_get_block_size(node, "riscv,cboz-block-size",
+					   &cboz_block_size, &cboz_hartid);
+		} else {
+			acpi_get_cbo_block_size(rhct, cpu, &cbom_block_size,
+						&cboz_block_size, NULL);
+		}
 	}
 
+	if (!acpi_disabled && rhct)
+		acpi_put_table((struct acpi_table_header *)rhct);
+
 	if (cbom_block_size)
 		riscv_cbom_block_size = cbom_block_size;