Re: [PATCH v2 2/4] acpi: tables: Add CDAT table parsing support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]
  Powered by Linux