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

[Dan Williams <dan.j.williams@intel.com>]

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;
 }