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

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

 



On Tue, May 23, 2023 at 12:25 AM Dan Williams <dan.j.williams@xxxxxxxxx> 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@xxxxxxxxx>
> > >> ---
> > >>   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?



[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