Re: [PATCH V4 11/17] cxl/region: Add devm_cxl_pmem_add_region() for pmem region creation

[Neeraj Kumar <s.neeraj@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 4340 bytes --]

On 17/12/25 03:28PM, Jonathan Cameron wrote:
>On Wed, 19 Nov 2025 13:22:49 +0530
>Neeraj Kumar <s.neeraj@samsung.com> wrote:
>
>> devm_cxl_pmem_add_region() is used to create cxl region based on region
>> information scanned from LSA.
>>
>> devm_cxl_add_region() is used to just allocate cxlr and its fields are
>> filled later by userspace tool using device attributes (*_store()).
>>
>> Inspiration for devm_cxl_pmem_add_region() is taken from these device
>> attributes (_store*) calls. It allocates cxlr and fills information
>> parsed from LSA and calls device_add(&cxlr->dev) to initiate further
>> region creation porbes
>>
>> Rename __create_region() to cxl_create_region(), which will be used
>> in later patch to create cxl region after fetching region information
>> from LSA.
>>
>> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
>
>I think there is an underflow of the device reference count in an error
>path. See below.
>
>Jonathan
>
>> +static struct cxl_region *
>> +devm_cxl_pmem_add_region(struct cxl_root_decoder *cxlrd, int id,
>> +			 struct cxl_pmem_region_params *params,
>> +			 struct cxl_decoder *cxld,
>> +			 enum cxl_decoder_type type)
>> +{
>> +	struct cxl_endpoint_decoder *cxled;
>> +	struct cxl_region_params *p;
>> +	struct cxl_port *root_port;
>> +	struct device *dev;
>> +	int rc;
>> +
>> +	struct cxl_region *cxlr __free(put_cxl_region) =
>> +		cxl_region_alloc(cxlrd, id);
>It can be tricky to get the use of __free() when related
>to devices that are being registered right.  I'm not sure it
>is quite correct here.
>
>> +	if (IS_ERR(cxlr))
>> +		return cxlr;
>> +
>> +	cxlr->mode = CXL_PARTMODE_PMEM;
>> +	cxlr->type = type;
>> +
>> +	dev = &cxlr->dev;
>> +	rc = dev_set_name(dev, "region%d", id);
>> +	if (rc)
>> +		return ERR_PTR(rc);
>> +
>> +	p = &cxlr->params;
>> +	p->uuid = params->uuid;
>> +	p->interleave_ways = params->nlabel;
>> +	p->interleave_granularity = params->ig;
>> +
>> +	rc = alloc_region_hpa(cxlr, params->rawsize);
>> +	if (rc)
>> +		return ERR_PTR(rc);
>> +
>> +	cxled = to_cxl_endpoint_decoder(&cxld->dev);
>> +
>> +	rc = cxl_dpa_set_part(cxled, CXL_PARTMODE_PMEM);
>> +	if (rc)
>> +		return ERR_PTR(rc);
>> +
>> +	rc = alloc_region_dpa(cxled, params->rawsize);
>> +	if (rc)
>> +		return ERR_PTR(rc);
>> +
>> +	/*
>> +	 * TODO: Currently we have support of interleave_way == 1, where
>> +	 * we can only have one region per mem device. It means mem device
>> +	 * position (params->position) will always be 0. It is therefore
>> +	 * attaching only one target at params->position
>> +	 */
>> +	if (params->position)
>> +		return ERR_PTR(-EOPNOTSUPP);
>> +
>> +	rc = attach_target(cxlr, cxled, params->position, TASK_INTERRUPTIBLE);
>> +	if (rc)
>> +		return ERR_PTR(rc);
>> +
>> +	rc = __commit(cxlr);
>> +	if (rc)
>> +		return ERR_PTR(rc);
>> +
>> +	rc = device_add(dev);
>> +	if (rc)
>> +		return ERR_PTR(rc);
>> +
>> +	root_port = to_cxl_port(cxlrd->cxlsd.cxld.dev.parent);
>> +	rc = devm_add_action_or_reset(root_port->uport_dev,
>> +			unregister_region, cxlr);
>> +	if (rc)
>In this path the __free(put_cxl_region) will put once.
>The unregister_region will both unregister and put.  The
>dev_add_action_or_reset() will have called unregister_region()
>Which does both device_del() and a put on cxlr->dev.
>
>I might have missed another reference but at first glance at least
>this underflows.
>
>Note the different error path for the devm_add_action_or_reset
>in current devm_cxl_add_region() which is there because there isn't
>another reference count to decrement.
>
>Various ways to solve this.  A common one is to separate the
>allocation and adding stuff into another function (with __free as
>you have here) and call that from here, leaving this outer wrapper
>just doing the devm_add_action_or_reset() if everything else
>has succeeded and hence no need for the outer function to do any
>other reference coutn handling.  Or just don't use __free() as
>is done in devm_cxl_add_region()
>

I have used __free() based on Dave's review comment in V2[1] to
avoid extra gotos. Thanks for catching this reference underflow.

I have fixed it in V5 as per your suggestion.
I have used separate routine cxl_pmem_region_prep() where i have used __free().

[1]: https://lore.kernel.org/linux-cxl/148912029.181757055784505.JavaMail.epsvc@epcpadp2new/



Regards,
Neeraj

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]