| Message ID | 168443478863.2966470.17477171378435115702.stgit@djiang5-mobl3 |
|---|---|
| State | Superseded |
| Headers | show |
| Series | acpi: Add CDAT parsing support to ACPI tables code | expand |
Dave Jiang wrote: > The CDAT table is very similar to ACPI tables when it comes to sub-table > and entry structures. The helper functions can be also used to parse the > CDAT table. Add support to the helper functions to deal with an external > CDAT table, and also handle the endieness since CDAT can be processed by a > BE host. Export a function cdat_table_parse() for CXL driver to parse > a CDAT table. > > In order to minimize ACPICA code changes, __force is being utilized to deal > with the case of a big endien (BE) host parsing a CDAT. All CDAT data > structure variables are being force casted to __leX as appropriate. > > Cc: Rafael J. Wysocki <rafael@kernel.org> > Cc: Len Brown <lenb@kernel.org> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > --- > v2: > - Make acpi_table_header and acpi_table_cdat a union. (Jonathan) > - Use local var to make acpi_table_get_length() more readable. (Jonathan) > - Remove ACPI_SIG_CDAT define, already defined. > --- > drivers/acpi/tables.c | 5 +++- > drivers/acpi/tables_lib.c | 52 ++++++++++++++++++++++++++++++++++++++++++--- > include/linux/acpi.h | 22 +++++++++++++++++-- > 3 files changed, 72 insertions(+), 7 deletions(-) > > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > index cfc76efd8788..f7e1ea192576 100644 > --- a/drivers/acpi/tables.c > +++ b/drivers/acpi/tables.c > @@ -241,8 +241,9 @@ int __init_or_acpilib acpi_table_parse_entries_array( > return -ENODEV; > } > > - count = acpi_parse_entries_array(id, table_size, table_header, > - proc, proc_num, max_entries); > + count = acpi_parse_entries_array(id, table_size, > + (union table_header *)table_header, I think the force cast can be cleaned up, more below... > + proc, proc_num, max_entries); > > acpi_put_table(table_header); > return count; [..] > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 57ffba91bfb5..4a5b40463048 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -22,11 +22,17 @@ > #include <acpi/acpi.h> > > /* Table Handlers */ > +union table_header { > + struct acpi_table_header acpi; > + struct acpi_table_cdat cdat; > +}; 'table_header' seems too generic a name for a type in a header file included as widely as acpi.h. How about 'union acpi_parse_header'? Moreover I think the type-casting when calling the helpers might look better with explicit type-punning showing the conversion from ACPI and CDAT callers into the common parser. Something like the following folded on top (only compile tested): -- >8 -- diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index f7e1ea192576..ef31232fdcfb 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -219,7 +219,7 @@ int __init_or_acpilib acpi_table_parse_entries_array( char *id, unsigned long table_size, struct acpi_subtable_proc *proc, int proc_num, unsigned int max_entries) { - struct acpi_table_header *table_header = NULL; + union acpi_convert_header hdr; int count; u32 instance = 0; @@ -235,17 +235,16 @@ int __init_or_acpilib acpi_table_parse_entries_array( if (!strncmp(id, ACPI_SIG_MADT, 4)) instance = acpi_apic_instance; - acpi_get_table(id, instance, &table_header); - if (!table_header) { + acpi_get_table(id, instance, &hdr.acpi); + if (!hdr.acpi) { pr_debug("%4.4s not present\n", id); return -ENODEV; } - count = acpi_parse_entries_array(id, table_size, - (union table_header *)table_header, - proc, proc_num, max_entries); + count = acpi_parse_entries_array(id, table_size, hdr.parse, proc, + proc_num, max_entries); - acpi_put_table(table_header); + acpi_put_table(hdr.acpi); return count; } diff --git a/drivers/acpi/tables_lib.c b/drivers/acpi/tables_lib.c index 71e2fb1735e5..bd886900762a 100644 --- a/drivers/acpi/tables_lib.c +++ b/drivers/acpi/tables_lib.c @@ -105,7 +105,7 @@ static enum acpi_subtable_type acpi_get_subtable_type(char *id) } static unsigned long acpi_table_get_length(enum acpi_subtable_type type, - union table_header *hdr) + union acpi_parse_header *hdr) { if (type == CDAT_SUBTABLE) { __le32 length = (__force __le32)hdr->cdat.length; @@ -155,7 +155,7 @@ static int call_handler(struct acpi_subtable_proc *proc, * Otherwise, -ENODEV or -EINVAL is returned. */ int acpi_parse_entries_array(char *id, unsigned long table_size, - union table_header *table_header, + union acpi_parse_header *table_header, struct acpi_subtable_proc *proc, int proc_num, unsigned int max_entries) { @@ -224,6 +224,7 @@ int cdat_table_parse(enum acpi_cdat_type type, acpi_tbl_entry_handler_arg handler_arg, void *arg, struct acpi_table_cdat *table_header) { + union acpi_convert_header hdr = { .cdat = table_header }; struct acpi_subtable_proc proc = { .id = type, .handler_arg = handler_arg, @@ -234,7 +235,7 @@ int cdat_table_parse(enum acpi_cdat_type type, return -EINVAL; return acpi_parse_entries_array(ACPI_SIG_CDAT, - sizeof(struct acpi_table_cdat), - (union table_header *)table_header, &proc, 1, 0); + sizeof(struct acpi_table_cdat), + hdr.parse, &proc, 1, 0); } EXPORT_SYMBOL_NS_GPL(cdat_table_parse, CXL); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index dcaaaffff318..40caea4ba227 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -25,11 +25,21 @@ struct irq_domain_ops; #include <acpi/acpi.h> /* Table Handlers */ -union table_header { +union acpi_parse_header { struct acpi_table_header acpi; struct acpi_table_cdat cdat; }; +/* + * Perform type punning between ACPI and CDAT callers of the core table + * parsing routines that use acpi_parse_header internally + */ +union acpi_convert_header { + struct acpi_table_header *acpi; + struct acpi_table_cdat *cdat; + union acpi_parse_header *parse; +}; + union acpi_subtable_headers { struct acpi_subtable_header common; struct acpi_hmat_structure hmat; @@ -1539,7 +1549,7 @@ static inline void acpi_device_notify_remove(struct device *dev) { } #ifdef CONFIG_ACPI_TABLES_LIB int acpi_parse_entries_array(char *id, unsigned long table_size, - union table_header *table_header, + union acpi_parse_header *table_header, struct acpi_subtable_proc *proc, int proc_num, unsigned int max_entries); @@ -1547,10 +1557,11 @@ int cdat_table_parse(enum acpi_cdat_type type, acpi_tbl_entry_handler_arg handler, void *arg, struct acpi_table_cdat *table_header); #else -static inline int acpi_parse_entries_array( - char *id, unsigned long table_size, - union table_header *table_header, struct acpi_subtable_proc *proc, - int proc_num, unsigned int max_entries) +static inline int +acpi_parse_entries_array(char *id, unsigned long table_size, + union acpi_parse_header *table_header, + struct acpi_subtable_proc *proc, int proc_num, + unsigned int max_entries) { return -EOPNOTSUPP; }
On Tue, May 23, 2023 at 1:13 AM Dan Williams <dan.j.williams@intel.com> wrote: > > Dave Jiang wrote: > > The CDAT table is very similar to ACPI tables when it comes to sub-table > > and entry structures. The helper functions can be also used to parse the > > CDAT table. Add support to the helper functions to deal with an external > > CDAT table, and also handle the endieness since CDAT can be processed by a > > BE host. Export a function cdat_table_parse() for CXL driver to parse > > a CDAT table. > > > > In order to minimize ACPICA code changes, __force is being utilized to deal > > with the case of a big endien (BE) host parsing a CDAT. All CDAT data > > structure variables are being force casted to __leX as appropriate. > > > > Cc: Rafael J. Wysocki <rafael@kernel.org> > > Cc: Len Brown <lenb@kernel.org> > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > > > --- > > v2: > > - Make acpi_table_header and acpi_table_cdat a union. (Jonathan) > > - Use local var to make acpi_table_get_length() more readable. (Jonathan) > > - Remove ACPI_SIG_CDAT define, already defined. > > --- > > drivers/acpi/tables.c | 5 +++- > > drivers/acpi/tables_lib.c | 52 ++++++++++++++++++++++++++++++++++++++++++--- > > include/linux/acpi.h | 22 +++++++++++++++++-- > > 3 files changed, 72 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > > index cfc76efd8788..f7e1ea192576 100644 > > --- a/drivers/acpi/tables.c > > +++ b/drivers/acpi/tables.c > > @@ -241,8 +241,9 @@ int __init_or_acpilib acpi_table_parse_entries_array( > > return -ENODEV; > > } > > > > - count = acpi_parse_entries_array(id, table_size, table_header, > > - proc, proc_num, max_entries); > > + count = acpi_parse_entries_array(id, table_size, > > + (union table_header *)table_header, > > I think the force cast can be cleaned up, more below... > > > + proc, proc_num, max_entries); > > > > acpi_put_table(table_header); > > return count; > [..] > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > > index 57ffba91bfb5..4a5b40463048 100644 > > --- a/include/linux/acpi.h > > +++ b/include/linux/acpi.h > > @@ -22,11 +22,17 @@ > > #include <acpi/acpi.h> > > > > /* Table Handlers */ > > +union table_header { > > + struct acpi_table_header acpi; > > + struct acpi_table_cdat cdat; > > +}; > > 'table_header' seems too generic a name for a type in a header file > included as widely as acpi.h. How about 'union acpi_parse_header'? > > Moreover I think the type-casting when calling the helpers might look > better with explicit type-punning showing the conversion from ACPI and > CDAT callers into the common parser. Something like the following folded > on top (only compile tested): > > -- >8 -- > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > index f7e1ea192576..ef31232fdcfb 100644 > --- a/drivers/acpi/tables.c > +++ b/drivers/acpi/tables.c > @@ -219,7 +219,7 @@ int __init_or_acpilib acpi_table_parse_entries_array( > char *id, unsigned long table_size, struct acpi_subtable_proc *proc, > int proc_num, unsigned int max_entries) > { > - struct acpi_table_header *table_header = NULL; > + union acpi_convert_header hdr; > int count; > u32 instance = 0; > > @@ -235,17 +235,16 @@ int __init_or_acpilib acpi_table_parse_entries_array( > if (!strncmp(id, ACPI_SIG_MADT, 4)) > instance = acpi_apic_instance; > > - acpi_get_table(id, instance, &table_header); > - if (!table_header) { > + acpi_get_table(id, instance, &hdr.acpi); > + if (!hdr.acpi) { > pr_debug("%4.4s not present\n", id); > return -ENODEV; > } > > - count = acpi_parse_entries_array(id, table_size, > - (union table_header *)table_header, > - proc, proc_num, max_entries); > + count = acpi_parse_entries_array(id, table_size, hdr.parse, proc, > + proc_num, max_entries); > > - acpi_put_table(table_header); > + acpi_put_table(hdr.acpi); > return count; > } > > diff --git a/drivers/acpi/tables_lib.c b/drivers/acpi/tables_lib.c > index 71e2fb1735e5..bd886900762a 100644 > --- a/drivers/acpi/tables_lib.c > +++ b/drivers/acpi/tables_lib.c > @@ -105,7 +105,7 @@ static enum acpi_subtable_type acpi_get_subtable_type(char *id) > } > > static unsigned long acpi_table_get_length(enum acpi_subtable_type type, > - union table_header *hdr) > + union acpi_parse_header *hdr) If this is going to be common library code, I'm wondering how much of "acpi" needs to be there in the names.
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index cfc76efd8788..f7e1ea192576 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -241,8 +241,9 @@ int __init_or_acpilib acpi_table_parse_entries_array( return -ENODEV; } - count = acpi_parse_entries_array(id, table_size, table_header, - proc, proc_num, max_entries); + count = acpi_parse_entries_array(id, table_size, + (union table_header *)table_header, + proc, proc_num, max_entries); acpi_put_table(table_header); return count; diff --git a/drivers/acpi/tables_lib.c b/drivers/acpi/tables_lib.c index 701001610fa9..71e2fb1735e5 100644 --- a/drivers/acpi/tables_lib.c +++ b/drivers/acpi/tables_lib.c @@ -28,6 +28,7 @@ enum acpi_subtable_type { ACPI_SUBTABLE_HMAT, ACPI_SUBTABLE_PRMT, ACPI_SUBTABLE_CEDT, + CDAT_SUBTABLE, }; struct acpi_subtable_entry { @@ -46,6 +47,8 @@ static unsigned long acpi_get_entry_type(struct acpi_subtable_entry *entry) return 0; case ACPI_SUBTABLE_CEDT: return entry->hdr->cedt.type; + case CDAT_SUBTABLE: + return entry->hdr->cdat.type; } return 0; } @@ -61,6 +64,11 @@ static unsigned long acpi_get_entry_length(struct acpi_subtable_entry *entry) return entry->hdr->prmt.length; case ACPI_SUBTABLE_CEDT: return entry->hdr->cedt.length; + case CDAT_SUBTABLE: { + __le16 length = (__force __le16)entry->hdr->cdat.length; + + return le16_to_cpu(length); + } } return 0; } @@ -77,6 +85,8 @@ acpi_get_subtable_header_length(struct acpi_subtable_entry *entry) return sizeof(entry->hdr->prmt); case ACPI_SUBTABLE_CEDT: return sizeof(entry->hdr->cedt); + case CDAT_SUBTABLE: + return sizeof(entry->hdr->cdat); } return 0; } @@ -89,9 +99,23 @@ static enum acpi_subtable_type acpi_get_subtable_type(char *id) return ACPI_SUBTABLE_PRMT; if (strncmp(id, ACPI_SIG_CEDT, 4) == 0) return ACPI_SUBTABLE_CEDT; + if (strncmp(id, ACPI_SIG_CDAT, 4) == 0) + return CDAT_SUBTABLE; return ACPI_SUBTABLE_COMMON; } +static unsigned long acpi_table_get_length(enum acpi_subtable_type type, + union table_header *hdr) +{ + if (type == CDAT_SUBTABLE) { + __le32 length = (__force __le32)hdr->cdat.length; + + return le32_to_cpu(length); + } + + return hdr->acpi.length; +} + static bool has_handler(struct acpi_subtable_proc *proc) { return proc->handler || proc->handler_arg; @@ -131,21 +155,24 @@ static int call_handler(struct acpi_subtable_proc *proc, * Otherwise, -ENODEV or -EINVAL is returned. */ int acpi_parse_entries_array(char *id, unsigned long table_size, - struct acpi_table_header *table_header, + union table_header *table_header, struct acpi_subtable_proc *proc, int proc_num, unsigned int max_entries) { struct acpi_subtable_entry entry; + enum acpi_subtable_type type; unsigned long table_end, subtable_len, entry_len; int count = 0; int errs = 0; int i; - table_end = (unsigned long)table_header + table_header->length; + type = acpi_get_subtable_type(id); + table_end = (unsigned long)table_header + + acpi_table_get_length(type, table_header); /* Parse all entries looking for a match. */ - entry.type = acpi_get_subtable_type(id); + entry.type = type; entry.hdr = (union acpi_subtable_headers *) ((unsigned long)table_header + table_size); subtable_len = acpi_get_subtable_header_length(&entry); @@ -192,3 +219,22 @@ int acpi_parse_entries_array(char *id, unsigned long table_size, return errs ? -EINVAL : count; } EXPORT_SYMBOL_GPL(acpi_parse_entries_array); + +int cdat_table_parse(enum acpi_cdat_type type, + acpi_tbl_entry_handler_arg handler_arg, void *arg, + struct acpi_table_cdat *table_header) +{ + struct acpi_subtable_proc proc = { + .id = type, + .handler_arg = handler_arg, + .arg = arg, + }; + + if (!table_header) + return -EINVAL; + + return acpi_parse_entries_array(ACPI_SIG_CDAT, + sizeof(struct acpi_table_cdat), + (union table_header *)table_header, &proc, 1, 0); +} +EXPORT_SYMBOL_NS_GPL(cdat_table_parse, CXL); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 57ffba91bfb5..4a5b40463048 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -22,11 +22,17 @@ #include <acpi/acpi.h> /* Table Handlers */ +union table_header { + struct acpi_table_header acpi; + struct acpi_table_cdat cdat; +}; + union acpi_subtable_headers { struct acpi_subtable_header common; struct acpi_hmat_structure hmat; struct acpi_prmt_module_header prmt; struct acpi_cedt_header cedt; + struct acpi_cdat_header cdat; }; typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table); @@ -1526,17 +1532,29 @@ static inline void acpi_device_notify_remove(struct device *dev) { } #ifdef CONFIG_ACPI_TABLES_LIB int acpi_parse_entries_array(char *id, unsigned long table_size, - struct acpi_table_header *table_header, + union table_header *table_header, struct acpi_subtable_proc *proc, int proc_num, unsigned int max_entries); + +int cdat_table_parse(enum acpi_cdat_type type, + acpi_tbl_entry_handler_arg handler, void *arg, + struct acpi_table_cdat *table_header); #else static inline int acpi_parse_entries_array( char *id, unsigned long table_size, - struct acpi_table_header *table_header, struct acpi_subtable_proc *proc, + union table_header *table_header, struct acpi_subtable_proc *proc, int proc_num, unsigned int max_entries) { return -EOPNOTSUPP; } + +static inline int cdat_table_parse( + enum acpi_cdat_type type, + acpi_tbl_entry_handler_arg handler, void *arg, + struct acpi_table_cdat *table_header) +{ + return -EOPNOTSUPP; +} #endif #endif /*_LINUX_ACPI_H*/
The CDAT table is very similar to ACPI tables when it comes to sub-table and entry structures. The helper functions can be also used to parse the CDAT table. Add support to the helper functions to deal with an external CDAT table, and also handle the endieness since CDAT can be processed by a BE host. Export a function cdat_table_parse() for CXL driver to parse a CDAT table. In order to minimize ACPICA code changes, __force is being utilized to deal with the case of a big endien (BE) host parsing a CDAT. All CDAT data structure variables are being force casted to __leX as appropriate. Cc: Rafael J. Wysocki <rafael@kernel.org> Cc: Len Brown <lenb@kernel.org> Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- v2: - Make acpi_table_header and acpi_table_cdat a union. (Jonathan) - Use local var to make acpi_table_get_length() more readable. (Jonathan) - Remove ACPI_SIG_CDAT define, already defined. --- drivers/acpi/tables.c | 5 +++- drivers/acpi/tables_lib.c | 52 ++++++++++++++++++++++++++++++++++++++++++--- include/linux/acpi.h | 22 +++++++++++++++++-- 3 files changed, 72 insertions(+), 7 deletions(-)