Re: [PATCH v12 2/8] cxl/mem: Read dynamic capacity configuration from the device

On Tue, Aug 04, 2026 at 12:55:27AM +0100, Jonathan Cameron wrote:
> On Fri, 31 Jul 2026 01:48:07 -0700
> Anisa Su <[email protected]> wrote:
> 
> > From: Ira Weiny <[email protected]>
> > 
> > Devices which optionally support Dynamic Capacity (DC) are configured
> > via mailbox commands.  CXL r4.0 section 9.13.3 requires the host to issue
> > the Get DC Configuration command in order to properly configure DCDs.
> > Without the Get DC Configuration command DCD can't be supported.
> > 
> > Implement the DC mailbox commands as specified in CXL 4.0 section
> > 8.2.10.9.9 (opcodes 48XXh) to read and store the DCD configuration
> > information.  Disable DCD if an invalid configuration is found.
> > 
> > Linux has no support for more than one dynamic capacity partition.  Read
> > and validate all the partitions but configure only the first partition
> > as 'dynamic ram 1'.  Additional partitions can be added in the future if
> > such a device ever materializes.  Additionally it is anticipated that no
> > skips will be present from the end of the pmem partition.  Check for and
> > disallow this configuration as well.
> > 
> > Based on an original patch by Navneet Singh.
> > 
> > Signed-off-by: Ira Weiny <[email protected]>
> > Signed-off-by: Anisa Su <[email protected]>
> > Tested-by: Wonjae Lee <[email protected]>
> > Tested-by: Junhee Park <[email protected]>
> > Tested-by: Heesoo Kim <[email protected]>
> 
> Hi Anisa,
> 
> A few things to add to the reviews of others.
> 
Thanks Jonathan! Details below.

> Jonathan
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 4790524c32a7..d79019fbd790 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -1352,6 +1352,197 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd)
> >  	return -EBUSY;
> >  }
> >  
> > +static int cxl_dc_check(struct device *dev, struct cxl_dc_partition_info *part_array,
> > +			u8 index, struct cxl_dc_partition *dev_part)
> > +{
> > +	u64 blk_size = le64_to_cpu(dev_part->block_size);
> > +	u64 len = le64_to_cpu(dev_part->length);
> > +
> > +	part_array[index].start = le64_to_cpu(dev_part->base);
> > +	part_array[index].size = le64_to_cpu(dev_part->decode_length);
> > +	part_array[index].size *= CXL_CAPACITY_MULTIPLIER;
> 
> I'd be tempted to make it clear you are setting whole structure with
> 
> 	part_array[index] = (struct cxl_dc_partition_info) {
> 		.start = le64_to_cpu(dev_part->base),
> 		.size = le64_to_cpu(dev_part->decode_length) * CXL_CAPACITY_MULTIPLIER,
> //This is a case where I think readability justifies the long line.
> 
Agreed, and it also improves readability of patch 3, which adds the DSMAD handle.
Originally handle verification came after setting start and size, and
before setting handle. Now, the handle verification goes once in front,
and all 3 fields set together. So the end result reads:

	if (handle & ~0xFF) {
		dev_warn(dev, "DSMAD handle 0x%x has non-zero reserved bits\n", handle);
		return -EINVAL;
	}

	part_array[index] = (struct cxl_dc_partition_info) {
		.start = le64_to_cpu(dev_part->base),
		.size = le64_to_cpu(dev_part->decode_length) * CXL_CAPACITY_MULTIPLIER,
		.handle = handle,
	};

> 	};
> > +
> > +	/* Check partitions are in increasing DPA order */
> > +	if (index > 0) {
> > +		struct cxl_dc_partition_info *prev_part = &part_array[index - 1];
> > +
> > +		if ((prev_part->start + prev_part->size) >
> > +		     part_array[index].start) {
> 
> I'd be tempted to put the above on one line.
> 
Done.
	if (prev_part->start + prev_part->size != part_array[index].start) {

The comparison changed from > to != because Alison pointed out this was
only an overlap check and gaps were passing. Since Linux requires
contiguous partitions I made it require exact contiguity.

One thing I mentioned in my reply to Alison: since only partiton 0 is
configured, a device with a valid partition 0 has DCD disabled if
a gap exists in subsequent partitions, even though the spec permits the
DPA gaps and we never use other partitions.

cxl_dpa_setup() is where partitions are configured and enforces the contiguity
rule. Is it more correct for cxl_dev_dc_identify() to check for spec compliance
only (no overlap) and defer the Linux-specific rule of no gaps between
partitions up to cxl_dpa_check()?

> > +			dev_err(dev,
> > +				"DPA ordering violation for DC partition %d and %d\n",
> > +				index - 1, index);
> > +			return -EINVAL;
> > +		}
> > +	}
> 
> Alison's comments seem valid to me and apply to rest of this function at least so I'll
> not add anything else on that.
> 

Agreed and worked through.

> > +
> > +}
> 
> ...
> 
> > +/**
> > + * cxl_dev_dc_identify() - Reads the dynamic capacity information from the
> > + *                         device.
> > + * @mbox: Mailbox to query
> > + * @dc_info: The dynamic partition information to return
> > + *
> > + * Read Dynamic Capacity information from the device and return the partition
> > + * information.
> > + *
> > + * Return: 0 if identify was executed successfully, -ERRNO on error.
> > + *         on error only dc_info is left unchanged.
> > + */
> > +int cxl_dev_dc_identify(struct cxl_mailbox *mbox,
> > +			struct cxl_dc_partition_info *dc_info)
> > +{
> > +	struct cxl_dc_partition_info partitions[CXL_MAX_DC_PARTITIONS];
> > +	struct cxl_mbox_get_dc_config_out *dc_resp __free(kfree) = NULL;
> > +	struct device *dev = mbox->host;
> > +	u8 start_partition;
> > +	u8 num_partitions;
> > +	u8 partition_count;
> > +	size_t dc_resp_size;
> > +
> > +	/* Bound requested number of partitions by mailbox payload size */
> > +	partition_count = min_t(size_t, CXL_MAX_DC_PARTITIONS,
> > +				(mbox->payload_size - sizeof(*dc_resp) -
> > +				 sizeof(struct cxl_mbox_get_dc_config_tail)) /
> > +				sizeof(struct cxl_dc_partition));
> > +	dc_resp_size = struct_size(dc_resp, partition, partition_count) +
> > +		       sizeof(struct cxl_mbox_get_dc_config_tail);
> > +
> > +	dc_resp = kmalloc(dc_resp_size, GFP_KERNEL);
> > +	if (!dc_resp)
> > +		return -ENOMEM;
> > +
> > +	/*
> > +	 * Read and check all partition information for validity and potential
> > +	 * debugging; see debug output in cxl_dc_check()
> > +	 */
> > +	start_partition = 0;
> > +	num_partitions = 0;
> > +	do {
> > +		int rc, i, j;
> > +
> > +		rc = cxl_get_dc_config(mbox, start_partition, partition_count,
> > +				       dc_resp, dc_resp_size);
> > +		if (rc < 0) {
> > +			dev_err(dev, "Failed to get DC config: %d\n", rc);
> > +			return rc;
> > +		}
> > +
> > +		if (rc == 0) {
> > +			dev_err(dev,
> > +				"Device reported %u partitions available but returned none at index %u\n",
> > +				dc_resp->avail_partition_count, start_partition);
> > +			return -EIO;
> > +		}
> > +
> > +		num_partitions += rc;
> > +
> > +		if (num_partitions < 1 || num_partitions > CXL_MAX_DC_PARTITIONS) {
> 
> How could num_partitions end up either 0 or negative given we added at least 1 and
> I don't see a path by which it will get decremented. Roll over isn't a thing given
> we would have failed the other check on a previous loop.
> 
Good point, dropped. This leaves:
	
	if (num_partitions > CXL_MAX_DC_PARTITIONS) {

Alison separately asked for avail_partition_count to be range checked
against the 1 to 8 Table 8-346 requires, which is now done in
cxl_get_dc_config() before the count is used as the loop bound. So this
check is left as a defense against a faulty device that somehow returns
more partitions than advertised.

> 
> > +			dev_err(dev, "Invalid num of dynamic capacity partitions %d\n",
> > +				num_partitions);
> > +			return -EINVAL;
> > +		}
> > +
> > +		for (i = start_partition, j = 0; i < num_partitions; i++, j++) {
> > +			rc = cxl_dc_check(dev, partitions, i,
> > +					  &dc_resp->partition[j]);
> > +			if (rc)
> > +				return rc;
> > +		}
> > +
> > +		start_partition = num_partitions;
> > +
> > +	} while (num_partitions < dc_resp->avail_partition_count);
> > +
> > +	/* Return 1st partition */
> > +	dc_info->start = partitions[0].start;
> > +	dc_info->size = partitions[0].size;
> > +	dev_dbg(dev, "Returning partition 0 %llu size %llu\n",
> > +		dc_info->start, dc_info->size);
> > +
> > +	return 0;
> > +}
> 
> 
> 
> > @@ -1422,6 +1613,35 @@ int cxl_get_dirty_count(struct cxl_memdev_state *mds, u32 *count)
> >  }
> >  EXPORT_SYMBOL_NS_GPL(cxl_get_dirty_count, "CXL");
> >  
> > +void cxl_configure_dcd(struct cxl_memdev_state *mds, struct cxl_dpa_info *info)
> > +{
> > +	struct cxl_dc_partition_info dc_info = { 0 };
> 
> I think the preferred way of doing this is heading for the C23 option of
> 
>   = { };
> 
> but CXL has plenty of { 0 }; so fair enough if you prefer to keep it this
> way.  For anyone concerned about older compilers there is a specific selftest
> for { } doing what we expect even if the structure has holes.
> 
Switched to { }.

> 
> > +	struct device *dev = mds->cxlds.dev;
> > +	int rc;
> > +
> > +	rc = cxl_dev_dc_identify(&mds->cxlds.cxl_mbox, &dc_info);
> > +	if (rc) {
> > +		dev_warn(dev,
> > +			 "Failed to read Dynamic Capacity config: %d\n", rc);
> 
> Fun question on whether we consider a DCD capable device failing such
> a basic thing as a warning level thing or a hard failure thing.
> 
> To me DCD is a important core device feature (unlikely maybe fwctl, features etc)
> so we should be failing. 
> 
> We should definitely be returning an error from here, even if ultimate
> we decide not to act on that error in cxl_pci_probe().
> 
Agreed, cxl_configure_dcd() returns int now. cxl_pci_probe() now
disables dcd if cxl_configure_dcd() fails:

	if (cxl_dcd_supported(mds)) {
		rc = cxl_configure_dcd(mds, &range_info);
		if (rc)
			cxl_disable_dcd(mds);
	}

The three paths return distinct errors:
1. the cxl_dev_dc_identify() err propagates,
2. -EINVAL for a DC partition overlapping static capacity
3. -EOPNOTSUPP for the gap below.

Re: should "we consider a DCD capable device failing such a basic thing as
a warning level thing or a hard failure thing":

I went with continuing the probe without DCD. Failing cxl_pci_probe()
costs the device its static capacity too, which seems a worse outcome than
running without dynamic capacity.
But I could be persuaded the other way -- if a DCD device that cannot report
its DC Config then it should not come up at all.

> > +		cxl_disable_dcd(mds);
> > +		return;
> > +	}
> > +
> > +	/* Skips between pmem and the dynamic partition are not supported */
> > +	if (dc_info.start != info->size) {
> > +		dev_warn(dev,
> > +			 "Dynamic Capacity skip from pmem not supported\n");
> > +		cxl_disable_dcd(mds);
> > +		return;
> 
> In this case not failing hard is probably fair if that is a specification
> allowed feature we are just not choosing to support for now.
> 
Agreed, so it stays a warning and returns -EOPNOTSUPP. From Alison's
review, I split it up to differentiate skip from overlap. Overlap
returns -EINVAL. So now we have:

	if (dc_info.start < info->size) {
		dev_err(dev, "DC partition 0 base %#llx overlaps static capacity ending at %#llx\n",
			dc_info.start, info->size);
		return -EINVAL;
	}

	/* A gap between static capacity and the DC partition is not supported */
	if (dc_info.start > info->size) {
		dev_warn(dev, "DC partition 0 base %#llx leaves a gap from static capacity ending at %#llx\n",
			 dc_info.start, info->size);
		return -EOPNOTSUPP;
	}

The old message also said "pmem" when the value it compares against is
info->size, ie mds->total_bytes, the whole static capacity from Identify
rather than the end of the pmem partition, which is not quite accurate
for a volatile-only device. Reworded.

Thanks,
Anisa

> > +	}
> > +
> > +	info->size += dc_info.size;
> > +	dev_dbg(dev, "Adding dynamic ram partition 1; %llu size %llu\n",
> > +		dc_info.start, dc_info.size);
> > +	add_part(info, dc_info.start, dc_info.size, CXL_PARTMODE_DYNAMIC_RAM_1);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_configure_dcd, "CXL");
> 
>