| Message ID | 20190503232407.37195-4-jeremy.linton@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: SPE ACPI enablement | expand |
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
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 >
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 --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
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(-)