Re: [PATCH v11 02/31] cxl/mem: Read dynamic capacity configuration from the device

On Thu, Jun 25, 2026 at 06:16:36PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] Mailbox payload size statically requests too many partitions, failing initialization on devices with small mailboxes.

fine... I fixed it but I find it unlikely for a DCD to have the minimum 256B
mailbox size

> - [High] Incorrect unit conversion of DC partition decode_length causes severe integer overflow.

false pos
explained below

> - [High] Logic Error in DPA Skip Check mathematically guarantees failure, breaking DCD on all devices.
false pos
explained below

> - [Medium] Valid CXL devices configured with 0 Dynamic Capacity partitions are erroneously treated as having an I/O error.

false pos. Partition info is only requested if mds->dcd_supported, which is set
to true if the device supports the full set of DCD commands. If the
device supports the commands and has 0 partitions that is an error.

> --
> 
> commit cbb0f735b5888cbb19cb493b31cc330ef3afdadf
> Author: Ira Weiny <[email protected]>
> 
> cxl/mem: Read dynamic capacity configuration from the device
> 
> This commit implements the mailbox commands to read Dynamic Capacity
> partition configurations from CXL devices. It validates the partitions
> and configures the first partition as dynamic RAM.
> 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 07aba6f0b7194..2932bbd67e550 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -1347,6 +1347,188 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd)
> 
> [ ... ]
> 
> > +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;
> 
> [Severity: High]
> Is it correct to multiply decode_length by CXL_CAPACITY_MULTIPLIER here?
> 
it is.

> The CXL specification provides the decode length in bytes, similar to the 
> base and length fields. Multiplying this by 256MB will cause a 64-bit 

r4.0 8.3.10.9.9.1 Table 8-347 "Region Decode Length" The
device-assigned number of bytes of DPA that this region consumes, in
multiples of 256MB"

decode length is provided in multiples of 256B. This conversion is
necessary to get actual # of bytes.

> integer overflow for capacities of 64GB or larger. This overflow can wrap 
> around to a small value, bypassing the later DPA ordering checks and 
> registering a corrupted partition size.
> 
> [ ... ]
> 
> > +/* Returns the number of partitions in dc_resp or -ERRNO */
> > +static int cxl_get_dc_config(struct cxl_mailbox *mbox, u8 start_partition,
> > +			     struct cxl_mbox_get_dc_config_out *dc_resp,
> > +			     size_t dc_resp_size)
> > +{
> > +	struct cxl_mbox_get_dc_config_in get_dc = (struct cxl_mbox_get_dc_config_in) {
> > +		.partition_count = CXL_MAX_DC_PARTITIONS,
> > +		.start_partition_index = start_partition,
> > +	};
> > +	struct cxl_mbox_cmd mbox_cmd = (struct cxl_mbox_cmd) {
> > +		.opcode = CXL_MBOX_OP_GET_DC_CONFIG,
> > +		.payload_in = &get_dc,
> > +		.size_in = sizeof(get_dc),
> > +		.size_out = dc_resp_size,
> > +		.payload_out = dc_resp,
> > +		.min_out = 8,
> > +	};
> 
> [ ... ]
> 
> > +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);
> > +	struct device *dev = mbox->host;
> > +	u8 start_partition;
> > +	u8 num_partitions;
> > +	size_t dc_resp_size = struct_size(dc_resp,
> > +					  partition,
> > +					  CXL_MAX_DC_PARTITIONS);
> 
> [Severity: High]
> Can this static size calculation cause initialization failures on devices 
> with small mailboxes?
> 
> The code requests CXL_MAX_DC_PARTITIONS (8 partitions), resulting in an 
> expected response payload size of 328 bytes (an 8-byte header plus 8 
> 40-byte partition entries). However, the CXL specification allows a minimum 
> mailbox payload size of 256 bytes. 
> 
> If a device has a mailbox smaller than 328 bytes, cxl_internal_send_cmd() 
> will unconditionally fail with -E2BIG. Should the requested number of 
> partitions be dynamically bounded by the device's negotiated payload size?
> 
Fixed. # of partitions requested per iteration of the loop
capped to mailbox size.

> > +	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, 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;
> > +		}
> 
> [Severity: Medium]
> Does this unintentionally fail on valid devices configured with 0 Dynamic 
> Capacity partitions?
> 
> If a device supports DCD but is configured with zero partitions, 
> avail_partition_count is 0, and cxl_get_dc_config() legitimately returns 0. 
> This path treats it as a hard error and returns -EIO, logging a confusing 
> error message instead of cleanly disabling DCD and proceeding.
> 
> [ ... ]
> 
> > +void cxl_configure_dcd(struct cxl_memdev_state *mds, struct cxl_dpa_info *info)
> > +{
> > +	struct cxl_dc_partition_info dc_info = { 0 };
> > +	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);
> > +		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;
> > +	}
> 
> [Severity: High]
> Is it intended to compare dc_info.start against info->size here?
> 
> Because cxl_mem_dpa_fetch() initializes info->size to mds->total_bytes, 
> it already includes the dynamic capacity. 

no it doesn't. it only includes ram and pmem partitions.

> This means dc_info.start will 
> always be strictly less than info->size, causing this skip check to falsely 
> fail on every valid device with a DC partition and breaking DCD entirely.
> 
> > +
> > +	info->size += dc_info.size;
> 
> [Severity: High]
> Does this inadvertently double-count the dynamic capacity?
> 
no.

> Since info->size already includes the total device capacity, adding 
> dc_info.size here will inflate the total DPA span incorrectly.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2