From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
To: Dave Jiang <dave.jiang@intel.com>
Cc: <linux-cxl@vger.kernel.org>, <dave@stgolabs.net>,
<alison.schofield@intel.com>, <vishal.l.verma@intel.com>,
<ira.weiny@intel.com>, <dan.j.williams@intel.com>,
"Rafael J. Wysocki" <rafael@kernel.org>,
Len Brown <lenb@kernel.org>, <linux-acpi@vger.kernel.org>
Subject: Re: [PATCH] cxl: Remove core/acpi.c and cxl core dependency on ACPI
Date: Mon, 9 Jun 2025 11:38:36 +0100 [thread overview]
Message-ID: <20250609113836.0000558a@huawei.com> (raw)
In-Reply-To: <20250605001148.1698633-1-dave.jiang@intel.com>
On Wed, 4 Jun 2025 17:11:47 -0700
Dave Jiang <dave.jiang@intel.com> wrote:
> It was a mistake to introduce core/acpi.c and putting ACPI dependency on
> cxl_core when adding the extended linear cache support. Add a callback
> in the cxl_root_decoder to retrieve the extended linear cache size from
> ACPI via the cxl_acpi driver.
>
> In order to deal with cxl_test, a device parameter had to be introduced
> to the hmat_get_extended_linear_cache_size() call in order to help with
> the mock wrapped function from ACPI. Even though the 'struct device' is
> not used by the actual hmat_get_extended_linear_cache_size() function.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> drivers/acpi/numa/hmat.c | 4 +++-
> drivers/cxl/acpi.c | 15 ++++++++++++++-
> drivers/cxl/core/Makefile | 1 -
> drivers/cxl/core/acpi.c | 11 -----------
> drivers/cxl/core/core.h | 2 --
> drivers/cxl/core/port.c | 5 ++++-
> drivers/cxl/core/region.c | 6 +++++-
> drivers/cxl/cxl.h | 12 +++++++++++-
> include/linux/acpi.h | 6 ++++--
> tools/testing/cxl/Kbuild | 2 +-
> tools/testing/cxl/test/cxl.c | 20 ++++++++++++++++++++
> tools/testing/cxl/test/mock.c | 23 +++++++++++++++++++++++
> tools/testing/cxl/test/mock.h | 4 ++++
> 13 files changed, 89 insertions(+), 22 deletions(-)
> delete mode 100644 drivers/cxl/core/acpi.c
>
> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> index 9d9052258e92..bc8441dc7fe2 100644
> --- a/drivers/acpi/numa/hmat.c
> +++ b/drivers/acpi/numa/hmat.c
> @@ -110,6 +110,7 @@ static struct memory_target *find_mem_target(unsigned int mem_pxm)
>
> /**
> * hmat_get_extended_linear_cache_size - Retrieve the extended linear cache size
> + * @dev: device for debug output
This seems misleading given it isn't used for that. Hmm. I guess we don't really want
to shout that we need it for stubbing.
I think we need acpi maintainer buy in for this. +CC linux-acpi, Len and Rafael.
> * @backing_res: resource from the backing media
> * @nid: node id for the memory region
> * @cache_size: (Output) size of extended linear cache.
> @@ -117,7 +118,8 @@ static struct memory_target *find_mem_target(unsigned int mem_pxm)
> * Return: 0 on success. Errno on failure.
> *
> */
> -int hmat_get_extended_linear_cache_size(struct resource *backing_res, int nid,
> +int hmat_get_extended_linear_cache_size(struct device *dev,
> + struct resource *backing_res, int nid,
> resource_size_t *cache_size)
> {
> unsigned int pxm = node_to_pxm(nid);
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index cb14829bb9be..7076d471ada5 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -337,6 +337,19 @@ static int add_or_reset_cxl_resource(struct resource *parent, struct resource *r
> return rc;
> }
>
> +static int cxl_acpi_get_extended_linear_cache_size(struct cxl_root_decoder *cxlrd,
> + struct resource *backing_res,
> + int nid,
> + resource_size_t *size)
> +{
> + return hmat_get_extended_linear_cache_size(&cxlrd->cxlsd.cxld.dev,
> + backing_res, nid, size);
> +}
> +
> +static const struct cxl_rcd_ops acpi_rcd_ops = {
> + .get_extended_linear_cache_size = cxl_acpi_get_extended_linear_cache_size,
> +};
> +
> DEFINE_FREE(put_cxlrd, struct cxl_root_decoder *,
> if (!IS_ERR_OR_NULL(_T)) put_device(&_T->cxlsd.cxld.dev))
> DEFINE_FREE(del_cxl_resource, struct resource *, if (_T) del_cxl_resource(_T))
> @@ -375,7 +388,7 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
> return rc;
>
> struct cxl_root_decoder *cxlrd __free(put_cxlrd) =
> - cxl_root_decoder_alloc(root_port, ways);
> + cxl_root_decoder_alloc(root_port, ways, &acpi_rcd_ops);
>
> if (IS_ERR(cxlrd))
> return PTR_ERR(cxlrd);
> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> index 086df97a0fcf..f67e879dbf9a 100644
> --- a/drivers/cxl/core/Makefile
> +++ b/drivers/cxl/core/Makefile
> @@ -15,7 +15,6 @@ cxl_core-y += hdm.o
> cxl_core-y += pmu.o
> cxl_core-y += cdat.o
> cxl_core-y += ras.o
> -cxl_core-y += acpi.o
> cxl_core-$(CONFIG_TRACING) += trace.o
> cxl_core-$(CONFIG_CXL_REGION) += region.o
> cxl_core-$(CONFIG_CXL_MCE) += mce.o
> diff --git a/drivers/cxl/core/acpi.c b/drivers/cxl/core/acpi.c
> deleted file mode 100644
> index f13b4dae6ac5..000000000000
> --- a/drivers/cxl/core/acpi.c
> +++ /dev/null
> @@ -1,11 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-only
> -/* Copyright(c) 2024 Intel Corporation. All rights reserved. */
> -#include <linux/acpi.h>
> -#include "cxl.h"
> -#include "core.h"
> -
> -int cxl_acpi_get_extended_linear_cache_size(struct resource *backing_res,
> - int nid, resource_size_t *size)
> -{
> - return hmat_get_extended_linear_cache_size(backing_res, nid, size);
> -}
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 17b692eb3257..719cee0de1ec 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -120,8 +120,6 @@ int cxl_port_get_switch_dport_bandwidth(struct cxl_port *port,
> int cxl_ras_init(void);
> void cxl_ras_exit(void);
> int cxl_gpf_port_setup(struct cxl_dport *dport);
> -int cxl_acpi_get_extended_linear_cache_size(struct resource *backing_res,
> - int nid, resource_size_t *size);
>
> #ifdef CONFIG_CXL_FEATURES
> size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 726bd4a7de27..89b4cdc2bd8c 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1800,6 +1800,7 @@ static int cxl_switch_decoder_init(struct cxl_port *port,
> * cxl_root_decoder_alloc - Allocate a root level decoder
> * @port: owning CXL root of this decoder
> * @nr_targets: static number of downstream targets
> + * @ops: root decoder callback operations
> *
> * Return: A new cxl decoder to be registered by cxl_decoder_add(). A
> * 'CXL root' decoder is one that decodes from a top-level / static platform
> @@ -1807,7 +1808,8 @@ static int cxl_switch_decoder_init(struct cxl_port *port,
> * topology.
> */
> struct cxl_root_decoder *cxl_root_decoder_alloc(struct cxl_port *port,
> - unsigned int nr_targets)
> + unsigned int nr_targets,
> + const struct cxl_rcd_ops *ops)
> {
> struct cxl_root_decoder *cxlrd;
> struct cxl_switch_decoder *cxlsd;
> @@ -1846,6 +1848,7 @@ struct cxl_root_decoder *cxl_root_decoder_alloc(struct cxl_port *port,
>
> atomic_set(&cxlrd->region_id, rc);
> cxlrd->qos_class = CXL_QOS_CLASS_INVALID;
> + cxlrd->ops = ops;
> return cxlrd;
> }
> EXPORT_SYMBOL_NS_GPL(cxl_root_decoder_alloc, "CXL");
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index c3f4dc244df7..ac05fe7335c9 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3239,7 +3239,11 @@ static int cxl_extended_linear_cache_resize(struct cxl_region *cxlr,
> resource_size_t cache_size, start;
> int rc;
>
> - rc = cxl_acpi_get_extended_linear_cache_size(res, nid, &cache_size);
> + if (!cxlrd->ops || !cxlrd->ops->get_extended_linear_cache_size)
> + return -EOPNOTSUPP;
> +
> + rc = cxlrd->ops->get_extended_linear_cache_size(cxlrd, res, nid,
> + &cache_size);
> if (rc)
> return rc;
>
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index a9ab46eb0610..899092835e04 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -420,6 +420,12 @@ struct cxl_switch_decoder {
> struct cxl_root_decoder;
> typedef u64 (*cxl_hpa_to_spa_fn)(struct cxl_root_decoder *cxlrd, u64 hpa);
>
> +struct cxl_rcd_ops {
> + int (*get_extended_linear_cache_size)(struct cxl_root_decoder *cxlrd,
> + struct resource *backing_res,
> + int nid, resource_size_t *size);
> +};
> +
> /**
> * struct cxl_root_decoder - Static platform CXL address decoder
> * @res: host / parent resource for region allocations
> @@ -428,6 +434,7 @@ typedef u64 (*cxl_hpa_to_spa_fn)(struct cxl_root_decoder *cxlrd, u64 hpa);
> * @platform_data: platform specific configuration data
> * @range_lock: sync region autodiscovery by address range
> * @qos_class: QoS performance class cookie
> + * @ops: root decoder specific operations
> * @cxlsd: base cxl switch decoder
> */
> struct cxl_root_decoder {
> @@ -437,7 +444,9 @@ struct cxl_root_decoder {
> void *platform_data;
> struct mutex range_lock;
> int qos_class;
> + const struct cxl_rcd_ops *ops;
> struct cxl_switch_decoder cxlsd;
> + /* DO NOT ADD ANYTHING AFTER THIS LINE. cxlsd trails with a flex array */
Valid change, but not part of this patch.
> };
>
> /*
> @@ -772,7 +781,8 @@ bool is_root_decoder(struct device *dev);
> bool is_switch_decoder(struct device *dev);
> bool is_endpoint_decoder(struct device *dev);
> struct cxl_root_decoder *cxl_root_decoder_alloc(struct cxl_port *port,
> - unsigned int nr_targets);
> + unsigned int nr_targets,
> + const struct cxl_rcd_ops *ops);
> struct cxl_switch_decoder *cxl_switch_decoder_alloc(struct cxl_port *port,
> unsigned int nr_targets);
> int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map);
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 3f2e93ed9730..507aa15913d2 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1095,10 +1095,12 @@ static inline acpi_handle acpi_get_processor_handle(int cpu)
> #endif /* !CONFIG_ACPI */
>
> #ifdef CONFIG_ACPI_HMAT
> -int hmat_get_extended_linear_cache_size(struct resource *backing_res, int nid,
> +int hmat_get_extended_linear_cache_size(struct device *dev,
> + struct resource *backing_res, int nid,
> resource_size_t *size);
> #else
> -static inline int hmat_get_extended_linear_cache_size(struct resource *backing_res,
> +static inline int hmat_get_extended_linear_cache_size(struct device *dev,
> + struct resource *backing_res,
> int nid, resource_size_t *size)
> {
> return -EOPNOTSUPP;
> diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
> index 387f3df8b988..3e6e3c4ab797 100644
> --- a/tools/testing/cxl/Kbuild
> +++ b/tools/testing/cxl/Kbuild
> @@ -15,6 +15,7 @@ ldflags-y += --wrap=devm_cxl_add_rch_dport
> ldflags-y += --wrap=cxl_rcd_component_reg_phys
> ldflags-y += --wrap=cxl_endpoint_parse_cdat
> ldflags-y += --wrap=cxl_dport_init_ras_reporting
> +ldflags-y += --wrap=hmat_get_extended_linear_cache_size
>
> DRIVERS := ../../../drivers
> CXL_SRC := $(DRIVERS)/cxl
> @@ -62,7 +63,6 @@ cxl_core-y += $(CXL_CORE_SRC)/hdm.o
> cxl_core-y += $(CXL_CORE_SRC)/pmu.o
> cxl_core-y += $(CXL_CORE_SRC)/cdat.o
> cxl_core-y += $(CXL_CORE_SRC)/ras.o
> -cxl_core-y += $(CXL_CORE_SRC)/acpi.o
> cxl_core-$(CONFIG_TRACING) += $(CXL_CORE_SRC)/trace.o
> cxl_core-$(CONFIG_CXL_REGION) += $(CXL_CORE_SRC)/region.o
> cxl_core-$(CONFIG_CXL_MCE) += $(CXL_CORE_SRC)/mce.o
> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index 1c3336095923..40d9f7b76d01 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -470,6 +470,24 @@ struct cxl_cedt_context {
> struct device *dev;
> };
>
> +static int mock_hmat_get_extended_linear_cache_size(struct device *dev,
> + struct resource *backing_res,
> + int nid, resource_size_t *size)
> +{
> + struct cxl_decoder *cxld;
> + struct cxl_port *port;
> +
> + if (!is_root_decoder(dev))
Here is the real reason for the parameter and it's not debug...
> + return -EINVAL;
> +
> + cxld = to_cxl_decoder(dev);
> + port = to_cxl_port(cxld->dev.parent);
> + if (is_mock_port(&port->dev))
> + return -EOPNOTSUPP;
> +
> + return hmat_get_extended_linear_cache_size(dev, backing_res, nid, size);
> +}
> +
> static int mock_acpi_table_parse_cedt(enum acpi_cedt_type id,
> acpi_tbl_entry_handler_arg handler_arg,
> void *arg)
> @@ -1040,6 +1058,8 @@ static struct cxl_mock_ops cxl_mock_ops = {
> .devm_cxl_enumerate_decoders = mock_cxl_enumerate_decoders,
> .cxl_endpoint_parse_cdat = mock_cxl_endpoint_parse_cdat,
> .list = LIST_HEAD_INIT(cxl_mock_ops.list),
> + .hmat_get_extended_linear_cache_size =
> + mock_hmat_get_extended_linear_cache_size,
> };
>
> static void mock_companion(struct acpi_device *adev, struct device *dev)
> diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
> index af2594e4f35d..69aa3deee6c2 100644
> --- a/tools/testing/cxl/test/mock.c
> +++ b/tools/testing/cxl/test/mock.c
> @@ -78,6 +78,29 @@ int __wrap_acpi_table_parse_cedt(enum acpi_cedt_type id,
> }
> EXPORT_SYMBOL_NS_GPL(__wrap_acpi_table_parse_cedt, "ACPI");
>
> +int __wrap_hmat_get_extended_linear_cache_size(struct device *dev,
> + struct resource *backing_res,
> + int nid,
> + resource_size_t *size)
> +{
> + int index;
> + struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
> + int rc;
> +
> + if (ops)
> + rc = ops->hmat_get_extended_linear_cache_size(dev,
> + backing_res, nid,
> + size);
> + else
> + rc = hmat_get_extended_linear_cache_size(dev, backing_res,
> + nid, size);
> +
> + put_cxl_mock_ops(index);
> +
> + return rc;
> +}
> +EXPORT_SYMBOL_NS_GPL(__wrap_hmat_get_extended_linear_cache_size, "CXL");
> +
> acpi_status __wrap_acpi_evaluate_integer(acpi_handle handle,
> acpi_string pathname,
> struct acpi_object_list *arguments,
> diff --git a/tools/testing/cxl/test/mock.h b/tools/testing/cxl/test/mock.h
> index d1b0271d2822..240ad3d3e6f9 100644
> --- a/tools/testing/cxl/test/mock.h
> +++ b/tools/testing/cxl/test/mock.h
> @@ -26,6 +26,10 @@ struct cxl_mock_ops {
> int (*devm_cxl_enumerate_decoders)(
> struct cxl_hdm *hdm, struct cxl_endpoint_dvsec_info *info);
> void (*cxl_endpoint_parse_cdat)(struct cxl_port *port);
> + int (*hmat_get_extended_linear_cache_size)(struct device *dev,
> + struct resource *backing_res,
> + int nid,
> + resource_size_t *size);
> };
>
> void register_cxl_mock_ops(struct cxl_mock_ops *ops);
>
> base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca
next prev parent reply other threads:[~2025-06-09 10:38 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-06-05 0:11 [PATCH] cxl: Remove core/acpi.c and cxl core dependency on ACPI Dave Jiang
2025-06-09 10:38 ` Jonathan Cameron [this message]
2025-06-09 15:10 ` Dave Jiang
2025-06-09 17:03 ` Fabio M. De Francesco
2025-06-09 23:59 ` Dave Jiang
-- strict thread matches above, loose matches on Subject: below --
2025-06-10 0:02 Dave Jiang
2025-06-10 0:03 ` Dave Jiang
2025-07-11 15:15 Robert Richter
2025-07-11 15:40 ` Dave Jiang
2025-07-11 16:36 ` Jonathan Cameron
2025-07-11 18:30 ` Alison Schofield
2025-07-14 21:36 ` Dave Jiang
2025-07-14 23:51 ` Dave Jiang
2025-07-15 10:14 ` Robert Richter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250609113836.0000558a@huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=alison.schofield@intel.com \
--cc=dan.j.williams@intel.com \
--cc=dave.jiang@intel.com \
--cc=dave@stgolabs.net \
--cc=ira.weiny@intel.com \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-cxl@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=vishal.l.verma@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.