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
