Re: [PATCH v10 07/31] cxl/region: Add DC DAX region support

[Dave Jiang <dave.jiang@intel.com>]

On 6/2/26 2:22 AM, Anisa Su wrote:
> On Wed, May 27, 2026 at 05:16:44PM -0700, Dave Jiang wrote:
>>
>>
>> On 5/23/26 2:43 AM, Anisa Su wrote:
>>> From: Ira Weiny <ira.weiny@intel.com>

< --snip -->

>>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>>> index a7f71f36531f..2d33001dac26 100644
>>> --- a/drivers/cxl/core/port.c
>>> +++ b/drivers/cxl/core/port.c
>>> @@ -337,6 +337,7 @@ static struct attribute *cxl_decoder_root_attrs[] = {
>>>  	&dev_attr_qos_class.attr,
>>>  	SET_CXL_REGION_ATTR(create_pmem_region)
>>>  	SET_CXL_REGION_ATTR(create_ram_region)
>>> +	SET_CXL_REGION_ATTR(create_dynamic_ram_a_region)
>>
>> With this add, may need to add checks in cxl_root_decoder_visible() for dynamic_ram for create and also delete. 
>>
> So for this check, since there's no CXL_DECODER_F_ bit defined for DCD, I considered
> traversing through all endpoints and seeing if they have a DYNAMIC_RAM_A
> partition, but that traversal already happens in the store_targetN() path,
> which also includes the mode mismatch check.
> 
> Specifically, in cxl_region_attach:
> 
> if (cxlds->part[cxled->part].mode != cxlr->mode) {
> 	dev_dbg(&cxlr->dev, "%s region mode: %d mismatch\n",
> 		dev_name(&cxled->cxld.dev), cxlr->mode);
> 	return -EINVAL;
> }
> 
> Is it sufficient here to prohibit creating a dynamic ram region if the root
> decoder does not support ram?
> 
> if (a == CXL_REGION_ATTR(create_dynamic_ram_a_region) && !can_create_ram(cxlrd))
> 	return 0;
> 

I think so.

>>>  	SET_CXL_REGION_ATTR(delete_region)
>>>  	NULL,
>>>  };
>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>> index edc267c6cf77..7561bf3d8af8 100644
>>> --- a/drivers/cxl/core/region.c
>>> +++ b/drivers/cxl/core/region.c
>>> @@ -493,6 +493,11 @@ static int set_interleave_ways(struct cxl_region *cxlr, int val)
>>>  	int save, rc;
>>>  	u8 iw;
>>>  
>>> +	if (cxlr->mode == CXL_PARTMODE_DYNAMIC_RAM_A && val != 1) {
>>> +		dev_err(&cxlr->dev, "Interleaving and DCD not supported\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>>  	rc = ways_to_eiw(val, &iw);
>>>  	if (rc)
>>>  		return rc;
>>> @@ -2389,6 +2394,7 @@ static size_t store_targetN(struct cxl_region *cxlr, const char *buf, int pos,
>>>  	if (sysfs_streq(buf, "\n"))
>>>  		rc = detach_target(cxlr, pos);
>>>  	else {
>>> +		struct cxl_endpoint_decoder *cxled;
>>>  		struct device *dev;
>>>  
>>>  		dev = bus_find_device_by_name(&cxl_bus_type, NULL, buf);
>>> @@ -2400,8 +2406,14 @@ static size_t store_targetN(struct cxl_region *cxlr, const char *buf, int pos,
>>>  			goto out;
>>>  		}
>>>  
>>> -		rc = attach_target(cxlr, to_cxl_endpoint_decoder(dev), pos,
>>> -				   TASK_INTERRUPTIBLE);
>>> +		cxled = to_cxl_endpoint_decoder(dev);
>>> +		if (cxlr->mode == CXL_PARTMODE_DYNAMIC_RAM_A &&
>>> +		    !cxl_dcd_supported(cxled_to_mds(cxled))) {
>>
>> cxled_to_mds() can return NULL with the earlier change suggested. Need to handle that
>>
> Fixed
>> DJ
>>
> Thanks,
> Anisa
> 
> Also, for potential future support for multiple DC partitions not to be awkward, I
> think it would make sense to rename dynamic_ram_a to dynamic_ram_1. Any
> objections?

No objections from me. Seems reasonable.