diff mbox series

[v3,3/5] ACPI/PPTT: Modify node flag detection to find last IDENTICAL

Message ID 20190503232407.37195-4-jeremy.linton@arm.com
State New
Headers show
Series arm64: SPE ACPI enablement | expand

Commit Message

Jeremy Linton May 3, 2019, 11:24 p.m. UTC 
The ACPI specification implies that the IDENTICAL flag should be
set on all non leaf nodes where the children are identical.
This means that we need to be searching for the last node with
the identical flag set rather than the first one.

Since this flag is also dependent on the table revision, we
need to add a bit of extra code to verify the table revision,
and the next node's state in the traversal. Since we want to
avoid function pointers here, lets just special case
the IDENTICAL flag.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

---
 drivers/acpi/pptt.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

-- 
2.21.0

Comments

Sudeep Holla June 7, 2019, 9:53 a.m. UTC | #1 
On Fri, May 03, 2019 at 06:24:05PM -0500, Jeremy Linton wrote:
> The ACPI specification implies that the IDENTICAL flag should be

> set on all non leaf nodes where the children are identical.

> This means that we need to be searching for the last node with

> the identical flag set rather than the first one.

> 

> Since this flag is also dependent on the table revision, we

> need to add a bit of extra code to verify the table revision,

> and the next node's state in the traversal. Since we want to

> avoid function pointers here, lets just special case

> the IDENTICAL flag.

> 

> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

> ---

>  drivers/acpi/pptt.c | 28 +++++++++++++++++++++++++---

>  1 file changed, 25 insertions(+), 3 deletions(-)

> 

> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c

> index 1865515297ca..456e1c0a35ae 100644

> --- a/drivers/acpi/pptt.c

> +++ b/drivers/acpi/pptt.c

> @@ -432,17 +432,39 @@ static void cache_setup_acpi_cpu(struct acpi_table_header *table,

>  	}

>  }

>  

> +static bool flag_identical(struct acpi_table_header *table_hdr,

> +			  struct acpi_pptt_processor *cpu)


Not sure if it's email client problem, but I see quite a few mis-alignment
with parenthesis like above one.

> +{

> +	struct acpi_pptt_processor *next;

> +

> +	/* heterogeneous machines must use PPTT revision > 1 */

> +	if (table_hdr->revision < 2)

> +		return false;

> +

> +	/* Locate the last node in the tree with IDENTICAL set */

> +	if (cpu->flags & ACPI_PPTT_ACPI_IDENTICAL) {

> +		next = fetch_pptt_node(table_hdr, cpu->parent);

> +		if (!(next && next->flags & ACPI_PPTT_ACPI_IDENTICAL))

> +			return true;

> +	}

> +

> +	return false;

> +}

> +

>  /* Passing level values greater than this will result in search termination */

>  #define PPTT_ABORT_PACKAGE 0xFF

>  

> -static struct acpi_pptt_processor *acpi_find_processor_package_id(struct acpi_table_header *table_hdr,

> +static struct acpi_pptt_processor *acpi_find_processor_tag_id(struct acpi_table_header *table_hdr,

>  								  struct acpi_pptt_processor *cpu,

>  								  int level, int flag)

>  {

>  	struct acpi_pptt_processor *prev_node;

>  

>  	while (cpu && level) {

> -		if (cpu->flags & flag)

> +		if (flag == ACPI_PPTT_ACPI_IDENTICAL) {


flag_identical anyways check the flag, so I assume you can drop the above
check.

> +			if (flag_identical(table_hdr, cpu))

> +				break;

> +		} else if (cpu->flags & flag)

>  			break;

>  		pr_debug("level %d\n", level);

>  		prev_node = fetch_pptt_node(table_hdr, cpu->parent);

> @@ -480,7 +502,7 @@ static int topology_get_acpi_cpu_tag(struct acpi_table_header *table,

>  

>  	cpu_node = acpi_find_processor_node(table, acpi_cpu_id);

>  	if (cpu_node) {

> -		cpu_node = acpi_find_processor_package_id(table, cpu_node,

> +		cpu_node = acpi_find_processor_tag_id(table, cpu_node,

>  							  level, flag);



Again misaligned, may be that's because of renaming.

--
Regards,
Sudeep
Jeremy Linton June 7, 2019, 1:15 p.m. UTC | #2 
Hi,

Thanks for taking a look at this.

On 6/7/19 4:53 AM, Sudeep Holla wrote:
> On Fri, May 03, 2019 at 06:24:05PM -0500, Jeremy Linton wrote:

>> The ACPI specification implies that the IDENTICAL flag should be

>> set on all non leaf nodes where the children are identical.

>> This means that we need to be searching for the last node with

>> the identical flag set rather than the first one.

>>

>> Since this flag is also dependent on the table revision, we

>> need to add a bit of extra code to verify the table revision,

>> and the next node's state in the traversal. Since we want to

>> avoid function pointers here, lets just special case

>> the IDENTICAL flag.

>>

>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

>> ---

>>   drivers/acpi/pptt.c | 28 +++++++++++++++++++++++++---

>>   1 file changed, 25 insertions(+), 3 deletions(-)

>>

>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c

>> index 1865515297ca..456e1c0a35ae 100644

>> --- a/drivers/acpi/pptt.c

>> +++ b/drivers/acpi/pptt.c

>> @@ -432,17 +432,39 @@ static void cache_setup_acpi_cpu(struct acpi_table_header *table,

>>   	}

>>   }

>>   

>> +static bool flag_identical(struct acpi_table_header *table_hdr,

>> +			  struct acpi_pptt_processor *cpu)

> 

> Not sure if it's email client problem, but I see quite a few mis-alignment

> with parenthesis like above one.


It looks fine in the original editor/text patch, but yes in my email 
client I see it off by one (or two/three now that i'm replying). Its a 
mix of tabs/spaces and I've seen this happen before in my email client 
due to the leading "[>+]"?


> 

>> +{

>> +	struct acpi_pptt_processor *next;

>> +

>> +	/* heterogeneous machines must use PPTT revision > 1 */

>> +	if (table_hdr->revision < 2)

>> +		return false;

>> +

>> +	/* Locate the last node in the tree with IDENTICAL set */

>> +	if (cpu->flags & ACPI_PPTT_ACPI_IDENTICAL) {

>> +		next = fetch_pptt_node(table_hdr, cpu->parent);

>> +		if (!(next && next->flags & ACPI_PPTT_ACPI_IDENTICAL))

>> +			return true;

>> +	}

>> +

>> +	return false;

>> +}

>> +

>>   /* Passing level values greater than this will result in search termination */

>>   #define PPTT_ABORT_PACKAGE 0xFF

>>   

>> -static struct acpi_pptt_processor *acpi_find_processor_package_id(struct acpi_table_header *table_hdr,

>> +static struct acpi_pptt_processor *acpi_find_processor_tag_id(struct acpi_table_header *table_hdr,

>>   								  struct acpi_pptt_processor *cpu,

>>   								  int level, int flag)

>>   {

>>   	struct acpi_pptt_processor *prev_node;

>>   

>>   	while (cpu && level) {

>> -		if (cpu->flags & flag)

>> +		if (flag == ACPI_PPTT_ACPI_IDENTICAL) {

> 

> flag_identical anyways check the flag, so I assume you can drop the above

> check.


? I think that would be a bug because then we would always be returning 
the value of the IDENTICAL flag rather than the other flags passed into 
this routine. This is the special case I think Raphael was asking for 
rather than the function pointer/callback interface.

> 

>> +			if (flag_identical(table_hdr, cpu))

>> +				break;

>> +		} else if (cpu->flags & flag)

>>   			break;

>>   		pr_debug("level %d\n", level);

>>   		prev_node = fetch_pptt_node(table_hdr, cpu->parent);

>> @@ -480,7 +502,7 @@ static int topology_get_acpi_cpu_tag(struct acpi_table_header *table,

>>   

>>   	cpu_node = acpi_find_processor_node(table, acpi_cpu_id);

>>   	if (cpu_node) {

>> -		cpu_node = acpi_find_processor_package_id(table, cpu_node,

>> +		cpu_node = acpi_find_processor_tag_id(table, cpu_node,

>>   							  level, flag);

> 

> 

> Again misaligned, may be that's because of renaming.

> 

> --

> Regards,

> Sudeep

>
Sudeep Holla June 7, 2019, 1:47 p.m. UTC | #3 
On Fri, Jun 07, 2019 at 08:15:50AM -0500, Jeremy Linton wrote:
> Hi,

> 

> Thanks for taking a look at this.

> 

> On 6/7/19 4:53 AM, Sudeep Holla wrote:

> > On Fri, May 03, 2019 at 06:24:05PM -0500, Jeremy Linton wrote:

> > > The ACPI specification implies that the IDENTICAL flag should be

> > > set on all non leaf nodes where the children are identical.

> > > This means that we need to be searching for the last node with

> > > the identical flag set rather than the first one.

> > > 

> > > Since this flag is also dependent on the table revision, we

> > > need to add a bit of extra code to verify the table revision,

> > > and the next node's state in the traversal. Since we want to

> > > avoid function pointers here, lets just special case

> > > the IDENTICAL flag.

> > > 

> > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

> > > ---

> > >   drivers/acpi/pptt.c | 28 +++++++++++++++++++++++++---

> > >   1 file changed, 25 insertions(+), 3 deletions(-)

> > > 

> > > diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c

> > > index 1865515297ca..456e1c0a35ae 100644

> > > --- a/drivers/acpi/pptt.c

> > > +++ b/drivers/acpi/pptt.c

> > > @@ -432,17 +432,39 @@ static void cache_setup_acpi_cpu(struct acpi_table_header *table,

> > >   	}

> > >   }

> > > +static bool flag_identical(struct acpi_table_header *table_hdr,

> > > +			  struct acpi_pptt_processor *cpu)

> > 

> > Not sure if it's email client problem, but I see quite a few mis-alignment

> > with parenthesis like above one.

> 

> It looks fine in the original editor/text patch, but yes in my email client

> I see it off by one (or two/three now that i'm replying). Its a mix of

> tabs/spaces and I've seen this happen before in my email client due to the

> leading "[>+]"?

>


No I have configured(hopefully correctly) my client, but if you not seeing
issue with patch, that's fine.

> 

> > 

> > > +{

> > > +	struct acpi_pptt_processor *next;

> > > +

> > > +	/* heterogeneous machines must use PPTT revision > 1 */

> > > +	if (table_hdr->revision < 2)

> > > +		return false;

> > > +

> > > +	/* Locate the last node in the tree with IDENTICAL set */

> > > +	if (cpu->flags & ACPI_PPTT_ACPI_IDENTICAL) {

> > > +		next = fetch_pptt_node(table_hdr, cpu->parent);

> > > +		if (!(next && next->flags & ACPI_PPTT_ACPI_IDENTICAL))

> > > +			return true;

> > > +	}

> > > +

> > > +	return false;

> > > +}

> > > +

> > >   /* Passing level values greater than this will result in search termination */

> > >   #define PPTT_ABORT_PACKAGE 0xFF

> > > -static struct acpi_pptt_processor *acpi_find_processor_package_id(struct acpi_table_header *table_hdr,

> > > +static struct acpi_pptt_processor *acpi_find_processor_tag_id(struct acpi_table_header *table_hdr,

> > >   								  struct acpi_pptt_processor *cpu,

> > >   								  int level, int flag)

> > >   {

> > >   	struct acpi_pptt_processor *prev_node;

> > >   	while (cpu && level) {

> > > -		if (cpu->flags & flag)

> > > +		if (flag == ACPI_PPTT_ACPI_IDENTICAL) {

> > 

> > flag_identical anyways check the flag, so I assume you can drop the above

> > check.

> 

> ? I think that would be a bug because then we would always be returning the

> value of the IDENTICAL flag rather than the other flags passed into this

> routine. This is the special case I think Raphael was asking for rather than

> the function pointer/callback interface.

>


Ah OK, got it. Worth a comment ? I am sure I will forget next time I see this.

--
Regards,
Sudeep
diff mbox series

Patch

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
index 1865515297ca..456e1c0a35ae 100644
--- a/drivers/acpi/pptt.c
+++ b/drivers/acpi/pptt.c
@@ -432,17 +432,39 @@  static void cache_setup_acpi_cpu(struct acpi_table_header *table,
 	}
 }
 
+static bool flag_identical(struct acpi_table_header *table_hdr,
+			  struct acpi_pptt_processor *cpu)
+{
+	struct acpi_pptt_processor *next;
+
+	/* heterogeneous machines must use PPTT revision > 1 */
+	if (table_hdr->revision < 2)
+		return false;
+
+	/* Locate the last node in the tree with IDENTICAL set */
+	if (cpu->flags & ACPI_PPTT_ACPI_IDENTICAL) {
+		next = fetch_pptt_node(table_hdr, cpu->parent);
+		if (!(next && next->flags & ACPI_PPTT_ACPI_IDENTICAL))
+			return true;
+	}
+
+	return false;
+}
+
 /* Passing level values greater than this will result in search termination */
 #define PPTT_ABORT_PACKAGE 0xFF
 
-static struct acpi_pptt_processor *acpi_find_processor_package_id(struct acpi_table_header *table_hdr,
+static struct acpi_pptt_processor *acpi_find_processor_tag_id(struct acpi_table_header *table_hdr,
 								  struct acpi_pptt_processor *cpu,
 								  int level, int flag)
 {
 	struct acpi_pptt_processor *prev_node;
 
 	while (cpu && level) {
-		if (cpu->flags & flag)
+		if (flag == ACPI_PPTT_ACPI_IDENTICAL) {
+			if (flag_identical(table_hdr, cpu))
+				break;
+		} else if (cpu->flags & flag)
 			break;
 		pr_debug("level %d\n", level);
 		prev_node = fetch_pptt_node(table_hdr, cpu->parent);
@@ -480,7 +502,7 @@  static int topology_get_acpi_cpu_tag(struct acpi_table_header *table,
 
 	cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
 	if (cpu_node) {
-		cpu_node = acpi_find_processor_package_id(table, cpu_node,
+		cpu_node = acpi_find_processor_tag_id(table, cpu_node,
 							  level, flag);
 		/*
 		 * As per specification if the processor structure represents