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. - [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. --
--- 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. > + 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? 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
