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. - [High] Incorrect unit conversion of DC partition decode_length causes severe integer overflow. - [High] Logic Error in DPA Skip Check mathematically guarantees failure, breaking DCD on all devices. - [Medium] Valid CXL devices configured with 0 Dynamic Capacity partitions are erroneously treated as having an I/O 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? 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 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? > + 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. 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? 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
