* [PATCH v2 1/4] acpi: Move common tables helper functions to common lib
2023-05-18 18:32 [PATCH v2 0/4] acpi: Add CDAT parsing support to ACPI tables code Dave Jiang
@ 2023-05-18 18:33 ` Dave Jiang
2023-05-22 21:31 ` Dan Williams
2023-06-01 14:50 ` Jonathan Cameron
2023-05-18 18:33 ` [PATCH v2 2/4] acpi: tables: Add CDAT table parsing support Dave Jiang
` (2 subsequent siblings)
3 siblings, 2 replies; 19+ messages in thread
From: Dave Jiang @ 2023-05-18 18:33 UTC (permalink / raw)
To: linux-acpi, linux-cxl
Cc: rafael, lenb, dan.j.williams, ira.weiny, vishal.l.verma,
alison.schofield, lukas, Jonathan.Cameron
Some of the routines in ACPI tables.c can be shared with parsing CDAT.
However, CDAT is used by CXL and can exist on platforms that do not use
ACPI. Split out the common routine from ACPI to accomodate platforms that
do not support ACPI. The common routines can be built outside of ACPI if
ACPI_TABLES_LIB is selected.
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
drivers/Makefile | 2
drivers/acpi/Kconfig | 4 +
drivers/acpi/Makefile | 3 +
drivers/acpi/tables.c | 173 ----------------------------------------
drivers/acpi/tables_lib.c | 194 +++++++++++++++++++++++++++++++++++++++++++++
include/linux/acpi.h | 63 +++++++++------
6 files changed, 241 insertions(+), 198 deletions(-)
create mode 100644 drivers/acpi/tables_lib.c
diff --git a/drivers/Makefile b/drivers/Makefile
index 20b118dca999..1824797f7dfe 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -31,7 +31,7 @@ obj-y += idle/
# IPMI must come before ACPI in order to provide IPMI opregion support
obj-y += char/ipmi/
-obj-$(CONFIG_ACPI) += acpi/
+obj-y += acpi/
# PnP must come after ACPI since it will eventually need to check if acpi
# was used and do nothing if so
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index ccbeab9500ec..ce74a20dc42f 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -6,12 +6,16 @@
config ARCH_SUPPORTS_ACPI
bool
+config ACPI_TABLES_LIB
+ bool
+
menuconfig ACPI
bool "ACPI (Advanced Configuration and Power Interface) Support"
depends on ARCH_SUPPORTS_ACPI
select PNP
select NLS
select CRC32
+ select ACPI_TABLES_LIB
default y if X86
help
Advanced Configuration and Power Interface (ACPI) support for
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index feb36c0b9446..4558e2876823 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -13,6 +13,9 @@ tables.o: $(src)/../../include/$(CONFIG_ACPI_CUSTOM_DSDT_FILE) ;
endif
+obj-$(CONFIG_ACPI_TABLES_LIB) += acpi_tables_lib.o
+acpi_tables_lib-y := tables_lib.o
+
obj-$(CONFIG_ACPI) += tables.o
obj-$(CONFIG_X86) += blacklist.o
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 7b4680da57d7..cfc76efd8788 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -37,18 +37,6 @@ static struct acpi_table_desc initial_tables[ACPI_MAX_TABLES] __initdata;
static int acpi_apic_instance __initdata_or_acpilib;
-enum acpi_subtable_type {
- ACPI_SUBTABLE_COMMON,
- ACPI_SUBTABLE_HMAT,
- ACPI_SUBTABLE_PRMT,
- ACPI_SUBTABLE_CEDT,
-};
-
-struct acpi_subtable_entry {
- union acpi_subtable_headers *hdr;
- enum acpi_subtable_type type;
-};
-
/*
* Disable table checksum verification for the early stage due to the size
* limitation of the current x86 early mapping implementation.
@@ -227,167 +215,6 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
}
}
-static unsigned long __init_or_acpilib
-acpi_get_entry_type(struct acpi_subtable_entry *entry)
-{
- switch (entry->type) {
- case ACPI_SUBTABLE_COMMON:
- return entry->hdr->common.type;
- case ACPI_SUBTABLE_HMAT:
- return entry->hdr->hmat.type;
- case ACPI_SUBTABLE_PRMT:
- return 0;
- case ACPI_SUBTABLE_CEDT:
- return entry->hdr->cedt.type;
- }
- return 0;
-}
-
-static unsigned long __init_or_acpilib
-acpi_get_entry_length(struct acpi_subtable_entry *entry)
-{
- switch (entry->type) {
- case ACPI_SUBTABLE_COMMON:
- return entry->hdr->common.length;
- case ACPI_SUBTABLE_HMAT:
- return entry->hdr->hmat.length;
- case ACPI_SUBTABLE_PRMT:
- return entry->hdr->prmt.length;
- case ACPI_SUBTABLE_CEDT:
- return entry->hdr->cedt.length;
- }
- return 0;
-}
-
-static unsigned long __init_or_acpilib
-acpi_get_subtable_header_length(struct acpi_subtable_entry *entry)
-{
- switch (entry->type) {
- case ACPI_SUBTABLE_COMMON:
- return sizeof(entry->hdr->common);
- case ACPI_SUBTABLE_HMAT:
- return sizeof(entry->hdr->hmat);
- case ACPI_SUBTABLE_PRMT:
- return sizeof(entry->hdr->prmt);
- case ACPI_SUBTABLE_CEDT:
- return sizeof(entry->hdr->cedt);
- }
- return 0;
-}
-
-static enum acpi_subtable_type __init_or_acpilib
-acpi_get_subtable_type(char *id)
-{
- if (strncmp(id, ACPI_SIG_HMAT, 4) == 0)
- return ACPI_SUBTABLE_HMAT;
- if (strncmp(id, ACPI_SIG_PRMT, 4) == 0)
- return ACPI_SUBTABLE_PRMT;
- if (strncmp(id, ACPI_SIG_CEDT, 4) == 0)
- return ACPI_SUBTABLE_CEDT;
- return ACPI_SUBTABLE_COMMON;
-}
-
-static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc)
-{
- return proc->handler || proc->handler_arg;
-}
-
-static __init_or_acpilib int call_handler(struct acpi_subtable_proc *proc,
- union acpi_subtable_headers *hdr,
- unsigned long end)
-{
- if (proc->handler)
- return proc->handler(hdr, end);
- if (proc->handler_arg)
- return proc->handler_arg(hdr, proc->arg, end);
- return -EINVAL;
-}
-
-/**
- * acpi_parse_entries_array - for each proc_num find a suitable subtable
- *
- * @id: table id (for debugging purposes)
- * @table_size: size of the root table
- * @table_header: where does the table start?
- * @proc: array of acpi_subtable_proc struct containing entry id
- * and associated handler with it
- * @proc_num: how big proc is?
- * @max_entries: how many entries can we process?
- *
- * For each proc_num find a subtable with proc->id and run proc->handler
- * on it. Assumption is that there's only single handler for particular
- * entry id.
- *
- * The table_size is not the size of the complete ACPI table (the length
- * field in the header struct), but only the size of the root table; i.e.,
- * the offset from the very first byte of the complete ACPI table, to the
- * first byte of the very first subtable.
- *
- * On success returns sum of all matching entries for all proc handlers.
- * Otherwise, -ENODEV or -EINVAL is returned.
- */
-static int __init_or_acpilib acpi_parse_entries_array(
- char *id, unsigned long table_size,
- struct acpi_table_header *table_header, struct acpi_subtable_proc *proc,
- int proc_num, unsigned int max_entries)
-{
- struct acpi_subtable_entry entry;
- 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;
-
- /* Parse all entries looking for a match. */
-
- entry.type = acpi_get_subtable_type(id);
- entry.hdr = (union acpi_subtable_headers *)
- ((unsigned long)table_header + table_size);
- subtable_len = acpi_get_subtable_header_length(&entry);
-
- while (((unsigned long)entry.hdr) + subtable_len < table_end) {
- if (max_entries && count >= max_entries)
- break;
-
- for (i = 0; i < proc_num; i++) {
- if (acpi_get_entry_type(&entry) != proc[i].id)
- continue;
- if (!has_handler(&proc[i]) ||
- (!errs &&
- call_handler(&proc[i], entry.hdr, table_end))) {
- errs++;
- continue;
- }
-
- proc[i].count++;
- break;
- }
- if (i != proc_num)
- count++;
-
- /*
- * If entry->length is 0, break from this loop to avoid
- * infinite loop.
- */
- entry_len = acpi_get_entry_length(&entry);
- if (entry_len == 0) {
- pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, proc->id);
- return -EINVAL;
- }
-
- entry.hdr = (union acpi_subtable_headers *)
- ((unsigned long)entry.hdr + entry_len);
- }
-
- if (max_entries && count > max_entries) {
- pr_warn("[%4.4s:0x%02x] found the maximum %i entries\n",
- id, proc->id, count);
- }
-
- return errs ? -EINVAL : count;
-}
-
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)
diff --git a/drivers/acpi/tables_lib.c b/drivers/acpi/tables_lib.c
new file mode 100644
index 000000000000..701001610fa9
--- /dev/null
+++ b/drivers/acpi/tables_lib.c
@@ -0,0 +1,194 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * acpi_tables.c - ACPI Boot-Time Table Parsing
+ *
+ * Copyright (C) 2001 Paul Diefenbaugh <paul.s.diefenbaugh@intel.com>
+ */
+
+/* Uncomment next line to get verbose printout */
+/* #define DEBUG */
+#define pr_fmt(fmt) "ACPI: " fmt
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/smp.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/irq.h>
+#include <linux/errno.h>
+#include <linux/acpi.h>
+#include <linux/memblock.h>
+#include <linux/earlycpio.h>
+#include <linux/initrd.h>
+#include <linux/security.h>
+#include <linux/kmemleak.h>
+
+enum acpi_subtable_type {
+ ACPI_SUBTABLE_COMMON,
+ ACPI_SUBTABLE_HMAT,
+ ACPI_SUBTABLE_PRMT,
+ ACPI_SUBTABLE_CEDT,
+};
+
+struct acpi_subtable_entry {
+ union acpi_subtable_headers *hdr;
+ enum acpi_subtable_type type;
+};
+
+static unsigned long acpi_get_entry_type(struct acpi_subtable_entry *entry)
+{
+ switch (entry->type) {
+ case ACPI_SUBTABLE_COMMON:
+ return entry->hdr->common.type;
+ case ACPI_SUBTABLE_HMAT:
+ return entry->hdr->hmat.type;
+ case ACPI_SUBTABLE_PRMT:
+ return 0;
+ case ACPI_SUBTABLE_CEDT:
+ return entry->hdr->cedt.type;
+ }
+ return 0;
+}
+
+static unsigned long acpi_get_entry_length(struct acpi_subtable_entry *entry)
+{
+ switch (entry->type) {
+ case ACPI_SUBTABLE_COMMON:
+ return entry->hdr->common.length;
+ case ACPI_SUBTABLE_HMAT:
+ return entry->hdr->hmat.length;
+ case ACPI_SUBTABLE_PRMT:
+ return entry->hdr->prmt.length;
+ case ACPI_SUBTABLE_CEDT:
+ return entry->hdr->cedt.length;
+ }
+ return 0;
+}
+
+static unsigned long
+acpi_get_subtable_header_length(struct acpi_subtable_entry *entry)
+{
+ switch (entry->type) {
+ case ACPI_SUBTABLE_COMMON:
+ return sizeof(entry->hdr->common);
+ case ACPI_SUBTABLE_HMAT:
+ return sizeof(entry->hdr->hmat);
+ case ACPI_SUBTABLE_PRMT:
+ return sizeof(entry->hdr->prmt);
+ case ACPI_SUBTABLE_CEDT:
+ return sizeof(entry->hdr->cedt);
+ }
+ return 0;
+}
+
+static enum acpi_subtable_type acpi_get_subtable_type(char *id)
+{
+ if (strncmp(id, ACPI_SIG_HMAT, 4) == 0)
+ return ACPI_SUBTABLE_HMAT;
+ if (strncmp(id, ACPI_SIG_PRMT, 4) == 0)
+ return ACPI_SUBTABLE_PRMT;
+ if (strncmp(id, ACPI_SIG_CEDT, 4) == 0)
+ return ACPI_SUBTABLE_CEDT;
+ return ACPI_SUBTABLE_COMMON;
+}
+
+static bool has_handler(struct acpi_subtable_proc *proc)
+{
+ return proc->handler || proc->handler_arg;
+}
+
+static int call_handler(struct acpi_subtable_proc *proc,
+ union acpi_subtable_headers *hdr, unsigned long end)
+{
+ if (proc->handler)
+ return proc->handler(hdr, end);
+ if (proc->handler_arg)
+ return proc->handler_arg(hdr, proc->arg, end);
+ return -EINVAL;
+}
+
+/**
+ * acpi_parse_entries_array - for each proc_num find a suitable subtable
+ *
+ * @id: table id (for debugging purposes)
+ * @table_size: size of the root table
+ * @table_header: where does the table start?
+ * @proc: array of acpi_subtable_proc struct containing entry id
+ * and associated handler with it
+ * @proc_num: how big proc is?
+ * @max_entries: how many entries can we process?
+ *
+ * For each proc_num find a subtable with proc->id and run proc->handler
+ * on it. Assumption is that there's only single handler for particular
+ * entry id.
+ *
+ * The table_size is not the size of the complete ACPI table (the length
+ * field in the header struct), but only the size of the root table; i.e.,
+ * the offset from the very first byte of the complete ACPI table, to the
+ * first byte of the very first subtable.
+ *
+ * On success returns sum of all matching entries for all proc handlers.
+ * Otherwise, -ENODEV or -EINVAL is returned.
+ */
+int acpi_parse_entries_array(char *id, unsigned long table_size,
+ struct acpi_table_header *table_header,
+ struct acpi_subtable_proc *proc,
+ int proc_num, unsigned int max_entries)
+{
+ struct acpi_subtable_entry entry;
+ 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;
+
+ /* Parse all entries looking for a match. */
+
+ entry.type = acpi_get_subtable_type(id);
+ entry.hdr = (union acpi_subtable_headers *)
+ ((unsigned long)table_header + table_size);
+ subtable_len = acpi_get_subtable_header_length(&entry);
+
+ while (((unsigned long)entry.hdr) + subtable_len < table_end) {
+ if (max_entries && count >= max_entries)
+ break;
+
+ for (i = 0; i < proc_num; i++) {
+ if (acpi_get_entry_type(&entry) != proc[i].id)
+ continue;
+ if (!has_handler(&proc[i]) ||
+ (!errs &&
+ call_handler(&proc[i], entry.hdr, table_end))) {
+ errs++;
+ continue;
+ }
+
+ proc[i].count++;
+ break;
+ }
+ if (i != proc_num)
+ count++;
+
+ /*
+ * If entry->length is 0, break from this loop to avoid
+ * infinite loop.
+ */
+ entry_len = acpi_get_entry_length(&entry);
+ if (entry_len == 0) {
+ pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, proc->id);
+ return -EINVAL;
+ }
+
+ entry.hdr = (union acpi_subtable_headers *)
+ ((unsigned long)entry.hdr + entry_len);
+ }
+
+ if (max_entries && count > max_entries) {
+ pr_warn("[%4.4s:0x%02x] found the maximum %i entries\n",
+ id, proc->id, count);
+ }
+
+ return errs ? -EINVAL : count;
+}
+EXPORT_SYMBOL_GPL(acpi_parse_entries_array);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index efff750f326d..57ffba91bfb5 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -21,6 +21,30 @@
#endif
#include <acpi/acpi.h>
+/* Table Handlers */
+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;
+};
+
+typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table);
+
+typedef int (*acpi_tbl_entry_handler)(union acpi_subtable_headers *header,
+ const unsigned long end);
+
+typedef int (*acpi_tbl_entry_handler_arg)(union acpi_subtable_headers *header,
+ void *arg, const unsigned long end);
+
+struct acpi_subtable_proc {
+ int id;
+ acpi_tbl_entry_handler handler;
+ acpi_tbl_entry_handler_arg handler_arg;
+ void *arg;
+ int count;
+};
+
#ifdef CONFIG_ACPI
#include <linux/list.h>
@@ -129,22 +153,6 @@ enum acpi_address_range_id {
};
-/* Table Handlers */
-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;
-};
-
-typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table);
-
-typedef int (*acpi_tbl_entry_handler)(union acpi_subtable_headers *header,
- const unsigned long end);
-
-typedef int (*acpi_tbl_entry_handler_arg)(union acpi_subtable_headers *header,
- void *arg, const unsigned long end);
-
/* Debugger support */
struct acpi_debugger_ops {
@@ -218,14 +226,6 @@ static inline int acpi_debugger_notify_command_complete(void)
(!entry) || (unsigned long)entry + sizeof(*entry) > end || \
((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
-struct acpi_subtable_proc {
- int id;
- acpi_tbl_entry_handler handler;
- acpi_tbl_entry_handler_arg handler_arg;
- void *arg;
- int count;
-};
-
void __iomem *__acpi_map_table(unsigned long phys, unsigned long size);
void __acpi_unmap_table(void __iomem *map, unsigned long size);
int early_acpi_boot_init(void);
@@ -1524,4 +1524,19 @@ static inline void acpi_device_notify(struct device *dev) { }
static inline void acpi_device_notify_remove(struct device *dev) { }
#endif
+#ifdef CONFIG_ACPI_TABLES_LIB
+int acpi_parse_entries_array(char *id, unsigned long table_size,
+ struct acpi_table_header *table_header,
+ struct acpi_subtable_proc *proc,
+ int proc_num, unsigned int max_entries);
+#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,
+ int proc_num, unsigned int max_entries)
+{
+ return -EOPNOTSUPP;
+}
+#endif
+
#endif /*_LINUX_ACPI_H*/
^ permalink raw reply related [flat|nested] 19+ messages in thread* RE: [PATCH v2 1/4] acpi: Move common tables helper functions to common lib
2023-05-18 18:33 ` [PATCH v2 1/4] acpi: Move common tables helper functions to common lib Dave Jiang
@ 2023-05-22 21:31 ` Dan Williams
2023-05-22 22:13 ` Dave Jiang
2023-06-01 14:50 ` Jonathan Cameron
1 sibling, 1 reply; 19+ messages in thread
From: Dan Williams @ 2023-05-22 21:31 UTC (permalink / raw)
To: Dave Jiang, linux-acpi, linux-cxl
Cc: rafael, lenb, dan.j.williams, ira.weiny, vishal.l.verma,
alison.schofield, lukas, Jonathan.Cameron
Dave Jiang wrote:
> Some of the routines in ACPI tables.c can be shared with parsing CDAT.
s,ACPI tables.c,driver/acpi/tables.c,
> However, CDAT is used by CXL and can exist on platforms that do not use
> ACPI.
Clarify that CDAT is not an ACPI table:
CDAT is a device-provided data structure that is formatted similar to a
platform-provided ACPI table.
> Split out the common routine from ACPI to accomodate platforms that
> do not support ACPI. The common routines can be built outside of ACPI if
> ACPI_TABLES_LIB is selected.
Might be just me but I get confused where this is indicating "ACPI" the
platform vs "CONFIG_ACPI" the code. How about just:
Refactor the table parsing routines in driver/acpi/tables.c into helpers
that can be shared with the CXL driver even in the CONFIG_ACPI=n case.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> drivers/Makefile | 2
> drivers/acpi/Kconfig | 4 +
> drivers/acpi/Makefile | 3 +
> drivers/acpi/tables.c | 173 ----------------------------------------
> drivers/acpi/tables_lib.c | 194 +++++++++++++++++++++++++++++++++++++++++++++
> include/linux/acpi.h | 63 +++++++++------
> 6 files changed, 241 insertions(+), 198 deletions(-)
> create mode 100644 drivers/acpi/tables_lib.c
Conversion looks ok to me. Even though the cover letter said "Hi Rafael,
Please consider these for 6.5 merge window" my expectation is to take
these through CXL with ACPI acks.
One question below:
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 20b118dca999..1824797f7dfe 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -31,7 +31,7 @@ obj-y += idle/
> # IPMI must come before ACPI in order to provide IPMI opregion support
> obj-y += char/ipmi/
>
> -obj-$(CONFIG_ACPI) += acpi/
> +obj-y += acpi/
>
> # PnP must come after ACPI since it will eventually need to check if acpi
> # was used and do nothing if so
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index ccbeab9500ec..ce74a20dc42f 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -6,12 +6,16 @@
> config ARCH_SUPPORTS_ACPI
> bool
>
> +config ACPI_TABLES_LIB
> + bool
> +
> menuconfig ACPI
> bool "ACPI (Advanced Configuration and Power Interface) Support"
> depends on ARCH_SUPPORTS_ACPI
> select PNP
> select NLS
> select CRC32
> + select ACPI_TABLES_LIB
> default y if X86
> help
> Advanced Configuration and Power Interface (ACPI) support for
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index feb36c0b9446..4558e2876823 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -13,6 +13,9 @@ tables.o: $(src)/../../include/$(CONFIG_ACPI_CUSTOM_DSDT_FILE) ;
>
> endif
>
> +obj-$(CONFIG_ACPI_TABLES_LIB) += acpi_tables_lib.o
> +acpi_tables_lib-y := tables_lib.o
Why is a separate object name needed?
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/4] acpi: Move common tables helper functions to common lib
2023-05-22 21:31 ` Dan Williams
@ 2023-05-22 22:13 ` Dave Jiang
2023-05-22 22:25 ` Dan Williams
0 siblings, 1 reply; 19+ messages in thread
From: Dave Jiang @ 2023-05-22 22:13 UTC (permalink / raw)
To: Dan Williams, linux-acpi, linux-cxl
Cc: rafael, lenb, ira.weiny, vishal.l.verma, alison.schofield, lukas,
Jonathan.Cameron
On 5/22/23 14:31, Dan Williams wrote:
> Dave Jiang wrote:
>> Some of the routines in ACPI tables.c can be shared with parsing CDAT.
> s,ACPI tables.c,driver/acpi/tables.c,
>
>> However, CDAT is used by CXL and can exist on platforms that do not use
>> ACPI.
> Clarify that CDAT is not an ACPI table:
>
> CDAT is a device-provided data structure that is formatted similar to a
> platform-provided ACPI table.
>
>> Split out the common routine from ACPI to accomodate platforms that
>> do not support ACPI. The common routines can be built outside of ACPI if
>> ACPI_TABLES_LIB is selected.
> Might be just me but I get confused where this is indicating "ACPI" the
> platform vs "CONFIG_ACPI" the code. How about just:
>
> Refactor the table parsing routines in driver/acpi/tables.c into helpers
> that can be shared with the CXL driver even in the CONFIG_ACPI=n case.
>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> drivers/Makefile | 2
>> drivers/acpi/Kconfig | 4 +
>> drivers/acpi/Makefile | 3 +
>> drivers/acpi/tables.c | 173 ----------------------------------------
>> drivers/acpi/tables_lib.c | 194 +++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/acpi.h | 63 +++++++++------
>> 6 files changed, 241 insertions(+), 198 deletions(-)
>> create mode 100644 drivers/acpi/tables_lib.c
> Conversion looks ok to me. Even though the cover letter said "Hi Rafael,
> Please consider these for 6.5 merge window" my expectation is to take
> these through CXL with ACPI acks.
I thought you wanted Rafael to take the ACPI patches. But going to the
CXL tree works.
>
> One question below:
>
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index 20b118dca999..1824797f7dfe 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -31,7 +31,7 @@ obj-y += idle/
>> # IPMI must come before ACPI in order to provide IPMI opregion support
>> obj-y += char/ipmi/
>>
>> -obj-$(CONFIG_ACPI) += acpi/
>> +obj-y += acpi/
>>
>> # PnP must come after ACPI since it will eventually need to check if acpi
>> # was used and do nothing if so
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index ccbeab9500ec..ce74a20dc42f 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -6,12 +6,16 @@
>> config ARCH_SUPPORTS_ACPI
>> bool
>>
>> +config ACPI_TABLES_LIB
>> + bool
>> +
>> menuconfig ACPI
>> bool "ACPI (Advanced Configuration and Power Interface) Support"
>> depends on ARCH_SUPPORTS_ACPI
>> select PNP
>> select NLS
>> select CRC32
>> + select ACPI_TABLES_LIB
>> default y if X86
>> help
>> Advanced Configuration and Power Interface (ACPI) support for
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index feb36c0b9446..4558e2876823 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -13,6 +13,9 @@ tables.o: $(src)/../../include/$(CONFIG_ACPI_CUSTOM_DSDT_FILE) ;
>>
>> endif
>>
>> +obj-$(CONFIG_ACPI_TABLES_LIB) += acpi_tables_lib.o
>> +acpi_tables_lib-y := tables_lib.o
> Why is a separate object name needed?
Not all code in tables.c will be shared. There are ACPI table parsing
specific code in tables.c that CXL does not care about. Or do you mean
just do
obj-$(CONFIG_ACPI_TABLES_LIB) += tables_lib.o
?
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/4] acpi: Move common tables helper functions to common lib
2023-05-22 22:13 ` Dave Jiang
@ 2023-05-22 22:25 ` Dan Williams
2023-05-23 10:38 ` Rafael J. Wysocki
0 siblings, 1 reply; 19+ messages in thread
From: Dan Williams @ 2023-05-22 22:25 UTC (permalink / raw)
To: Dave Jiang, Dan Williams, linux-acpi, linux-cxl
Cc: rafael, lenb, ira.weiny, vishal.l.verma, alison.schofield, lukas,
Jonathan.Cameron
Dave Jiang wrote:
>
> On 5/22/23 14:31, Dan Williams wrote:
> > Dave Jiang wrote:
> >> Some of the routines in ACPI tables.c can be shared with parsing CDAT.
> > s,ACPI tables.c,driver/acpi/tables.c,
> >
> >> However, CDAT is used by CXL and can exist on platforms that do not use
> >> ACPI.
> > Clarify that CDAT is not an ACPI table:
> >
> > CDAT is a device-provided data structure that is formatted similar to a
> > platform-provided ACPI table.
> >
> >> Split out the common routine from ACPI to accomodate platforms that
> >> do not support ACPI. The common routines can be built outside of ACPI if
> >> ACPI_TABLES_LIB is selected.
> > Might be just me but I get confused where this is indicating "ACPI" the
> > platform vs "CONFIG_ACPI" the code. How about just:
> >
> > Refactor the table parsing routines in driver/acpi/tables.c into helpers
> > that can be shared with the CXL driver even in the CONFIG_ACPI=n case.
> >
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >> ---
> >> drivers/Makefile | 2
> >> drivers/acpi/Kconfig | 4 +
> >> drivers/acpi/Makefile | 3 +
> >> drivers/acpi/tables.c | 173 ----------------------------------------
> >> drivers/acpi/tables_lib.c | 194 +++++++++++++++++++++++++++++++++++++++++++++
> >> include/linux/acpi.h | 63 +++++++++------
> >> 6 files changed, 241 insertions(+), 198 deletions(-)
> >> create mode 100644 drivers/acpi/tables_lib.c
> > Conversion looks ok to me. Even though the cover letter said "Hi Rafael,
> > Please consider these for 6.5 merge window" my expectation is to take
> > these through CXL with ACPI acks.
>
> I thought you wanted Rafael to take the ACPI patches. But going to the
> CXL tree works.
Ultimately up to Rafael. Either need a stable ACPI tree baseline to base
the CDAT work upon, or take this all through CXL.
>
> >
> > One question below:
> >
> >> diff --git a/drivers/Makefile b/drivers/Makefile
> >> index 20b118dca999..1824797f7dfe 100644
> >> --- a/drivers/Makefile
> >> +++ b/drivers/Makefile
> >> @@ -31,7 +31,7 @@ obj-y += idle/
> >> # IPMI must come before ACPI in order to provide IPMI opregion support
> >> obj-y += char/ipmi/
> >>
> >> -obj-$(CONFIG_ACPI) += acpi/
> >> +obj-y += acpi/
> >>
> >> # PnP must come after ACPI since it will eventually need to check if acpi
> >> # was used and do nothing if so
> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> >> index ccbeab9500ec..ce74a20dc42f 100644
> >> --- a/drivers/acpi/Kconfig
> >> +++ b/drivers/acpi/Kconfig
> >> @@ -6,12 +6,16 @@
> >> config ARCH_SUPPORTS_ACPI
> >> bool
> >>
> >> +config ACPI_TABLES_LIB
> >> + bool
> >> +
> >> menuconfig ACPI
> >> bool "ACPI (Advanced Configuration and Power Interface) Support"
> >> depends on ARCH_SUPPORTS_ACPI
> >> select PNP
> >> select NLS
> >> select CRC32
> >> + select ACPI_TABLES_LIB
> >> default y if X86
> >> help
> >> Advanced Configuration and Power Interface (ACPI) support for
> >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> >> index feb36c0b9446..4558e2876823 100644
> >> --- a/drivers/acpi/Makefile
> >> +++ b/drivers/acpi/Makefile
> >> @@ -13,6 +13,9 @@ tables.o: $(src)/../../include/$(CONFIG_ACPI_CUSTOM_DSDT_FILE) ;
> >>
> >> endif
> >>
> >> +obj-$(CONFIG_ACPI_TABLES_LIB) += acpi_tables_lib.o
> >> +acpi_tables_lib-y := tables_lib.o
> > Why is a separate object name needed?
>
> Not all code in tables.c will be shared. There are ACPI table parsing
> specific code in tables.c that CXL does not care about. Or do you mean
> just do
>
> obj-$(CONFIG_ACPI_TABLES_LIB) += tables_lib.o
Yes, this.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/4] acpi: Move common tables helper functions to common lib
2023-05-22 22:25 ` Dan Williams
@ 2023-05-23 10:38 ` Rafael J. Wysocki
0 siblings, 0 replies; 19+ messages in thread
From: Rafael J. Wysocki @ 2023-05-23 10:38 UTC (permalink / raw)
To: Dan Williams
Cc: Dave Jiang, linux-acpi, linux-cxl, rafael, lenb, ira.weiny,
vishal.l.verma, alison.schofield, lukas, Jonathan.Cameron
On Tue, May 23, 2023 at 12:25 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> Dave Jiang wrote:
> >
> > On 5/22/23 14:31, Dan Williams wrote:
> > > Dave Jiang wrote:
> > >> Some of the routines in ACPI tables.c can be shared with parsing CDAT.
> > > s,ACPI tables.c,driver/acpi/tables.c,
> > >
> > >> However, CDAT is used by CXL and can exist on platforms that do not use
> > >> ACPI.
> > > Clarify that CDAT is not an ACPI table:
> > >
> > > CDAT is a device-provided data structure that is formatted similar to a
> > > platform-provided ACPI table.
> > >
> > >> Split out the common routine from ACPI to accomodate platforms that
> > >> do not support ACPI. The common routines can be built outside of ACPI if
> > >> ACPI_TABLES_LIB is selected.
> > > Might be just me but I get confused where this is indicating "ACPI" the
> > > platform vs "CONFIG_ACPI" the code. How about just:
> > >
> > > Refactor the table parsing routines in driver/acpi/tables.c into helpers
> > > that can be shared with the CXL driver even in the CONFIG_ACPI=n case.
> > >
> > >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > >> ---
> > >> drivers/Makefile | 2
> > >> drivers/acpi/Kconfig | 4 +
> > >> drivers/acpi/Makefile | 3 +
> > >> drivers/acpi/tables.c | 173 ----------------------------------------
> > >> drivers/acpi/tables_lib.c | 194 +++++++++++++++++++++++++++++++++++++++++++++
> > >> include/linux/acpi.h | 63 +++++++++------
> > >> 6 files changed, 241 insertions(+), 198 deletions(-)
> > >> create mode 100644 drivers/acpi/tables_lib.c
> > > Conversion looks ok to me. Even though the cover letter said "Hi Rafael,
> > > Please consider these for 6.5 merge window" my expectation is to take
> > > these through CXL with ACPI acks.
> >
> > I thought you wanted Rafael to take the ACPI patches. But going to the
> > CXL tree works.
>
> Ultimately up to Rafael. Either need a stable ACPI tree baseline to base
> the CDAT work upon, or take this all through CXL.
AFAIAC, it can go in via the CXL tree, but see below.
> >
> > >
> > > One question below:
> > >
> > >> diff --git a/drivers/Makefile b/drivers/Makefile
> > >> index 20b118dca999..1824797f7dfe 100644
> > >> --- a/drivers/Makefile
> > >> +++ b/drivers/Makefile
> > >> @@ -31,7 +31,7 @@ obj-y += idle/
> > >> # IPMI must come before ACPI in order to provide IPMI opregion support
> > >> obj-y += char/ipmi/
> > >>
> > >> -obj-$(CONFIG_ACPI) += acpi/
> > >> +obj-y += acpi/
> > >>
> > >> # PnP must come after ACPI since it will eventually need to check if acpi
> > >> # was used and do nothing if so
> > >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > >> index ccbeab9500ec..ce74a20dc42f 100644
> > >> --- a/drivers/acpi/Kconfig
> > >> +++ b/drivers/acpi/Kconfig
> > >> @@ -6,12 +6,16 @@
> > >> config ARCH_SUPPORTS_ACPI
> > >> bool
> > >>
> > >> +config ACPI_TABLES_LIB
> > >> + bool
> > >> +
> > >> menuconfig ACPI
> > >> bool "ACPI (Advanced Configuration and Power Interface) Support"
> > >> depends on ARCH_SUPPORTS_ACPI
> > >> select PNP
> > >> select NLS
> > >> select CRC32
> > >> + select ACPI_TABLES_LIB
> > >> default y if X86
> > >> help
> > >> Advanced Configuration and Power Interface (ACPI) support for
> > >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> > >> index feb36c0b9446..4558e2876823 100644
> > >> --- a/drivers/acpi/Makefile
> > >> +++ b/drivers/acpi/Makefile
> > >> @@ -13,6 +13,9 @@ tables.o: $(src)/../../include/$(CONFIG_ACPI_CUSTOM_DSDT_FILE) ;
> > >>
> > >> endif
> > >>
> > >> +obj-$(CONFIG_ACPI_TABLES_LIB) += acpi_tables_lib.o
> > >> +acpi_tables_lib-y := tables_lib.o
> > > Why is a separate object name needed?
> >
> > Not all code in tables.c will be shared. There are ACPI table parsing
> > specific code in tables.c that CXL does not care about. Or do you mean
> > just do
> >
> > obj-$(CONFIG_ACPI_TABLES_LIB) += tables_lib.o
>
> Yes, this.
First, this depends on what is there in tables_lib.c. IMV it should
not contain stuff that is not needed for CDAT parsing.
Second, I'm not sure if drivers/acpi/ is the most appropriate location
for it, maybe lib/ would be less confusing?
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/4] acpi: Move common tables helper functions to common lib
2023-05-18 18:33 ` [PATCH v2 1/4] acpi: Move common tables helper functions to common lib Dave Jiang
2023-05-22 21:31 ` Dan Williams
@ 2023-06-01 14:50 ` Jonathan Cameron
2023-06-01 15:38 ` Dave Jiang
1 sibling, 1 reply; 19+ messages in thread
From: Jonathan Cameron @ 2023-06-01 14:50 UTC (permalink / raw)
To: Dave Jiang
Cc: linux-acpi, linux-cxl, rafael, lenb, dan.j.williams, ira.weiny,
vishal.l.verma, alison.schofield, lukas
On Thu, 18 May 2023 11:33:02 -0700
Dave Jiang <dave.jiang@intel.com> wrote:
> Some of the routines in ACPI tables.c can be shared with parsing CDAT.
> However, CDAT is used by CXL and can exist on platforms that do not use
> ACPI. Split out the common routine from ACPI to accomodate platforms that
> do not support ACPI. The common routines can be built outside of ACPI if
> ACPI_TABLES_LIB is selected.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Comment inline - otherwise looks fine to me.
Jonathan
> diff --git a/drivers/acpi/tables_lib.c b/drivers/acpi/tables_lib.c
> new file mode 100644
> index 000000000000..701001610fa9
> --- /dev/null
> +++ b/drivers/acpi/tables_lib.c
> @@ -0,0 +1,194 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * acpi_tables.c - ACPI Boot-Time Table Parsing
> + *
> + * Copyright (C) 2001 Paul Diefenbaugh <paul.s.diefenbaugh@intel.com>
> + */
> +
> +/* Uncomment next line to get verbose printout */
> +/* #define DEBUG */
> +#define pr_fmt(fmt) "ACPI: " fmt
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/smp.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/irq.h>
Check these includes are all needed by this subset of the
original file.
Also could take opportunity to put what is left in
alphabetical order or some other convention.
> +#include <linux/errno.h>
> +#include <linux/acpi.h>
> +#include <linux/memblock.h>
> +#include <linux/earlycpio.h>
> +#include <linux/initrd.h>
> +#include <linux/security.h>
> +#include <linux/kmemleak.h>
...
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/4] acpi: Move common tables helper functions to common lib
2023-06-01 14:50 ` Jonathan Cameron
@ 2023-06-01 15:38 ` Dave Jiang
0 siblings, 0 replies; 19+ messages in thread
From: Dave Jiang @ 2023-06-01 15:38 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-acpi, linux-cxl, rafael, lenb, dan.j.williams, ira.weiny,
vishal.l.verma, alison.schofield, lukas
On 6/1/23 07:50, Jonathan Cameron wrote:
> On Thu, 18 May 2023 11:33:02 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
>
>> Some of the routines in ACPI tables.c can be shared with parsing CDAT.
>> However, CDAT is used by CXL and can exist on platforms that do not use
>> ACPI. Split out the common routine from ACPI to accomodate platforms that
>> do not support ACPI. The common routines can be built outside of ACPI if
>> ACPI_TABLES_LIB is selected.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Comment inline - otherwise looks fine to me.
>
> Jonathan
>
>> diff --git a/drivers/acpi/tables_lib.c b/drivers/acpi/tables_lib.c
>> new file mode 100644
>> index 000000000000..701001610fa9
>> --- /dev/null
>> +++ b/drivers/acpi/tables_lib.c
>> @@ -0,0 +1,194 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * acpi_tables.c - ACPI Boot-Time Table Parsing
>> + *
>> + * Copyright (C) 2001 Paul Diefenbaugh <paul.s.diefenbaugh@intel.com>
>> + */
>> +
>> +/* Uncomment next line to get verbose printout */
>> +/* #define DEBUG */
>> +#define pr_fmt(fmt) "ACPI: " fmt
>> +
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/smp.h>
>> +#include <linux/string.h>
>> +#include <linux/types.h>
>> +#include <linux/irq.h>
> Check these includes are all needed by this subset of the
> original file.
>
> Also could take opportunity to put what is left in
> alphabetical order or some other convention.
Sure I'll clean that up.
>
>
>> +#include <linux/errno.h>
>> +#include <linux/acpi.h>
>> +#include <linux/memblock.h>
>> +#include <linux/earlycpio.h>
>> +#include <linux/initrd.h>
>> +#include <linux/security.h>
>> +#include <linux/kmemleak.h>
> ...
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 2/4] acpi: tables: Add CDAT table parsing support
2023-05-18 18:32 [PATCH v2 0/4] acpi: Add CDAT parsing support to ACPI tables code Dave Jiang
2023-05-18 18:33 ` [PATCH v2 1/4] acpi: Move common tables helper functions to common lib Dave Jiang
@ 2023-05-18 18:33 ` Dave Jiang
2023-05-22 23:12 ` Dan Williams
2023-05-18 18:33 ` [PATCH v2 3/4] acpi: fix misnamed define for CDAT DSMAS Dave Jiang
2023-05-18 18:33 ` [PATCH v2 4/4] acpi: Add defines for CDAT SSLBIS Dave Jiang
3 siblings, 1 reply; 19+ messages in thread
From: Dave Jiang @ 2023-05-18 18:33 UTC (permalink / raw)
To: linux-acpi, linux-cxl
Cc: Rafael J. Wysocki, Len Brown, rafael, lenb, dan.j.williams,
ira.weiny, vishal.l.verma, alison.schofield, lukas,
Jonathan.Cameron
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,
+ 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*/
^ permalink raw reply related [flat|nested] 19+ messages in thread* RE: [PATCH v2 2/4] acpi: tables: Add CDAT table parsing support
2023-05-18 18:33 ` [PATCH v2 2/4] acpi: tables: Add CDAT table parsing support Dave Jiang
@ 2023-05-22 23:12 ` Dan Williams
2023-05-23 10:43 ` Rafael J. Wysocki
0 siblings, 1 reply; 19+ messages in thread
From: Dan Williams @ 2023-05-22 23:12 UTC (permalink / raw)
To: Dave Jiang, linux-acpi, linux-cxl
Cc: Rafael J. Wysocki, Len Brown, dan.j.williams, ira.weiny,
vishal.l.verma, alison.schofield, lukas, Jonathan.Cameron
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;
}
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2 2/4] acpi: tables: Add CDAT table parsing support
2023-05-22 23:12 ` Dan Williams
@ 2023-05-23 10:43 ` Rafael J. Wysocki
0 siblings, 0 replies; 19+ messages in thread
From: Rafael J. Wysocki @ 2023-05-23 10:43 UTC (permalink / raw)
To: Dan Williams
Cc: Dave Jiang, linux-acpi, linux-cxl, Rafael J. Wysocki, Len Brown,
ira.weiny, vishal.l.verma, alison.schofield, lukas,
Jonathan.Cameron
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 3/4] acpi: fix misnamed define for CDAT DSMAS
2023-05-18 18:32 [PATCH v2 0/4] acpi: Add CDAT parsing support to ACPI tables code Dave Jiang
2023-05-18 18:33 ` [PATCH v2 1/4] acpi: Move common tables helper functions to common lib Dave Jiang
2023-05-18 18:33 ` [PATCH v2 2/4] acpi: tables: Add CDAT table parsing support Dave Jiang
@ 2023-05-18 18:33 ` Dave Jiang
2023-05-22 23:23 ` Dan Williams
2023-05-18 18:33 ` [PATCH v2 4/4] acpi: Add defines for CDAT SSLBIS Dave Jiang
3 siblings, 1 reply; 19+ messages in thread
From: Dave Jiang @ 2023-05-18 18:33 UTC (permalink / raw)
To: linux-acpi, linux-cxl
Cc: Rafael J. Wysocki, Len Brown, rafael, lenb, dan.j.williams,
ira.weiny, vishal.l.verma, alison.schofield, lukas,
Jonathan.Cameron
ACPI_CEDT_DSMAS_NON_VOLATILE should be defined as
ACPI_CDAT_DSMAS_NON_VOLATILE. Fix misspelled define.
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
See https://github.com/acpica/acpica/pull/874
Merged
---
include/acpi/actbl1.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
index 81b9e794424d..15df363b9144 100644
--- a/include/acpi/actbl1.h
+++ b/include/acpi/actbl1.h
@@ -356,7 +356,7 @@ struct acpi_cdat_dsmas {
/* Flags for subtable above */
-#define ACPI_CEDT_DSMAS_NON_VOLATILE (1 << 2)
+#define ACPI_CDAT_DSMAS_NON_VOLATILE (1 << 2)
/* Subtable 1: Device scoped Latency and Bandwidth Information Structure (DSLBIS) */
^ permalink raw reply related [flat|nested] 19+ messages in thread* RE: [PATCH v2 3/4] acpi: fix misnamed define for CDAT DSMAS
2023-05-18 18:33 ` [PATCH v2 3/4] acpi: fix misnamed define for CDAT DSMAS Dave Jiang
@ 2023-05-22 23:23 ` Dan Williams
2023-05-23 10:46 ` Rafael J. Wysocki
0 siblings, 1 reply; 19+ messages in thread
From: Dan Williams @ 2023-05-22 23:23 UTC (permalink / raw)
To: Dave Jiang, linux-acpi, linux-cxl
Cc: Rafael J. Wysocki, Len Brown, dan.j.williams, ira.weiny,
vishal.l.verma, alison.schofield, lukas, Jonathan.Cameron
Dave Jiang wrote:
> ACPI_CEDT_DSMAS_NON_VOLATILE should be defined as
> ACPI_CDAT_DSMAS_NON_VOLATILE. Fix misspelled define.
>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Len Brown <lenb@kernel.org>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>
> ---
> See https://github.com/acpica/acpica/pull/874
> Merged
> ---
> include/acpi/actbl1.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
> index 81b9e794424d..15df363b9144 100644
> --- a/include/acpi/actbl1.h
> +++ b/include/acpi/actbl1.h
> @@ -356,7 +356,7 @@ struct acpi_cdat_dsmas {
>
> /* Flags for subtable above */
>
> -#define ACPI_CEDT_DSMAS_NON_VOLATILE (1 << 2)
> +#define ACPI_CDAT_DSMAS_NON_VOLATILE (1 << 2)
This needs to come in through an ACPICA update. If that is going to happen
this cycle, great, if not then I would handle it as a merge fixup after
the fact. I.e. just live with the misspelling to keep this patch set
ACPICA unencumbered, unless Rafael has different thoughts.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 3/4] acpi: fix misnamed define for CDAT DSMAS
2023-05-22 23:23 ` Dan Williams
@ 2023-05-23 10:46 ` Rafael J. Wysocki
2023-05-23 14:54 ` Dave Jiang
0 siblings, 1 reply; 19+ messages in thread
From: Rafael J. Wysocki @ 2023-05-23 10:46 UTC (permalink / raw)
To: Dan Williams
Cc: Dave Jiang, linux-acpi, linux-cxl, Rafael J. Wysocki, Len Brown,
ira.weiny, vishal.l.verma, alison.schofield, lukas,
Jonathan.Cameron
On Tue, May 23, 2023 at 1:24 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> Dave Jiang wrote:
> > ACPI_CEDT_DSMAS_NON_VOLATILE should be defined as
> > ACPI_CDAT_DSMAS_NON_VOLATILE. Fix misspelled define.
> >
> > Cc: Rafael J. Wysocki <rafael@kernel.org>
> > Cc: Len Brown <lenb@kernel.org>
> > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >
> > ---
> > See https://github.com/acpica/acpica/pull/874
> > Merged
> > ---
> > include/acpi/actbl1.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
> > index 81b9e794424d..15df363b9144 100644
> > --- a/include/acpi/actbl1.h
> > +++ b/include/acpi/actbl1.h
> > @@ -356,7 +356,7 @@ struct acpi_cdat_dsmas {
> >
> > /* Flags for subtable above */
> >
> > -#define ACPI_CEDT_DSMAS_NON_VOLATILE (1 << 2)
> > +#define ACPI_CDAT_DSMAS_NON_VOLATILE (1 << 2)
>
> This needs to come in through an ACPICA update. If that is going to happen
> this cycle, great, if not then I would handle it as a merge fixup after
> the fact. I.e. just live with the misspelling to keep this patch set
> ACPICA unencumbered, unless Rafael has different thoughts.
You can also submit an upstream ACPICA pull request with this change
and resend the patch with a Link tag pointing to that PR. Then we'll
decide how to handle it.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 3/4] acpi: fix misnamed define for CDAT DSMAS
2023-05-23 10:46 ` Rafael J. Wysocki
@ 2023-05-23 14:54 ` Dave Jiang
2023-05-23 15:16 ` Rafael J. Wysocki
0 siblings, 1 reply; 19+ messages in thread
From: Dave Jiang @ 2023-05-23 14:54 UTC (permalink / raw)
To: Rafael J. Wysocki, Dan Williams
Cc: linux-acpi, linux-cxl, Len Brown, ira.weiny, vishal.l.verma,
alison.schofield, lukas, Jonathan.Cameron
On 5/23/23 03:46, Rafael J. Wysocki wrote:
> On Tue, May 23, 2023 at 1:24 AM Dan Williams <dan.j.williams@intel.com> wrote:
>> Dave Jiang wrote:
>>> ACPI_CEDT_DSMAS_NON_VOLATILE should be defined as
>>> ACPI_CDAT_DSMAS_NON_VOLATILE. Fix misspelled define.
>>>
>>> Cc: Rafael J. Wysocki <rafael@kernel.org>
>>> Cc: Len Brown <lenb@kernel.org>
>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>
>>> ---
>>> See https://github.com/acpica/acpica/pull/874
>>> Merged
>>> ---
>>> include/acpi/actbl1.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
>>> index 81b9e794424d..15df363b9144 100644
>>> --- a/include/acpi/actbl1.h
>>> +++ b/include/acpi/actbl1.h
>>> @@ -356,7 +356,7 @@ struct acpi_cdat_dsmas {
>>>
>>> /* Flags for subtable above */
>>>
>>> -#define ACPI_CEDT_DSMAS_NON_VOLATILE (1 << 2)
>>> +#define ACPI_CDAT_DSMAS_NON_VOLATILE (1 << 2)
>> This needs to come in through an ACPICA update. If that is going to happen
>> this cycle, great, if not then I would handle it as a merge fixup after
>> the fact. I.e. just live with the misspelling to keep this patch set
>> ACPICA unencumbered, unless Rafael has different thoughts.
> You can also submit an upstream ACPICA pull request with this change
> and resend the patch with a Link tag pointing to that PR. Then we'll
> decide how to handle it.
That's been done and merged. I'll include the Link tag in the next rev.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 3/4] acpi: fix misnamed define for CDAT DSMAS
2023-05-23 14:54 ` Dave Jiang
@ 2023-05-23 15:16 ` Rafael J. Wysocki
0 siblings, 0 replies; 19+ messages in thread
From: Rafael J. Wysocki @ 2023-05-23 15:16 UTC (permalink / raw)
To: Dave Jiang
Cc: Rafael J. Wysocki, Dan Williams, linux-acpi, linux-cxl, Len Brown,
ira.weiny, vishal.l.verma, alison.schofield, lukas,
Jonathan.Cameron
On Tue, May 23, 2023 at 4:55 PM Dave Jiang <dave.jiang@intel.com> wrote:
>
>
> On 5/23/23 03:46, Rafael J. Wysocki wrote:
> > On Tue, May 23, 2023 at 1:24 AM Dan Williams <dan.j.williams@intel.com> wrote:
> >> Dave Jiang wrote:
> >>> ACPI_CEDT_DSMAS_NON_VOLATILE should be defined as
> >>> ACPI_CDAT_DSMAS_NON_VOLATILE. Fix misspelled define.
> >>>
> >>> Cc: Rafael J. Wysocki <rafael@kernel.org>
> >>> Cc: Len Brown <lenb@kernel.org>
> >>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >>>
> >>> ---
> >>> See https://github.com/acpica/acpica/pull/874
> >>> Merged
> >>> ---
> >>> include/acpi/actbl1.h | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
> >>> index 81b9e794424d..15df363b9144 100644
> >>> --- a/include/acpi/actbl1.h
> >>> +++ b/include/acpi/actbl1.h
> >>> @@ -356,7 +356,7 @@ struct acpi_cdat_dsmas {
> >>>
> >>> /* Flags for subtable above */
> >>>
> >>> -#define ACPI_CEDT_DSMAS_NON_VOLATILE (1 << 2)
> >>> +#define ACPI_CDAT_DSMAS_NON_VOLATILE (1 << 2)
> >> This needs to come in through an ACPICA update. If that is going to happen
> >> this cycle, great, if not then I would handle it as a merge fixup after
> >> the fact. I.e. just live with the misspelling to keep this patch set
> >> ACPICA unencumbered, unless Rafael has different thoughts.
> > You can also submit an upstream ACPICA pull request with this change
> > and resend the patch with a Link tag pointing to that PR. Then we'll
> > decide how to handle it.
>
> That's been done and merged. I'll include the Link tag in the next rev.
If it's merged, then the Linux patch can be applied without waiting
for an ACPICA release (which should take place in the near future
anyway).
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 4/4] acpi: Add defines for CDAT SSLBIS
2023-05-18 18:32 [PATCH v2 0/4] acpi: Add CDAT parsing support to ACPI tables code Dave Jiang
` (2 preceding siblings ...)
2023-05-18 18:33 ` [PATCH v2 3/4] acpi: fix misnamed define for CDAT DSMAS Dave Jiang
@ 2023-05-18 18:33 ` Dave Jiang
2023-05-22 23:24 ` Dan Williams
3 siblings, 1 reply; 19+ messages in thread
From: Dave Jiang @ 2023-05-18 18:33 UTC (permalink / raw)
To: linux-acpi, linux-cxl
Cc: rafael, lenb, dan.j.williams, ira.weiny, vishal.l.verma,
alison.schofield, lukas, Jonathan.Cameron
Add upstream port and any port definition for SSLBIS.
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
See https://github.com/acpica/acpica/pull/874
Merged
---
include/acpi/actbl1.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
index 15df363b9144..c6189fafe87f 100644
--- a/include/acpi/actbl1.h
+++ b/include/acpi/actbl1.h
@@ -419,6 +419,9 @@ struct acpi_cdat_sslbe {
u16 reserved;
};
+#define ACPI_CDAT_SSLBIS_US_PORT 0x0100
+#define ACPI_CDAT_SSLBIS_ANY_PORT 0xffff
+
/*******************************************************************************
*
* CEDT - CXL Early Discovery Table
^ permalink raw reply related [flat|nested] 19+ messages in thread* RE: [PATCH v2 4/4] acpi: Add defines for CDAT SSLBIS
2023-05-18 18:33 ` [PATCH v2 4/4] acpi: Add defines for CDAT SSLBIS Dave Jiang
@ 2023-05-22 23:24 ` Dan Williams
2023-05-23 10:49 ` Rafael J. Wysocki
0 siblings, 1 reply; 19+ messages in thread
From: Dan Williams @ 2023-05-22 23:24 UTC (permalink / raw)
To: Dave Jiang, linux-acpi, linux-cxl
Cc: rafael, lenb, dan.j.williams, ira.weiny, vishal.l.verma,
alison.schofield, lukas, Jonathan.Cameron
Dave Jiang wrote:
> Add upstream port and any port definition for SSLBIS.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>
> ---
> See https://github.com/acpica/acpica/pull/874
> Merged
> ---
> include/acpi/actbl1.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
> index 15df363b9144..c6189fafe87f 100644
> --- a/include/acpi/actbl1.h
> +++ b/include/acpi/actbl1.h
> @@ -419,6 +419,9 @@ struct acpi_cdat_sslbe {
> u16 reserved;
> };
>
> +#define ACPI_CDAT_SSLBIS_US_PORT 0x0100
> +#define ACPI_CDAT_SSLBIS_ANY_PORT 0xffff
> +
> /*******************************************************************************
> *
> * CEDT - CXL Early Discovery Table
Similar to the last patch, just define these locally outside of
include/acpi/actbl1.h in the patch that needs them and drop the ACPICA
entanglement.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 4/4] acpi: Add defines for CDAT SSLBIS
2023-05-22 23:24 ` Dan Williams
@ 2023-05-23 10:49 ` Rafael J. Wysocki
0 siblings, 0 replies; 19+ messages in thread
From: Rafael J. Wysocki @ 2023-05-23 10:49 UTC (permalink / raw)
To: Dan Williams
Cc: Dave Jiang, linux-acpi, linux-cxl, rafael, lenb, ira.weiny,
vishal.l.verma, alison.schofield, lukas, Jonathan.Cameron
On Tue, May 23, 2023 at 1:25 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> Dave Jiang wrote:
> > Add upstream port and any port definition for SSLBIS.
> >
> > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >
> > ---
> > See https://github.com/acpica/acpica/pull/874
> > Merged
> > ---
> > include/acpi/actbl1.h | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
> > index 15df363b9144..c6189fafe87f 100644
> > --- a/include/acpi/actbl1.h
> > +++ b/include/acpi/actbl1.h
> > @@ -419,6 +419,9 @@ struct acpi_cdat_sslbe {
> > u16 reserved;
> > };
> >
> > +#define ACPI_CDAT_SSLBIS_US_PORT 0x0100
> > +#define ACPI_CDAT_SSLBIS_ANY_PORT 0xffff
> > +
> > /*******************************************************************************
> > *
> > * CEDT - CXL Early Discovery Table
>
> Similar to the last patch, just define these locally outside of
> include/acpi/actbl1.h in the patch that needs them and drop the ACPICA
> entanglement.
Yes, if they are not used anywhere else, it's better to do that.
The local definitions can be dropped later when upstream ACPICA acquires them.
That said, it would be prudent to submit an upstream ACPICA pull
request adding them too.
^ permalink raw reply [flat|nested] 19+ messages in thread