Re: [PATCH v2 1/4] acpi: Move common tables helper functions to common lib

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

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.