Re: [PATCH v2] cxl: Remove core/acpi.c and cxl core dependency on ACPI

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Mon, 9 Jun 2025 17:02:37 -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>
> ---
> v2:
> - Remove introduction of 'struct device' parameter to
>   hmat_get_extended_linear_cache_size().
> - Drop comment on flex array (Jonathan)
> - Rename cxl_rcd_ops to cxl_rd_ops for 'root decoder ops' (Fabio)
There is a lot of indirection going on in here, but I think I understand
why it is all there.  A few comments inline. The device reference one
just passes what I'm comfortable assuming you will just fix it up though
so I'll wait for v3 for tags.

J

> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index 8a5815ca870d..3f6780179752 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -470,6 +470,43 @@ struct cxl_cedt_context {
>  	struct device *dev;
>  };
>  
> +static int match_root_decoder_by_range(struct device *dev, const void *data)
> +{
> +	const struct range *r1, *r2 = data;

I'm fussy about this, but I'd not mix a non assigned and assigned declaration
in one line.  (Trivial comment though so ignore if you like)

> +	struct cxl_root_decoder *cxlrd;
> +
> +	if (!is_root_decoder(dev))
> +		return 0;
> +
> +	cxlrd = to_cxl_root_decoder(dev);
> +	r1 = &cxlrd->cxlsd.cxld.hpa_range;
> +	return range_contains(r1, r2);
> +}
> +
> +static int mock_hmat_get_extended_linear_cache_size(struct resource *backing_res,
> +						    int nid, resource_size_t *size)
> +{
> +	struct range backing_range = {
> +		.start = backing_res->start,
> +		.end = backing_res->end,
> +	};
> +	struct cxl_decoder *cxld;
> +	struct cxl_port *port;
> +	struct device *dev;
> +
> +	dev = bus_find_device(&cxl_bus_type, NULL, &backing_range,
> +			      match_root_decoder_by_range);

This grabs a reference to the device.  Do we need a put_device()
somewhere?

> +	if (!dev)
> +		return -ENODEV;
> +
> +	cxld = to_cxl_decoder(dev);
> +	port = to_cxl_port(cxld->dev.parent);
> +	if (is_mock_dev(port->uport_dev))
> +		return -EOPNOTSUPP;
> +
> +	return hmat_get_extended_linear_cache_size(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 +1077,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,
>  };

Why is list set last in here?  It's first in the structure
and we now end up with the callbacks not next to each other which looks
weird.  Maybe move the LIST_HEAD_INIT() up, or if not move the new
callback setting up one line to next to parse_cdat.