On Mon, 12 Oct 2026 12:47:14 +0100 Jonathan Cameron <[email protected]> wrote:
> On Sun, 11 Oct 2026 07:54:53 +0900 > Anisa Su <[email protected]> wrote: > > > From: Ira Weiny <[email protected]> > > > > Devices which support Dynamic Capacity (DC) are configured via mailbox > > commands. CXL r4.0 section 9.13.3 describes the use of the Get DC > > Configuration command in order to obtain DCD partition characteristics. > > > > 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. > > > > Initial enablement for Linux only supports one dynamic capacity > > partition. Read all the partitions the device reports but validate > > only the first, and configure it as 'dynamic ram'. > > > > 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 chooses not to support gaps between static and dynamic > > capacity, 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 <[email protected]> > > Co-developed-by: Anisa Su <[email protected]> > > Signed-off-by: Anisa Su <[email protected]> > > Tested-by: Wonjae Lee <[email protected]> > > Tested-by: Junhee Park <[email protected]> > > Tested-by: Heesoo Kim <[email protected]> > > > > A couple of things inline. Only one that matters I think is the > cleanup.h fragility / readability thing about constructors and > destructors needing to be near each other. That bit of guidance > came from a specific (and I think correct) comment from Linus > that he really wanted them together and if that meant declaring > the variables inline rather than in old school c style, then so be > it. That might be in category of things Dave could cleanup up whilst > applying if all else is ready but that's up to him. > Forgot to add, with that tweak Reviewed-by: Jonathan Cameron <[email protected]> > Jonathan > > > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > > index 199bb986d674..72af23ba288e 100644 > > --- a/drivers/cxl/core/mbox.c > > +++ b/drivers/cxl/core/mbox.c > > > +/* 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; > > + > > + if (dc_resp->partitions_returned > partition_count) { > > + dev_err(mbox->host, "Device returned %u partitions, requested > > %u\n", > > + dc_resp->partitions_returned, partition_count); > > + return -EIO; > > + } > > + > > + /* The trailing extent/tag counts (CXL r4.0 Table 8-346) are not read */ > > + 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); > > Not important, but if you do end up respinning, maybe don't have one > parameter per line. > To me it doesn't really help with readability here. > > > + return -EIO; > > + } > > + > > +/** > > + * 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 partitions are read but not recorded. > > + * > > + * Return: 0 if identify was executed successfully, -ERRNO on error. > > + * On error, @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; > > See the cleanup.h guidance. This needs to move. > > > + struct device *dev = mbox->host; > > + u8 avail_partitions = 0; > > + 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(CXL_MAX_DC_PARTITIONS, > > + (mbox->payload_size - sizeof(*dc_resp) - > > + sizeof(struct cxl_mbox_get_dc_config_tail)) / > > + sizeof(struct cxl_dc_partition)); > > + dc_resp_size = struct_size(dc_resp, partition, partition_count) + > > + sizeof(struct cxl_mbox_get_dc_config_tail); > > + > > struct cxl_mbox_get_dc_config_out *dc_resp __free(kfree) = > kmalloc(dc_resp_size, GFP_KERNEL); > if (!dc_resp) > > Maybe if everything else is fine, Dave can tweak this whilst applying. > > star> + if (!dc_resp) > > + return -ENOMEM; > > + > > + start_partition = 0; > > + num_partitions = 0; > > + do { > > + int returned, rc; > > + > > + returned = cxl_get_dc_config(mbox, start_partition, > > + partition_count, dc_resp, > > + dc_resp_size); > > + if (returned < 0) { > > + dev_err(dev, "Failed to get DC config: %d\n", returned); > > + return returned; > > + } > > + > > + if (returned == 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; > > + } > > + > > + /* A DCD reports at least one partition (CXL r4.0 Table 8-346) > > */ > > + if (dc_resp->avail_partition_count == 0) { > > + dev_err(dev, "Device reported no DC partitions\n"); > > + return -EIO; > > + } > > + > > + avail_partitions = min(dc_resp->avail_partition_count, > > + CXL_MAX_DC_PARTITIONS); > > + > > + /* > > + * Only partition 0 is mapped, so it is the only one checked. > > + * The rest are read to complete the enumeration and dropped. > > + */ > > + if (start_partition == 0) { > > + rc = cxl_dc_check(dev, &partitions[0], > > + &dc_resp->partition[0]); > > + if (rc) > > + return rc; > > + } > > + > > + num_partitions += returned; > > + start_partition = num_partitions; > > I may be missing something subtle, but do start_partition and num_partitions > ever > differ in value? Far as I can spot they are both only updated here. If they > change in later patches and this is churn reduction ignore this comment! > > > > + > > + } while (num_partitions < avail_partitions); > > + > > + /* Linux only supports the 1st partition; nothing to do if it is > > unavailable */ > > + if (partitions[0].size == 0) > > + return -ENODEV; > > + > > + *dc_info = partitions[0]; > > + dev_dbg(dev, "Returning partition 0 %#llx size %#llx\n", > > + dc_info->start, dc_info->size); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_NS_GPL(cxl_dev_dc_identify, "CXL"); >
