On Tue, May 23, 2023 at 1:13 AM Dan Williams <dan.j.williams@xxxxxxxxx> 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@xxxxxxxxxx>
> > Cc: Len Brown <lenb@xxxxxxxxxx>
> > Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> >
> > ---
> > 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.
