| Message ID | 20171214021019.13579-2-ross.zwisler@linux.intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Thu, Dec 14, 2017 at 3:10 AM, Ross Zwisler <ross.zwisler@linux.intel.com> wrote: > The current implementation of acpi_parse_entries_array() assumes that each > subtable has a standard ACPI subtable entry of type struct > acpi_subtable_header. This standard subtable header has a one byte length > followed by a one byte type. > > The HMAT subtables have to allow for a longer length so they have subtable > headers of type struct acpi_hmat_structure which has a 2 byte type and a 4 > byte length. > > Enhance the subtable parsing in acpi_parse_entries_array() so that it can > handle these new HMAT subtables. > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> This one is fine by me. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 13, 2017 at 6:10 PM, Ross Zwisler <ross.zwisler@linux.intel.com> wrote: > The current implementation of acpi_parse_entries_array() assumes that each > subtable has a standard ACPI subtable entry of type struct > acpi_subtable_header. This standard subtable header has a one byte length > followed by a one byte type. > > The HMAT subtables have to allow for a longer length so they have subtable > headers of type struct acpi_hmat_structure which has a 2 byte type and a 4 > byte length. Hmm, NFIT has a 2 byte type and a 2 byte length, so its one more permutation. I happened to reinvent sub-table parsing in the NFIT driver, but it might be nice in the future to refactor that to use the common parsing. > > Enhance the subtable parsing in acpi_parse_entries_array() so that it can > handle these new HMAT subtables. > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > --- > drivers/acpi/tables.c | 52 ++++++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 41 insertions(+), 11 deletions(-) > > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > index 80ce2a7d224b..f777b94c234a 100644 > --- a/drivers/acpi/tables.c > +++ b/drivers/acpi/tables.c > @@ -218,6 +218,33 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) > } > } > > +static unsigned long __init > +acpi_get_entry_type(char *id, void *entry) > +{ > + if (strncmp(id, ACPI_SIG_HMAT, 4) == 0) > + return ((struct acpi_hmat_structure *)entry)->type; > + else > + return ((struct acpi_subtable_header *)entry)->type; > +} It seems inefficient to make all checks keep asking "is HMAT?". Especially if we want to extend this to other table types should we instead setup and pass a pair of function pointers to parse the sub-table format? -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, December 15, 2017 2:10:17 AM CET Dan Williams wrote: > On Wed, Dec 13, 2017 at 6:10 PM, Ross Zwisler > <ross.zwisler@linux.intel.com> wrote: > > The current implementation of acpi_parse_entries_array() assumes that each > > subtable has a standard ACPI subtable entry of type struct > > acpi_subtable_header. This standard subtable header has a one byte length > > followed by a one byte type. > > > > The HMAT subtables have to allow for a longer length so they have subtable > > headers of type struct acpi_hmat_structure which has a 2 byte type and a 4 > > byte length. > > Hmm, NFIT has a 2 byte type and a 2 byte length, so its one more > permutation. I happened to reinvent sub-table parsing in the NFIT > driver, but it might be nice in the future to refactor that to use the > common parsing. > > > > > Enhance the subtable parsing in acpi_parse_entries_array() so that it can > > handle these new HMAT subtables. > > > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > > --- > > drivers/acpi/tables.c | 52 ++++++++++++++++++++++++++++++++++++++++----------- > > 1 file changed, 41 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > > index 80ce2a7d224b..f777b94c234a 100644 > > --- a/drivers/acpi/tables.c > > +++ b/drivers/acpi/tables.c > > @@ -218,6 +218,33 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) > > } > > } > > > > +static unsigned long __init > > +acpi_get_entry_type(char *id, void *entry) > > +{ > > + if (strncmp(id, ACPI_SIG_HMAT, 4) == 0) > > + return ((struct acpi_hmat_structure *)entry)->type; > > + else > > + return ((struct acpi_subtable_header *)entry)->type; > > +} > > It seems inefficient to make all checks keep asking "is HMAT?". Well, ideally, the signature should be checked once. I guess that can be arranged for here. > Especially if we want to extend this to other table types should we > instead setup and pass a pair of function pointers to parse the > sub-table format? Function pointers may be too much even. :-) Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 15, 2017 at 5:53 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Friday, December 15, 2017 2:10:17 AM CET Dan Williams wrote: >> On Wed, Dec 13, 2017 at 6:10 PM, Ross Zwisler >> <ross.zwisler@linux.intel.com> wrote: >> > The current implementation of acpi_parse_entries_array() assumes that each >> > subtable has a standard ACPI subtable entry of type struct >> > acpi_subtable_header. This standard subtable header has a one byte length >> > followed by a one byte type. >> > >> > The HMAT subtables have to allow for a longer length so they have subtable >> > headers of type struct acpi_hmat_structure which has a 2 byte type and a 4 >> > byte length. >> >> Hmm, NFIT has a 2 byte type and a 2 byte length, so its one more >> permutation. I happened to reinvent sub-table parsing in the NFIT >> driver, but it might be nice in the future to refactor that to use the >> common parsing. >> >> > >> > Enhance the subtable parsing in acpi_parse_entries_array() so that it can >> > handle these new HMAT subtables. >> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> >> > --- >> > drivers/acpi/tables.c | 52 ++++++++++++++++++++++++++++++++++++++++----------- >> > 1 file changed, 41 insertions(+), 11 deletions(-) >> > >> > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c >> > index 80ce2a7d224b..f777b94c234a 100644 >> > --- a/drivers/acpi/tables.c >> > +++ b/drivers/acpi/tables.c >> > @@ -218,6 +218,33 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) >> > } >> > } >> > >> > +static unsigned long __init >> > +acpi_get_entry_type(char *id, void *entry) >> > +{ >> > + if (strncmp(id, ACPI_SIG_HMAT, 4) == 0) >> > + return ((struct acpi_hmat_structure *)entry)->type; >> > + else >> > + return ((struct acpi_subtable_header *)entry)->type; >> > +} >> >> It seems inefficient to make all checks keep asking "is HMAT?". > > Well, ideally, the signature should be checked once. I guess that can be > arranged for here. > >> Especially if we want to extend this to other table types should we >> instead setup and pass a pair of function pointers to parse the >> sub-table format? > > Function pointers may be too much even. :-) True, how about an enum of acpi sub-table header types? -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Dec 16, 2017 at 2:57 AM, Dan Williams <dan.j.williams@intel.com> wrote: > On Fri, Dec 15, 2017 at 5:53 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> On Friday, December 15, 2017 2:10:17 AM CET Dan Williams wrote: >>> On Wed, Dec 13, 2017 at 6:10 PM, Ross Zwisler >>> <ross.zwisler@linux.intel.com> wrote: >>> > The current implementation of acpi_parse_entries_array() assumes that each >>> > subtable has a standard ACPI subtable entry of type struct >>> > acpi_subtable_header. This standard subtable header has a one byte length >>> > followed by a one byte type. >>> > >>> > The HMAT subtables have to allow for a longer length so they have subtable >>> > headers of type struct acpi_hmat_structure which has a 2 byte type and a 4 >>> > byte length. >>> >>> Hmm, NFIT has a 2 byte type and a 2 byte length, so its one more >>> permutation. I happened to reinvent sub-table parsing in the NFIT >>> driver, but it might be nice in the future to refactor that to use the >>> common parsing. >>> >>> > >>> > Enhance the subtable parsing in acpi_parse_entries_array() so that it can >>> > handle these new HMAT subtables. >>> > >>> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> >>> > --- >>> > drivers/acpi/tables.c | 52 ++++++++++++++++++++++++++++++++++++++++----------- >>> > 1 file changed, 41 insertions(+), 11 deletions(-) >>> > >>> > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c >>> > index 80ce2a7d224b..f777b94c234a 100644 >>> > --- a/drivers/acpi/tables.c >>> > +++ b/drivers/acpi/tables.c >>> > @@ -218,6 +218,33 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) >>> > } >>> > } >>> > >>> > +static unsigned long __init >>> > +acpi_get_entry_type(char *id, void *entry) >>> > +{ >>> > + if (strncmp(id, ACPI_SIG_HMAT, 4) == 0) >>> > + return ((struct acpi_hmat_structure *)entry)->type; >>> > + else >>> > + return ((struct acpi_subtable_header *)entry)->type; >>> > +} >>> >>> It seems inefficient to make all checks keep asking "is HMAT?". >> >> Well, ideally, the signature should be checked once. I guess that can be >> arranged for here. >> >>> Especially if we want to extend this to other table types should we >>> instead setup and pass a pair of function pointers to parse the >>> sub-table format? >> >> Function pointers may be too much even. :-) > > True, how about an enum of acpi sub-table header types? That works. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 80ce2a7d224b..f777b94c234a 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -218,6 +218,33 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) } } +static unsigned long __init +acpi_get_entry_type(char *id, void *entry) +{ + if (strncmp(id, ACPI_SIG_HMAT, 4) == 0) + return ((struct acpi_hmat_structure *)entry)->type; + else + return ((struct acpi_subtable_header *)entry)->type; +} + +static unsigned long __init +acpi_get_entry_length(char *id, void *entry) +{ + if (strncmp(id, ACPI_SIG_HMAT, 4) == 0) + return ((struct acpi_hmat_structure *)entry)->length; + else + return ((struct acpi_subtable_header *)entry)->length; +} + +static unsigned long __init +acpi_get_subtable_header_length(char *id) +{ + if (strncmp(id, ACPI_SIG_HMAT, 4) == 0) + return sizeof(struct acpi_hmat_structure); + else + return sizeof(struct acpi_subtable_header); +} + /** * acpi_parse_entries_array - for each proc_num find a suitable subtable * @@ -242,10 +269,10 @@ acpi_parse_entries_array(char *id, unsigned long table_size, struct acpi_subtable_proc *proc, int proc_num, unsigned int max_entries) { - struct acpi_subtable_header *entry; - unsigned long table_end; + unsigned long table_end, subtable_header_length; int count = 0; int errs = 0; + void *entry; int i; if (acpi_disabled) @@ -263,19 +290,23 @@ acpi_parse_entries_array(char *id, unsigned long table_size, } table_end = (unsigned long)table_header + table_header->length; + subtable_header_length = acpi_get_subtable_header_length(id); /* Parse all entries looking for a match. */ - entry = (struct acpi_subtable_header *) - ((unsigned long)table_header + table_size); + entry = (void *)table_header + table_size; + + while (((unsigned long)entry) + subtable_header_length < table_end) { + unsigned long entry_type, entry_length; - while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) < - table_end) { if (max_entries && count >= max_entries) break; + entry_type = acpi_get_entry_type(id, entry); + entry_length = acpi_get_entry_length(id, entry); + for (i = 0; i < proc_num; i++) { - if (entry->type != proc[i].id) + if (entry_type != proc[i].id) continue; if (!proc[i].handler || (!errs && proc[i].handler(entry, table_end))) { @@ -290,16 +321,15 @@ acpi_parse_entries_array(char *id, unsigned long table_size, count++; /* - * If entry->length is 0, break from this loop to avoid + * If entry_length is 0, break from this loop to avoid * infinite loop. */ - if (entry->length == 0) { + if (entry_length == 0) { pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, proc->id); return -EINVAL; } - entry = (struct acpi_subtable_header *) - ((unsigned long)entry + entry->length); + entry += entry_length; } if (max_entries && count > max_entries) {
The current implementation of acpi_parse_entries_array() assumes that each subtable has a standard ACPI subtable entry of type struct acpi_subtable_header. This standard subtable header has a one byte length followed by a one byte type. The HMAT subtables have to allow for a longer length so they have subtable headers of type struct acpi_hmat_structure which has a 2 byte type and a 4 byte length. Enhance the subtable parsing in acpi_parse_entries_array() so that it can handle these new HMAT subtables. Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> --- drivers/acpi/tables.c | 52 ++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 41 insertions(+), 11 deletions(-)