Message-ID: <1296674576.21752909482669.JavaMail.epsvc@epcpadp2new>
Date: Fri, 18 Jul 2025 18:15:15 +0530
From: Neeraj Kumar <s.neeraj@...sung.com>
To: Dave Jiang <dave.jiang@...el.com>
Cc: dan.j.williams@...el.com, dave@...olabs.net,
	jonathan.cameron@...wei.com, alison.schofield@...el.com,
	vishal.l.verma@...el.com, ira.weiny@...el.com, a.manzanares@...sung.com,
	nifan.cxl@...il.com, anisa.su@...sung.com, vishak.g@...sung.com,
	krish.reddy@...sung.com, arun.george@...sung.com, alok.rathore@...sung.com,
	neeraj.kernel@...il.com, linux-kernel@...r.kernel.org,
	linux-cxl@...r.kernel.org, nvdimm@...ts.linux.dev, gost.dev@...sung.com,
	cpgs@...sung.com
Subject: Re: [RFC PATCH 14/20] cxl/region: Add cxl pmem region creation
 routine for region persistency

On 10/07/25 08:59AM, Dave Jiang wrote:
>
>
>On 6/17/25 5:39 AM, Neeraj Kumar wrote:
>> Added exported cxl_create_pmem_region routine to create cxl pmem region
>> 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
>>
>> Signed-off-by: Neeraj Kumar <s.neeraj@...sung.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/port.c b/drivers/cxl/core/port.c
>> index bca668193c49..2452f7c15b2d 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -2150,6 +2150,12 @@ void cxl_bus_drain(void)
>>  }
>>  EXPORT_SYMBOL_NS_GPL(cxl_bus_drain, "CXL");
>>
>> +void cxl_wq_flush(void)
>> +{
>> +	flush_workqueue(cxl_bus_wq);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_wq_flush, "CXL");
>> +
>>  bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd)
>>  {
>>  	return queue_work(cxl_bus_wq, &cxlmd->detach_work);
>> 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)
>
>Maybe call it resize_or_free_region_hpa()?
>
>Also rename 'val' to 'size'
>

Sure Dave, I will fix it next patch-set.

>> +{
>> +	int rc;
>> +
>> +	rc = down_write_killable(&cxl_region_rwsem);
>> +	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 0;
>> +}
>
>Share common code with core/region.c:size_store(). Please use helper function and not duplicate code.
>

Sure, I will try to minimize duplicate code as much as possible in next patch-set.

>> +
>> +static ssize_t update_region_dpa_size(struct cxl_region *cxlr,
>
>resize_or_free_dpa()
>

Sure, I will fix it in next patch-set

>> +		struct cxl_decoder *cxld,
>> +		unsigned long long size)
>
>u64 size
>

Sure, I will fix it in next patch-set

>> +{
>> +	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);
>> +	if (rc)
>> +		return rc;
>> +
>> +	return 0;
>> +}
>
>Share common code with core/port.c:dpa_size_store(). Please use helper function and not duplicate code.
>

Sure, I will try minimizing duplicate code as much as possible.

>> +
>> +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);
>> +
>> +	rc = cxl_dpa_set_mode(cxled, CXL_DECODER_PMEM);
>
>Don't think CXL_DECODER_PMEM exists any longer. It's CXL_PARTMODE_PMEM. Just beware there have been some changes while you are rebasing to the latest upstream code.
>

Thanks, I will rebase and fix it with latest change

>> +	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);
>> +
>> +	if (rc < 0)
>> +		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);
>> +	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)
>> +		p->state = CXL_CONFIG_COMMIT;
>> +out:
>> +	up_write(&cxl_region_rwsem);
>> +	if (rc)
>> +		return rc;
>> +	return 0;
>> +}
>
>Sharing common code with core/region.c:commit_store()?
>

Sure, I will handle it in next patch-set

>> +
>> +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);
>> +
>> +	port = to_cxl_port(cxlrd->cxlsd.cxld.dev.parent);
>> +
>> +	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 */
>Not sure what value this comment adds

Sure, I will remove these comments

>> +	if (update_region_size(cxlr, params->rawsize))
>> +		goto err;
>> +
>> +	/* Flush cxl wq */
>Can you explain here why a flush is needed?

These steps of region creation is taken from how ndctl creates region
using *_store(). I will re-look if its required here.

>> +	cxl_wq_flush();
>> +
>> +	/* Clear DPA Size */
>comment provides no value
>> +	if (update_region_dpa_size(cxlr, cxld, 0))
>> +		goto err;
>> +
>> +	/* Update DPA mode */
>same as above
>> +	if (update_region_dpa_mode(cxlr, cxld))
>> +		goto err;
>> +
>> +	/* Update DPA Size */
>same as above
>> +	if (update_region_dpa_size(cxlr, cxld, params->rawsize))
>> +		goto err;
>> +
>> +	/* Attach region targets */
>same as above
>> +	if (attach_region_target(cxlr, cxld, params->position))
>> +		goto err;
>> +
>> +	/* Commit Region */
>same as above

Sure, I will remove unwanted comments

>> +	if (commit_region(cxlr))
>> +		goto err;
>
>Can you please provide some verbose explanation as to what all these extra steps are doing for pmem versus devm_cxl_add_region() for dram?
>

These steps of region creation is taken from how ndctl creates region.
Sure I will update the comments accordingly.

>> +
>> +	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);
>> +	}
>> +
>> +	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");
>
>Can __create_region() be modified to determine whether to create dram region or pmem?
>
>DJ

Sure, I will refactor it accordingly

Regards,
Neeraj

Powered by blists - more mailing lists