Re: [RFC PATCH 14/20] cxl/region: Add cxl pmem region creation routine for region persistency

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

On Tue, 17 Jun 2025 18:09:38 +0530
Neeraj Kumar <s.neeraj@samsung.com> wrote:

> Added exported cxl_create_pmem_region routine to create cxl pmem region

For function names always add () after and drop 'function/routine' etc.
Ends up shorter and easier to read.

> from LSA parsed cxl region information.
> Inspirition for the function is taken from ndctl device attribute
> (_store) call. It allocates cxlr and fills information parsed from LSA
> and calls device_add(&cxlr->dev) to initiates further region creation
> porbes
Spell check.

> 
> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
> ---
>  drivers/cxl/core/port.c   |   6 ++
>  drivers/cxl/core/region.c | 208 ++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h         |  11 ++
>  3 files changed, 225 insertions(+)
> 

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index b98b1ccffd1c..8990e3c3474d 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2522,6 +2522,214 @@ static ssize_t create_ram_region_show(struct device *dev,
>  	return __create_region_show(to_cxl_root_decoder(dev), buf);
>  }
>  
> +static ssize_t update_region_size(struct cxl_region *cxlr, u64 val)
> +{
> +	int rc;
> +
> +	rc = down_write_killable(&cxl_region_rwsem);
ACQUIRE() as mentioned below.

> +	if (rc)
> +		return rc;
> +
> +	if (val)
> +		rc = alloc_hpa(cxlr, val);
> +	else
> +		rc = free_hpa(cxlr);
> +	up_write(&cxl_region_rwsem);
> +
> +	if (rc)
> +		return rc;

	return rc;

> +
> +	return 0;
> +}
> +
> +static ssize_t update_region_dpa_size(struct cxl_region *cxlr,
> +		struct cxl_decoder *cxld,
> +		unsigned long long size)
> +{
> +	int rc;
> +	struct cxl_endpoint_decoder *cxled =
> +		to_cxl_endpoint_decoder(&cxld->dev);
> +
> +	if (!IS_ALIGNED(size, SZ_256M))
> +		return -EINVAL;
> +
> +	rc = cxl_dpa_free(cxled);
> +	if (rc)
> +		return rc;
> +
> +	if (size == 0)
> +		return 0;
> +
> +	rc = cxl_dpa_alloc(cxled, size);
return cxl_dpa_alloc()

Unless something more is getting added here in later patches in which case
you can ignore this comment.

> +	if (rc)
> +		return rc;
> +
> +	return 0;
> +}
> +
> +static ssize_t update_region_dpa_mode(struct cxl_region *cxlr,
> +		struct cxl_decoder *cxld)
> +{
> +	int rc;
> +	struct cxl_endpoint_decoder *cxled =
> +		to_cxl_endpoint_decoder(&cxld->dev);
Maybe don't bother with local variable and just put this
below as the parameter.
> +
> +	rc = cxl_dpa_set_mode(cxled, CXL_DECODER_PMEM);

return cxl_dpa_set_mode()

> +	if (rc)
> +		return rc;
> +
> +	return 0;
> +}
> +
> +static size_t attach_region_target(struct cxl_region *cxlr,
> +		struct cxl_decoder *cxld, int pos)
> +{
> +	int rc;
> +	struct cxl_endpoint_decoder *cxled =
> +		to_cxl_endpoint_decoder(&cxld->dev);
> +
> +	rc = attach_target(cxlr, cxled, pos, TASK_INTERRUPTIBLE);
> +
No blank line here
> +	if (rc < 0)
Can it ever be > 0 ?
If not, return attach_target() should be fine.
> +		return rc;
> +
> +	return 0;
> +}
> +
> +static ssize_t commit_region(struct cxl_region *cxlr)
> +{
> +	struct cxl_region_params *p = &cxlr->params;
> +	ssize_t rc;
> +
> +	rc = down_write_killable(&cxl_region_rwsem);

Maybe look at Dan's new ACQUIRE() series. The last patch in there
is targeting code similar to this.

> +	if (rc)
> +		return rc;
> +
> +	/* Already in the requested state? */
> +	if (p->state >= CXL_CONFIG_COMMIT)
> +		goto out;
> +
> +	/* Not ready to commit? */
> +	if (p->state < CXL_CONFIG_ACTIVE) {
> +		rc = -ENXIO;
> +		goto out;
> +	}
> +
> +	/*
> +	 * Invalidate caches before region setup to drop any speculative
> +	 * consumption of this address space
> +	 */
> +	rc = cxl_region_invalidate_memregion(cxlr);
> +	if (rc)
> +		goto out;
> +
> +	rc = cxl_region_decode_commit(cxlr);
> +	if (rc == 0)
With AQUIRE() stuff you can just  do
	if (rc)
		return rc;

here

> +		p->state = CXL_CONFIG_COMMIT;
> +out:
> +	up_write(&cxl_region_rwsem);
> +	if (rc)
> +		return rc;
> +	return 0;
> +}
> +
> +static struct cxl_region *
> +devm_cxl_pmem_add_region(struct cxl_root_decoder *cxlrd,
> +		struct cxl_decoder *cxld,
> +		struct cxl_pmem_region_params *params, int id,
> +		enum cxl_decoder_mode mode, enum cxl_decoder_type type)
> +{
> +	struct cxl_port *port;
> +	struct cxl_region *cxlr;
> +	struct cxl_region_params *p;
> +	struct device *dev;
> +	int rc;
> +
> +	if (!cxlrd)
> +		return ERR_PTR(-EINVAL);

For a check like this, add a comment on why it might be NULL.
If it can't be, then drop the check.

> +
> +	port = to_cxl_port(cxlrd->cxlsd.cxld.dev.parent);

Maybe add a comment on which port this actually is.  These long indirections
can make that hard to figure out!

> +
> +	cxlr = cxl_region_alloc(cxlrd, id);
> +	if (IS_ERR(cxlr))
> +		return cxlr;
> +	cxlr->mode = mode;
> +	cxlr->type = type;
> +
> +	dev = &cxlr->dev;
> +	rc = dev_set_name(dev, "region%d", id);
> +	if (rc)
> +		goto err;
> +
> +	p = &cxlr->params;
> +	p->uuid = params->uuid;
> +	p->interleave_ways = params->nlabel;
> +	p->interleave_granularity = params->ig;
> +
> +	/* Update region size */
> +	if (update_region_size(cxlr, params->rawsize))
> +		goto err;
> +
> +	/* Flush cxl wq */

Much more useful to say 'why' you are flushing it.  It's obvious
from the code that you are.

> +	cxl_wq_flush();
> +
> +	/* Clear DPA Size */

I'd look at all these comments and where they are obvious, drop the comment
and let the code speak for itself.

> +	if (update_region_dpa_size(cxlr, cxld, 0))
> +		goto err;
> +
> +	/* Update DPA mode */
> +	if (update_region_dpa_mode(cxlr, cxld))
> +		goto err;
> +
> +	/* Update DPA Size */
> +	if (update_region_dpa_size(cxlr, cxld, params->rawsize))
> +		goto err;
> +
> +	/* Attach region targets */
It's attaching just one by the look of it.  I'd just drop the comment.

> +	if (attach_region_target(cxlr, cxld, params->position))
> +		goto err;
> +
> +	/* Commit Region */
> +	if (commit_region(cxlr))
> +		goto err;
> +
> +	rc = device_add(dev);
> +	if (rc)
> +		goto err;
> +
> +	rc = devm_add_action_or_reset(port->uport_dev, unregister_region, cxlr);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	dev_dbg(port->uport_dev, "%s: created %s\n",
> +		dev_name(&cxlrd->cxlsd.cxld.dev), dev_name(dev));
> +	return cxlr;
> +
> +err:
> +	put_device(dev);
> +	return ERR_PTR(rc);
> +}
> +
> +struct cxl_region *cxl_create_pmem_region(struct cxl_root_decoder *cxlrd,
> +		struct cxl_decoder *cxld,
> +		struct cxl_pmem_region_params *params, int id)
> +{
> +	int rc;
> +
> +	rc = memregion_alloc(GFP_KERNEL);
> +	if (rc < 0)
> +		return ERR_PTR(rc);
> +
> +	if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) {
> +		memregion_free(rc);
> +		return ERR_PTR(-EBUSY);
> +	}
I'm a little surprised not to see cleanup of the memregion via
devm somewhere here.
> +
> +	return devm_cxl_pmem_add_region(cxlrd, cxld, params, id,
> +			CXL_DECODER_PMEM, CXL_DECODER_HOSTONLYMEM);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_create_pmem_region, "CXL");
> +
>  static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd,
>  					  enum cxl_decoder_mode mode, int id)
>  {