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

[Anisa Su <anisa.su887@gmail.com>]

On Tue, Sep 08, 2026 at 09:43:10PM +0100, Jonathan Cameron wrote:
> On Tue,  8 Sep 2026 03:15:06 -0700
> Anisa Su <anisa.su887@gmail.com> wrote:
> 
> > From: Ira Weiny <iweiny@kernel.org>
> > 
> > Devices which 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.
> 
> I don't see text saying quite this and have a concern around what
> might be interpreted as being required should we ever deal deeper
> device resets.  I think you could in practice cache the result of
> this.  There is no specific implication that it 'must' be issued.
> 
> The text I see that is relevant is
>   The basic sequence to utilize Dynamic Capacity include:
> ...
>    * Issue Get Dynamic Capacity Configuration command:  The device reports its
>      number of available regions and each region's base address, length, block
>      size, and DSMAD Handle.
> 
> I read that as meaning that the host does to get the info not that
> it is needed to configure DCDs on the device side.
> 
> Anyhow, I'd just reword a tiny bit to something like
>   CXL r4.0 section 9.13.3 describes the use of the Get DC Configuration command
>   in order to obtain the DCD region/partition characteristics.
> 
Sure, done.

> > 
> > 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
> Seems backwards. Perhaps
>   Initial enablement for Linux only supports one dynamic capacity partition.
> 
Makes sense, reworded.

> > all the partitions the device reports but validate only the first, and
> > configure it as 'dynamic ram 1'.
> > 
> > The specification requires that volatile capacity starts at DPA 0 and pmem
> > starts at the DPA immediately following it, but DC partitions only need
> > to be 256MB aligned according to CXL r4.0 section 8.2.10.9.9.1 Table 8-347.
> > So a device could leave a gap between ram/pmem (static) capacity and its first
> > DC partition, or between one DC partition and the next.
> > 
> > However, Linux follows the precedent set by PMEM/RAM partitions and requires the
> > first DC partition to begin at the DPA immediately following static
> > capacity.
> > 
> > Based on an original patch by Navneet Singh.
> > 
> > Signed-off-by: Ira Weiny <iweiny@kernel.org>
> > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > Tested-by: Wonjae Lee <wj28.lee@samsung.com>
> > Tested-by: Junhee Park <jh9934.park@samsung.com>
> > Tested-by: Heesoo Kim <habil.kim@samsung.com>
> 
> A couple of other things inline.
> 
> Thanks,
> 
> Jonathan
> 
> 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 199bb986d674..a484e23b2b3a 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -1349,6 +1349,241 @@ 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,
> > +			struct cxl_dc_partition *dev_part)
> > +{
> > +	u64 blk_size = le64_to_cpu(dev_part->block_size);
> > +	u64 len = le64_to_cpu(dev_part->length);
> > +
> > +	/*
> > +	 * Not an error; leave the entry empty. A partially zeroed partition
> > +	 * is rejected by the checks below. CXL r4.0 Table 8-347.
> > +	 */
> > +	if (cxl_dc_partition_unavailable(dev_part)) {
> > +		*part = (struct cxl_dc_partition_info) { };
> > +		dev_dbg(dev, "Partition 0 unavailable for DC\n");
> > +		return 0;
> > +	}
> > +
> > +	*part = (struct cxl_dc_partition_info) {
> > +		.start = le64_to_cpu(dev_part->base),
> > +		.size = le64_to_cpu(dev_part->decode_length) * CXL_CAPACITY_MULTIPLIER,
> > +	};
> > +
> > +	/*
> > +	 * 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
> 
> oh. That's ugly.  Worth just fixing?  I guess we don't want a general
> macro like that on the critical path for this patch.  Perhaps one to revisit.
>
+ 1 for revisiting.

> > +	 */
> > +	if (blk_size == 0 || (blk_size & (blk_size - 1)) ||
> > +	    blk_size % CXL_DCD_BLOCK_LINE_SIZE) {
> > +		dev_err(dev, "DC partition 0 invalid block size %#llx\n", blk_size);
> > +		return -EINVAL;
> > +	}
> 
> ...
> 
> 
> > +/* Returns the number of partitions in dc_resp or -ERRNO */
> > +static int cxl_get_dc_config(struct cxl_mailbox *mbox, u8 start_partition,
> > +			     u8 partition_count,
> > +			     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 = partition_count,
> > +		.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,
> > +		/* The device must return at least the fixed header */
> > +		.min_out = sizeof(*dc_resp),
> > +	};
> > +	size_t expected_sz;
> > +	int rc;
> > +
> > +	rc = cxl_internal_send_cmd(mbox, &mbox_cmd);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	/* A DCD reports between 1 and 8 partitions */
> > +	if (dc_resp->avail_partition_count == 0 ||
> > +	    dc_resp->avail_partition_count > CXL_MAX_DC_PARTITIONS) {
> 
> This strikes me as a compatibility issue waiting to happen.  Today the spec
> supports 8, but maybe in future it will support more and I'd not consider
> that a backwards compatibility break (which this would make it).  I'd clamp
> to CXL_MAX_DC_PARITIONS and maybe print with dev_info() if you see something
> bigger in the wild.
> 
I see... I did the below and moved it to the caller
cxl_dev_dc_identify() so the dev_err/dev_info messages are only printed
once, since the current function is called in a loop.

		avail = dc_resp->avail_partition_count;
		if (avail == 0) {
			dev_err(dev, "Device reported no DC partitions\n");
			return -EIO;
		}

		/* More than Linux handles is not a reason to refuse DCD */
		if (avail > CXL_MAX_DC_PARTITIONS) {
			dev_info(dev, "Device reported %u DC partitions, reading the first %u\n",
				 avail, CXL_MAX_DC_PARTITIONS);
			avail = 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;
> > +	}
> > +
> ...
> 
> 
> > +	/*
> > +	 * The payload carries trailing extent/tag count fields after the
> > +	 * partition array (CXL r4.0 Table 8-346) which the driver ignores, so
> > +	 * the response is at least, not exactly, expected_sz.
> > +	 */
> 
> More generally we shouldn't be checking that a record isn't longer than expected
> because of similar backwards compat concerns.  Check is always that it is at least
> as large as we need.
> 
Reworded to just:

/* The trailing extent/tag counts (CXL r4.0 Table 8-346) are not read */

so it doesn't imply that if we were to read the trailing fields, the
check here would be mbox.size_out == expected_sz

> > +	expected_sz = struct_size(dc_resp, partition,
> > +				  dc_resp->partitions_returned);
> > +
> > +	if (mbox_cmd.size_out < expected_sz) {
> > +		dev_err(mbox->host,
> > +			"Payload size %zu less than expected %zu for %u partitions\n",
> > +			mbox_cmd.size_out,
> > +			expected_sz,
> > +			dc_resp->partitions_returned);
> > +		return -EIO;
> > +	}
> > +
> > +	dev_dbg(mbox->host, "Read %d/%d DC partitions\n",
> > +		dc_resp->partitions_returned, dc_resp->avail_partition_count);
> > +	return dc_resp->partitions_returned;
> > +}
> > +
> 
> > +/**
> > + * 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 every partition the device reports, but validate only the first:
> > + * Linux maps partition 0 and nothing else, so a defect in capacity the
> > + * driver never touches is not a reason to refuse the device dynamic
> > + * capacity.  The remaining entries of @partitions are left unset.
> > + *
> > + * 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. The
> > +	 * 256 byte spec minimum, verified in cxl_pci_setup_mailbox(), keeps
> > +	 * the subtraction below from underflowing.
> > +	 */
> > +	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));
> 
> I doubt we need min_t() rather than min() but maybe I'm missing something.
> Pretty much anything can be compared with small constants without needing
> to specify the type used.
> 
I think min() should be fine. Changed to min().

Thanks,
Anisa
> > +	dc_resp_size = struct_size(dc_resp, partition, partition_count) +
> > +		       sizeof(struct cxl_mbox_get_dc_config_tail);
> > +