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

[Dave Jiang <dave.jiang@intel.com>]

On 6/10/25 2:35 AM, Jonathan Cameron wrote:
> 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.

Yeah I figured it's better off to push all the complexity to cxl_test.

> 
> 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)
> 

I can change it. Original code was just copied directly from core/region.c.

>> +	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?

Yes. Forgot to add the __free(put_device).
> 
>> +	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.

I'll move it a line up.
> 
> 
>