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

On Fri, Jul 31, 2026 at 09:01:05AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] False-positive error logging and initialization failure when a device has 0 dynamic capacity partitions configured.

Incorrect: Table 8-346 says "A DCD shall report between 1 and 8 regions.
All other values are reserved". 0 is illegal so failing is correct.

> - [Medium] The validation check `is_power_of_2(blk_size)` truncates the 64-bit block size on 32-bit architectures, allowing invalid block sizes to pass validation.

Fixed. is_power_of_2(unsigned long n) indeed truncates on 32 bit. Replaced
with (blk_size & (blk_size - 1)).

> --
> 
> --- Patch [2]: [PATCH v12 2/8] cxl/mem: Read dynamic capacity configuration from the device ---
> commit 7a0233410b5ae133da2a756fa11cf61b2145cace
> Author: Ira Weiny <[email protected]>
> 
> cxl/mem: Read dynamic capacity configuration from the device
> 
> This commit implements the Dynamic Capacity (DC) mailbox commands to read
> and store the DCD configuration information from a CXL device. It reads
> and validates all partitions but configures only the first one.
> 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> 
> [ ... ]
> 
> > +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);
> 
> [ ... ]
> 
> > +	if (blk_size == 0 || blk_size % CXL_DCD_BLOCK_LINE_SIZE ||
> > +	    !is_power_of_2(blk_size)) {
> 
> [Severity: Medium]
> Could passing the 64-bit blk_size to is_power_of_2() cause silent truncation
> on 32-bit architectures?
> 
> is_power_of_2() takes an unsigned long, which might allow an invalid 64-bit
> block size that is not a power of 2 (but whose lower 32 bits form a power of 2)
> to bypass the validation check.
> 

Fixed:

	/*
	 * Block size is a power of 2 and a multiple of 40h. is_power_of_2()
	 * takes an unsigned long, which truncates blk_size on 32 bit.
	 */
	if (blk_size == 0 || (blk_size & (blk_size - 1)) ||
	    blk_size % CXL_DCD_BLOCK_LINE_SIZE) {

> > +		dev_err(dev, "DC partition %d invalid block size %llu\n",
> > +			index, blk_size);
> > +		return -EINVAL;
> > +	}
> 
> [ ... ]
> 
> > +int cxl_dev_dc_identify(struct cxl_mailbox *mbox,
> > +			struct cxl_dc_partition_info *dc_info)
> > +{
> 
> [ ... ]
> 
> > +	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;
> > +		}
> 
> [Severity: Medium]
> Does this code incorrectly fail initialization when a valid CXL device has 0
> dynamic capacity partitions configured?
> 
No, such a device is not valid. CXL r4.0 Table 8-346, Number of Available
Regions: "The device shall report the total number of available Dynamic
Capacity Regions. Each region may be unconfigured or configured with a
different block size and capacity. A DCD shall report between 1 and 8
regions. All other values are reserved."

So 0 is a reserved value and failing is the right response. Note the
distinction the table draws: a DCD always reports at least one region,
but the regions it reports may be unconfigured. An unconfigured region
reports zeros for decode length, length and block size, and v13 skips
those rather than rejecting them, after Richard Cheng raised the issue
on this patch.

Alison asked separately for the count to be range checked, so v13 also
validates it where the payload is parsed, before it is used as the read
loop bound:
	/* A DCD reports between 1 and 8 partitions */
	if (dc_resp->avail_partition_count == 0 ||
	    dc_resp->avail_partition_count > CXL_MAX_DC_PARTITIONS) {
		dev_err(mbox->host,
			"Device reported %u available DC partitions, expected 1 to %u\n",
			dc_resp->avail_partition_count, CXL_MAX_DC_PARTITIONS);
		return -EIO;
	}
A device reporting 0 now fails there with a message naming the field,
rather than reaching the rc == 0 path you quoted.

- Anisa

> Since this loop is implemented as a do-while loop, it will always execute at
> least once. If a device has 0 available partitions, it will return 0
> partitions (rc == 0), which causes this path to log an error and abort
> initialization with -EIO.
> 
> > +
> > +		num_partitions += rc;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2