Re: [PATCH v17 13/22] cxl: Define a driver interface for DPA allocation

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

On Tue, 24 Jun 2025 15:13:46 +0100
<alejandro.lucero-palau@amd.com> wrote:

> From: Alejandro Lucero <alucerop@amd.com>
> 
> Region creation involves finding available DPA (device-physical-address)
> capacity to map into HPA (host-physical-address) space.
> 
> In order to support CXL Type2 devices, define an API, cxl_request_dpa(),
> that tries to allocate the DPA memory the driver requires to operate.The
> memory requested should not be bigger than the max available HPA obtained
> previously with cxl_get_hpa_freespace.
> 
> Based on https://lore.kernel.org/linux-cxl/168592158743.1948938.7622563891193802610.stgit@dwillia2-xfh.jf.intel.com/
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Reviewed-by: Ben Cheatham <benjamin.cheatham@amd.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Hmm. I wouldn't trust this last guy not to have missed a few things.
See below.


> +static struct cxl_endpoint_decoder *
> +cxl_find_free_decoder(struct cxl_memdev *cxlmd)
> +{
> +	struct cxl_port *endpoint = cxlmd->endpoint;
> +	struct device *dev;
> +
> +	scoped_guard(rwsem_read, &cxl_dpa_rwsem) {
> +		dev = device_find_child(&endpoint->dev, NULL,
> +					find_free_decoder);
> +	}
> +	if (dev)
> +		return to_cxl_endpoint_decoder(dev);
> +
> +	return NULL;

If this code isn't going to get modified later, could be simpler as

	guard(rwsem_read)(&cxl_dpa_rwsem) {
	dev = device_find_child(&endpoint->dev, NULL, find_free_decoder);
	if (!dev)
		return NULL

	return to_cxl_endpoint_decoder(dev);
		
> +}
> +
> +/**
> + * cxl_request_dpa - search and reserve DPA given input constraints
> + * @cxlmd: memdev with an endpoint port with available decoders
> + * @mode: DPA operation mode (ram vs pmem)
> + * @alloc: dpa size required
> + *
> + * Returns a pointer to a cxl_endpoint_decoder struct or an error
> + *
> + * Given that a region needs to allocate from limited HPA capacity it
> + * may be the case that a device has more mappable DPA capacity than
> + * available HPA. The expectation is that @alloc is a driver known
> + * value based on the device capacity but it could not be available
> + * due to HPA constraints.
> + *
> + * Returns a pinned cxl_decoder with at least @alloc bytes of capacity
> + * reserved, or an error pointer. The caller is also expected to own the
> + * lifetime of the memdev registration associated with the endpoint to
> + * pin the decoder registered as well.
> + */
> +struct cxl_endpoint_decoder *cxl_request_dpa(struct cxl_memdev *cxlmd,
> +					     enum cxl_partition_mode mode,
> +					     resource_size_t alloc)
> +{
> +	struct cxl_endpoint_decoder *cxled __free(put_cxled) =
> +				cxl_find_free_decoder(cxlmd);
> +	struct device *cxled_dev;
> +	int rc;
> +
> +	if (!IS_ALIGNED(alloc, SZ_256M))
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!cxled) {
> +		rc = -ENODEV;
> +		goto err;
> +	}
> +
> +	rc = cxl_dpa_set_part(cxled, mode);
> +	if (rc)
> +		goto err;
> +
> +	rc = cxl_dpa_alloc(cxled, alloc);
> +	if (rc)
> +		goto err;
> +
> +	return cxled;

I was kind of expecting us to disable the put above wuth a return_ptr()
here.  If there is a reason why not, add a comment as it is not obvious
to me anyway!


> +err:
> +	put_device(cxled_dev);

It's not been assigned.  I'm surprised if none of the standard tooling
(sparse, smatch etc screamed about this one).
For complex series like this it's worth running them on each patch just to
avoid possible bot warnings later!


> +	return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_request_dpa, "CXL");
__CXL_CXL_H__ */