On 15/01/26 06:28PM, Jonathan Cameron wrote:
>On Fri,  9 Jan 2026 18:14:36 +0530
>Neeraj Kumar <s.neeraj@samsung.com> wrote:
>
>> create_pmem_region() creates CXL region based on region information
>> parsed from the Label Storage Area (LSA). This routine requires cxl
>> endpoint decoder and root decoder. Add cxl_find_root_decoder_by_port()
>> and cxl_find_free_ep_decoder() to find the root decoder and a free
>> endpoint decoder respectively.
>>
>> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
>Hi Neeraj,
>
>Just a few minor things.
>
>Jonathan
>
>> diff --git a/drivers/cxl/core/pmem_region.c b/drivers/cxl/core/pmem_region.c
>> index 53d3d81e9676..4a8cf8322cf0 100644
>> --- a/drivers/cxl/core/pmem_region.c
>> +++ b/drivers/cxl/core/pmem_region.c
>> @@ -287,3 +287,139 @@ int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
>>  	cxlr->cxl_nvb = NULL;
>>  	return rc;
>>  }
>> +
>> +static int match_root_decoder_by_dport(struct device *dev, const void *data)
>> +{
>> +	const struct cxl_port *ep_port = data;
>> +	struct cxl_root_decoder *cxlrd;
>> +	struct cxl_port *root_port;
>> +	struct cxl_decoder *cxld;
>> +	struct cxl_dport *dport;
>> +	bool dport_matched = false;
>> +
>> +	if (!is_root_decoder(dev))
>> +		return 0;
>> +
>> +	cxld = to_cxl_decoder(dev);
>> +	if (!(cxld->flags & CXL_DECODER_F_PMEM))
>> +		return 0;
>> +
>> +	cxlrd = to_cxl_root_decoder(dev);
>> +
>> +	root_port = cxlrd_to_port(cxlrd);
>> +	dport = cxl_find_dport_by_dev(root_port, ep_port->host_bridge);
>> +	if (!dport)
>> +		return 0;
>> +
>There is a fairly standard way to check if a loop matched without
>needing a boolean. Just check if the exit condition was reached.
>
>drop declaration of i out of here.
>> +	for (int i = 0; i < cxlrd->cxlsd.nr_targets; i++) {
>> +		if (dport == cxlrd->cxlsd.target[i]) {
>> +			dport_matched = true;
>No need for this.
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!dport_matched)
>	if (i == cxlrd->cxlsd.nr_targets)
>		return 0;

Thanks for your suggestion. I have fixed it accordingly in V6.

>
>> +		return 0;
>> +
>> +	return is_root_decoder(dev);
>> +}
>> +
>> +/**
>> + * cxl_find_root_decoder_by_port() - find a cxl root decoder on cxl bus
>> + * @port: any descendant port in CXL port topology
>> + * @cxled: endpoint decoder
>> + *
>> + * Caller of this function must call put_device() when done as a device ref
>> + * is taken via device_find_child()
>> + */
>> +static struct cxl_root_decoder *
>> +cxl_find_root_decoder_by_port(struct cxl_port *port,
>> +			      struct cxl_endpoint_decoder *cxled)
>> +{
>> +	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
>> +	struct cxl_port *ep_port = cxled_to_port(cxled);
>> +	struct device *dev;
>> +
>> +	if (!cxl_root)
>> +		return NULL;
>> +
>> +	dev = device_find_child(&cxl_root->port.dev, ep_port,
>> +				match_root_decoder_by_dport);
>> +	if (!dev)
>> +		return NULL;
>> +
>> +	return to_cxl_root_decoder(dev);
>> +}
>
>> +void create_pmem_region(struct nvdimm *nvdimm)
>> +{
>> +	struct cxl_region *cxlr;
>> +	struct cxl_memdev *cxlmd;
>> +	struct cxl_nvdimm *cxl_nvd;
>> +	struct cxl_endpoint_decoder *cxled;
>> +	struct cxl_pmem_region_params *params;
>
>CXL tends to be reverse xmas tree and good to stick to local style.

Changed them in reverse xmas tree style.


Regards,
Neeraj