| Message ID | 20250128-rfc-rearch-mem-res-v1-2-26d1ca151376@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl: Further clean up of memdev state | expand |
On 1/28/25 18:51, Ira Weiny wrote: > As was mentioned by Dan[1] cxl_memdev_state stores values which are only > used during device probe. This clutters the data structure and is a > hindrance on code maintenance. Those values are best handled with > temporary variables. > > Adjust the query of memory devices to read byte sizes in one call which > takes partition information into account. Use the values to create > partitions for device state initialization. Take care to separate the > mailbox queries from the initialization of device state to steer the > mbox code toward taking mailbox objects rather than memdev states. > Update spec references while changing these calls. > > Link: https://lore.kernel.org/all/67871f05cd767_20f32947f@dwillia2-xfh.jf.intel.com.notmuch/ [1] > Signed-off-by: Ira Weiny <ira.weiny@intel.com> Reviewed-by: Alejandro Lucero <alucerop@amd.com> FWIW, I had (as part of current in-progress v10) similar struct than used here for Type2 initialization when there is no mailbox. I had added a specific function for initialising that struct, but my idea now with this change is to have cxl_mem_dev_info initialized by the driver before calling cxl_dev_state_identify, and inside that function checking if total_bytes already != 0 for avoiding call the mbox command for getting the info. This will support both cases for Type2, with and without mailbox.
Alejandro Lucero Palau wrote: > > On 1/28/25 18:51, Ira Weiny wrote: > > As was mentioned by Dan[1] cxl_memdev_state stores values which are only > > used during device probe. This clutters the data structure and is a > > hindrance on code maintenance. Those values are best handled with > > temporary variables. > > > > Adjust the query of memory devices to read byte sizes in one call which > > takes partition information into account. Use the values to create > > partitions for device state initialization. Take care to separate the > > mailbox queries from the initialization of device state to steer the > > mbox code toward taking mailbox objects rather than memdev states. > > Update spec references while changing these calls. > > > > Link: https://lore.kernel.org/all/67871f05cd767_20f32947f@dwillia2-xfh.jf.intel.com.notmuch/ [1] > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > > Reviewed-by: Alejandro Lucero <alucerop@amd.com> > > > FWIW, I had (as part of current in-progress v10) similar struct than > used here for Type2 initialization when there is no mailbox. > > > I had added a specific function for initialising that struct, but my > idea now with this change is to have cxl_mem_dev_info initialized by the > driver before calling cxl_dev_state_identify, Why is the accelerator calling cxl_dev_state_identify? I did not see that in v9. My idea was that was a mailbox only call which is only needed for memdevs. And cxl_add_partition() can be called by accelerators as a convenience function to aid in creating cxl_dpa_info. (This and cxl_test needed that function shared so it just got left in mbox.c) > and inside that function I'm not clear which 'that function' you are referring to here. > checking if total_bytes already != 0 for avoiding call the mbox command > for getting the info. This will support both cases for Type2, with and > without mailbox. I think I agree with you except the != 0 to avoid mailbox commands. Unless I am miss-understanding Dan we need to get to a place where mailbox commands stop filling in structures unless those work for both type 2 and 3 __and__ are optional. Because putting in special checks for the type within a cxl/core/mailbox call is wrong IMO. Ira
On 1/28/25 11:51 AM, Ira Weiny wrote: > As was mentioned by Dan[1] cxl_memdev_state stores values which are only > used during device probe. This clutters the data structure and is a > hindrance on code maintenance. Those values are best handled with > temporary variables. > > Adjust the query of memory devices to read byte sizes in one call which > takes partition information into account. Use the values to create > partitions for device state initialization. Take care to separate the > mailbox queries from the initialization of device state to steer the > mbox code toward taking mailbox objects rather than memdev states. > Update spec references while changing these calls. > > Link: https://lore.kernel.org/all/67871f05cd767_20f32947f@dwillia2-xfh.jf.intel.com.notmuch/ [1] > Signed-off-by: Ira Weiny <ira.weiny@intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/mbox.c | 77 +++++++++++++++++--------------------------- > drivers/cxl/cxlmem.h | 25 +++++++------- > drivers/cxl/pci.c | 13 +++++--- > tools/testing/cxl/test/mem.c | 13 +++++--- > 4 files changed, 59 insertions(+), 69 deletions(-) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index 998e1df36db673c47c4e87b957df9c29bf3f291a..44618746ad79b0459501bb3001518f6b7d2ceaba 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -1068,6 +1068,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, "CXL"); > /** > * cxl_mem_get_partition_info - Get partition info > * @mds: The driver data for the operation > + * @dev_info: Device info results > * > * Retrieve the current partition info for the device specified. The active > * values are the current capacity in bytes. If not 0, the 'next' values are > @@ -1075,9 +1076,10 @@ EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, "CXL"); > * > * Return: 0 if no error: or the result of the mailbox command. > * > - * See CXL @8.2.9.5.2.1 Get Partition Info > + * See CXL 3.1 @8.2.9.9.2.1 Get Partition Info > */ > -static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds) > +static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds, > + struct cxl_mem_dev_info *dev_info) > { > struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox; > struct cxl_mbox_get_partition_info pi; > @@ -1093,9 +1095,9 @@ static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds) > if (rc) > return rc; > > - mds->active_volatile_bytes = > + dev_info->volatile_bytes = > le64_to_cpu(pi.active_volatile_cap) * CXL_CAPACITY_MULTIPLIER; > - mds->active_persistent_bytes = > + dev_info->persistent_bytes = > le64_to_cpu(pi.active_persistent_cap) * CXL_CAPACITY_MULTIPLIER; > > return 0; > @@ -1104,18 +1106,24 @@ static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds) > /** > * cxl_dev_state_identify() - Send the IDENTIFY command to the device. > * @mds: The driver data for the operation > + * @dev_info: Device info results > * > * Return: 0 if identify was executed successfully or media not ready. > * > - * This will dispatch the identify command to the device and on success populate > - * structures to be exported to sysfs. > + * This will dispatch the identify and partition info commands to the device > + * and on success populate structures required for the memory device to > + * operate. > + * > + * See CXL 3.1 @8.2.9.9.1.1 Identify Memory Device > */ > -int cxl_dev_state_identify(struct cxl_memdev_state *mds) > +int cxl_dev_state_identify(struct cxl_memdev_state *mds, > + struct cxl_mem_dev_info *dev_info) > { > struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox; > - /* See CXL 2.0 Table 175 Identify Memory Device Output Payload */ > + struct device *dev = cxl_mbox->host; > struct cxl_mbox_identify id; > struct cxl_mbox_cmd mbox_cmd; > + u64 partition_align_bytes; > u32 val; > int rc; > > @@ -1131,14 +1139,22 @@ int cxl_dev_state_identify(struct cxl_memdev_state *mds) > if (rc < 0) > return rc; > > - mds->total_bytes = > + dev_info->total_bytes = > le64_to_cpu(id.total_capacity) * CXL_CAPACITY_MULTIPLIER; > - mds->volatile_only_bytes = > + dev_info->volatile_bytes = > le64_to_cpu(id.volatile_capacity) * CXL_CAPACITY_MULTIPLIER; > - mds->persistent_only_bytes = > + dev_info->persistent_bytes = > le64_to_cpu(id.persistent_capacity) * CXL_CAPACITY_MULTIPLIER; > - mds->partition_align_bytes = > + > + partition_align_bytes = > le64_to_cpu(id.partition_align) * CXL_CAPACITY_MULTIPLIER; > + if (partition_align_bytes != 0) { > + rc = cxl_mem_get_partition_info(mds, dev_info); > + if (rc) { > + dev_err(dev, "Failed to query partition information\n"); > + return rc; > + } > + } > > mds->lsa_size = le32_to_cpu(id.lsa_size); > memcpy(mds->firmware_version, id.fw_revision, > @@ -1237,7 +1253,7 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd) > return rc; > } > > -static void add_part(struct cxl_dpa_info *info, u64 start, u64 size, enum cxl_partition_mode mode) > +void cxl_add_partition(struct cxl_dpa_info *info, u64 start, u64 size, enum cxl_partition_mode mode) > { > int i = info->nr_partitions; > > @@ -1251,40 +1267,7 @@ static void add_part(struct cxl_dpa_info *info, u64 start, u64 size, enum cxl_pa > info->part[i].mode = mode; > info->nr_partitions++; > } > - > -int cxl_mem_dpa_fetch(struct cxl_memdev_state *mds, struct cxl_dpa_info *info) > -{ > - struct cxl_dev_state *cxlds = &mds->cxlds; > - struct device *dev = cxlds->dev; > - int rc; > - > - if (!cxlds->media_ready) { > - info->size = 0; > - return 0; > - } > - > - info->size = mds->total_bytes; > - > - if (mds->partition_align_bytes == 0) { > - add_part(info, 0, mds->volatile_only_bytes, CXL_PARTMODE_RAM); > - add_part(info, mds->volatile_only_bytes, > - mds->persistent_only_bytes, CXL_PARTMODE_PMEM); > - return 0; > - } > - > - rc = cxl_mem_get_partition_info(mds); > - if (rc) { > - dev_err(dev, "Failed to query partition information\n"); > - return rc; > - } > - > - add_part(info, 0, mds->active_volatile_bytes, CXL_PARTMODE_RAM); > - add_part(info, mds->active_volatile_bytes, mds->active_persistent_bytes, > - CXL_PARTMODE_PMEM); > - > - return 0; > -} > -EXPORT_SYMBOL_NS_GPL(cxl_mem_dpa_fetch, "CXL"); > +EXPORT_SYMBOL_NS_GPL(cxl_add_partition, "CXL"); > > int cxl_set_timestamp(struct cxl_memdev_state *mds) > { > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > index 29817f533e5e408243d361c46ddbf0c295b4fda4..62e641d69eac975bae023f221c40713ad0317d1a 100644 > --- a/drivers/cxl/cxlmem.h > +++ b/drivers/cxl/cxlmem.h > @@ -542,12 +542,6 @@ static inline struct cxl_dev_state *mbox_to_cxlds(struct cxl_mailbox *cxl_mbox) > * @firmware_version: Firmware version for the memory device. > * @enabled_cmds: Hardware commands found enabled in CEL. > * @exclusive_cmds: Commands that are kernel-internal only > - * @total_bytes: sum of all possible capacities > - * @volatile_only_bytes: hard volatile capacity > - * @persistent_only_bytes: hard persistent capacity > - * @partition_align_bytes: alignment size for partition-able capacity > - * @active_volatile_bytes: sum of hard + soft volatile > - * @active_persistent_bytes: sum of hard + soft persistent > * @event: event log driver state > * @poison: poison driver state info > * @security: security driver state info > @@ -562,12 +556,6 @@ struct cxl_memdev_state { > char firmware_version[0x10]; > DECLARE_BITMAP(enabled_cmds, CXL_MEM_COMMAND_ID_MAX); > DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX); > - u64 total_bytes; > - u64 volatile_only_bytes; > - u64 persistent_only_bytes; > - u64 partition_align_bytes; > - u64 active_volatile_bytes; > - u64 active_persistent_bytes; > > struct cxl_event_state event; > struct cxl_poison_state poison; > @@ -885,10 +873,19 @@ enum { > > int cxl_internal_send_cmd(struct cxl_mailbox *cxl_mbox, > struct cxl_mbox_cmd *cmd); > -int cxl_dev_state_identify(struct cxl_memdev_state *mds); > + > +struct cxl_mem_dev_info { > + u64 total_bytes; > + u64 volatile_bytes; > + u64 persistent_bytes; > +}; > + > +int cxl_dev_state_identify(struct cxl_memdev_state *mds, > + struct cxl_mem_dev_info *dev_info); > +void cxl_add_partition(struct cxl_dpa_info *info, u64 start, u64 size, > + enum cxl_partition_mode mode); > int cxl_await_media_ready(struct cxl_dev_state *cxlds); > int cxl_enumerate_cmds(struct cxl_memdev_state *mds); > -int cxl_mem_dpa_fetch(struct cxl_memdev_state *mds, struct cxl_dpa_info *info); > struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev); > void set_exclusive_cxl_commands(struct cxl_memdev_state *mds, > unsigned long *cmds); > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index fa30e763a0ce4fd78441f486ce08c81e21207e29..b69ba756e7f722a327c42fb57f843635cf8367a2 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -903,6 +903,7 @@ __ATTRIBUTE_GROUPS(cxl_rcd); > static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > { > struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus); > + struct cxl_mem_dev_info dev_info = { 0 }; > struct cxl_dpa_info range_info = { 0 }; > struct cxl_memdev_state *mds; > struct cxl_dev_state *cxlds; > @@ -989,13 +990,17 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > if (rc) > return rc; > > - rc = cxl_dev_state_identify(mds); > + rc = cxl_dev_state_identify(mds, &dev_info); > if (rc) > return rc; > > - rc = cxl_mem_dpa_fetch(mds, &range_info); > - if (rc) > - return rc; > + if (cxlds->media_ready) { > + range_info.size = dev_info.total_bytes; > + cxl_add_partition(&range_info, 0, dev_info.volatile_bytes, > + CXL_PARTMODE_RAM); > + cxl_add_partition(&range_info, dev_info.volatile_bytes, > + dev_info.persistent_bytes, CXL_PARTMODE_PMEM); > + } > > rc = cxl_dpa_setup(cxlds, &range_info); > if (rc) > diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c > index ed365e083c8f545b6c1308f48b5f4f7bc51135e8..b88bc3fd8122e6dd2969d525e72f1ea8d5069913 100644 > --- a/tools/testing/cxl/test/mem.c > +++ b/tools/testing/cxl/test/mem.c > @@ -1477,6 +1477,7 @@ static int cxl_mock_mem_probe(struct platform_device *pdev) > struct cxl_dev_state *cxlds; > struct cxl_mockmem_data *mdata; > struct cxl_mailbox *cxl_mbox; > + struct cxl_mem_dev_info dev_info = { 0 }; > struct cxl_dpa_info range_info = { 0 }; > int rc; > > @@ -1534,13 +1535,17 @@ static int cxl_mock_mem_probe(struct platform_device *pdev) > return rc; > > cxlds->media_ready = true; > - rc = cxl_dev_state_identify(mds); > + rc = cxl_dev_state_identify(mds, &dev_info); > if (rc) > return rc; > > - rc = cxl_mem_dpa_fetch(mds, &range_info); > - if (rc) > - return rc; > + if (cxlds->media_ready) { > + range_info.size = dev_info.total_bytes; > + cxl_add_partition(&range_info, 0, dev_info.volatile_bytes, > + CXL_PARTMODE_RAM); > + cxl_add_partition(&range_info, dev_info.volatile_bytes, > + dev_info.persistent_bytes, CXL_PARTMODE_PMEM); > + } > > rc = cxl_dpa_setup(cxlds, &range_info); > if (rc) >
<snip> > >> FWIW, I had (as part of current in-progress v10) similar struct than >> used here for Type2 initialization when there is no mailbox. >> >> >> I had added a specific function for initialising that struct, but my >> idea now with this change is to have cxl_mem_dev_info initialized by the >> driver before calling cxl_dev_state_identify, > Why is the accelerator calling cxl_dev_state_identify? I did not see that > in v9. My idea was that was a mailbox only call which is only needed for > memdevs. And cxl_add_partition() can be called by accelerators as a > convenience function to aid in creating cxl_dpa_info. (This and cxl_test > needed that function shared so it just got left in mbox.c) No in v9 ... that predates Dan's DPA patches. Type2 without an mbox needs to give those values obtained from CXL_MBOX_OP_IDENTIFY. I'm using an struct for passing those values to cxl_dev_state_identify for having same function serving Type2 with and without mbox. I could have another function instead and calling that one for Type2 with mbox. My idea is to keep similar initialization than Type3 pci driver. >> and inside that function > I'm not clear which 'that function' you are referring to here. cxl_dev_state_identify >> checking if total_bytes already != 0 for avoiding call the mbox command >> for getting the info. This will support both cases for Type2, with and >> without mailbox. > I think I agree with you except the != 0 to avoid mailbox commands. > > Unless I am miss-understanding Dan we need to get to a place where mailbox > commands stop filling in structures unless those work for both type 2 and > 3 __and__ are optional. Because putting in special checks for the type > within a cxl/core/mailbox call is wrong IMO. As I said, I can avoid that check with a wrapper for Type2, then only Type2 with mbox and supporting that command (is it mandatory if an mbox?) will end up calling cxl_dev_state_identify. > Ira
Alejandro Lucero Palau wrote: > > <snip> > > > > >> FWIW, I had (as part of current in-progress v10) similar struct than > >> used here for Type2 initialization when there is no mailbox. > >> > >> > >> I had added a specific function for initialising that struct, but my > >> idea now with this change is to have cxl_mem_dev_info initialized by the > >> driver before calling cxl_dev_state_identify, > > Why is the accelerator calling cxl_dev_state_identify? I did not see that > > in v9. My idea was that was a mailbox only call which is only needed for > > memdevs. And cxl_add_partition() can be called by accelerators as a > > convenience function to aid in creating cxl_dpa_info. (This and cxl_test > > needed that function shared so it just got left in mbox.c) > > > No in v9 ... that predates Dan's DPA patches. > > > Type2 without an mbox needs to give those values obtained from > CXL_MBOX_OP_IDENTIFY. I'm using an struct for passing those values to > cxl_dev_state_identify for having same function serving Type2 with and > without mbox. But in the case of no mbox where does the type2 get those values from within cxl_dev_state_identify()? The idea with this patch change is to only call cxl_dev_state_identify IFF the device has a mailbox. Are you saying the same thing? > I could have another function instead and calling that one > for Type2 with mbox. My idea is to keep similar initialization than > Type3 pci driver. Agreed. But without an mbox I'm hoping you don't need to call cxl_dev_state_identify at all if you don't need to query the device... ie don't have a mailbox. > > > >> and inside that function > > I'm not clear which 'that function' you are referring to here. > > > cxl_dev_state_identify > > > >> checking if total_bytes already != 0 for avoiding call the mbox command > >> for getting the info. This will support both cases for Type2, with and > >> without mailbox. > > I think I agree with you except the != 0 to avoid mailbox commands. > > > > Unless I am miss-understanding Dan we need to get to a place where mailbox > > commands stop filling in structures unless those work for both type 2 and > > 3 __and__ are optional. Because putting in special checks for the type > > within a cxl/core/mailbox call is wrong IMO. > > > As I said, I can avoid that check with a wrapper for Type2, then only > Type2 with mbox and supporting that command (is it mandatory if an > mbox?) will end up calling cxl_dev_state_identify. Even if type 2 has a mailbox the partition information may or may not need to be in the identify command. Why force any type 2 to call that mailbox command? Ira
On Tue, 28 Jan 2025, Ira Weiny wrote: >As was mentioned by Dan[1] cxl_memdev_state stores values which are only >used during device probe. This clutters the data structure and is a >hindrance on code maintenance. Those values are best handled with >temporary variables. > >Adjust the query of memory devices to read byte sizes in one call which >takes partition information into account. Use the values to create >partitions for device state initialization. Take care to separate the >mailbox queries from the initialization of device state to steer the >mbox code toward taking mailbox objects rather than memdev states. >Update spec references while changing these calls. > >Link: https://lore.kernel.org/all/67871f05cd767_20f32947f@dwillia2-xfh.jf.intel.com.notmuch/ [1] >Signed-off-by: Ira Weiny <ira.weiny@intel.com> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
On Tue, 28 Jan 2025 12:51:08 -0600 Ira Weiny <ira.weiny@intel.com> wrote: > As was mentioned by Dan[1] cxl_memdev_state stores values which are only > used during device probe. This clutters the data structure and is a > hindrance on code maintenance. Those values are best handled with > temporary variables. > > Adjust the query of memory devices to read byte sizes in one call which > takes partition information into account. Use the values to create > partitions for device state initialization. Take care to separate the > mailbox queries from the initialization of device state to steer the > mbox code toward taking mailbox objects rather than memdev states. > Update spec references while changing these calls. Why not jump to 3.2? > > Link: https://lore.kernel.org/all/67871f05cd767_20f32947f@dwillia2-xfh.jf.intel.com.notmuch/ [1] > Signed-off-by: Ira Weiny <ira.weiny@intel.com> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Not sure why you had this as an RFC! Was it just that we are waiting for v3 from Dan? If so maybe Dan, just stick these on the back of your series if you are happen with these and make Dave's job a tiny bit easier (and so rebases happen without needing to sync the two sets). Jonathan
Jonathan Cameron wrote: > On Tue, 28 Jan 2025 12:51:08 -0600 > Ira Weiny <ira.weiny@intel.com> wrote: > > > As was mentioned by Dan[1] cxl_memdev_state stores values which are only > > used during device probe. This clutters the data structure and is a > > hindrance on code maintenance. Those values are best handled with > > temporary variables. > > > > Adjust the query of memory devices to read byte sizes in one call which > > takes partition information into account. Use the values to create > > partitions for device state initialization. Take care to separate the > > mailbox queries from the initialization of device state to steer the > > mbox code toward taking mailbox objects rather than memdev states. > > Update spec references while changing these calls. > Why not jump to 3.2? <shrug> Just been in the 3.1 spec for so long I forgot. > > > > Link: https://lore.kernel.org/all/67871f05cd767_20f32947f@dwillia2-xfh.jf.intel.com.notmuch/ [1] > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Not sure why you had this as an RFC! > Was it just that we are waiting for v3 from Dan? Yes. I wanted to get these out there for Alejandro to see the direction of the dev state call. > > If so maybe Dan, just stick these on the back of your series if you are > happen with these and make Dave's job a tiny bit easier (and so > rebases happen without needing to sync the two sets). That is fine with me if Dan has time. Ira
Ira Weiny wrote: > As was mentioned by Dan[1] cxl_memdev_state stores values which are only > used during device probe. This clutters the data structure and is a > hindrance on code maintenance. Those values are best handled with > temporary variables. > > Adjust the query of memory devices to read byte sizes in one call which > takes partition information into account. Use the values to create > partitions for device state initialization. Take care to separate the > mailbox queries from the initialization of device state to steer the > mbox code toward taking mailbox objects rather than memdev states. > Update spec references while changing these calls. > > Link: https://lore.kernel.org/all/67871f05cd767_20f32947f@dwillia2-xfh.jf.intel.com.notmuch/ [1] > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > --- So I am confused by some of the choices here, comments below... > drivers/cxl/core/mbox.c | 77 +++++++++++++++++--------------------------- > drivers/cxl/cxlmem.h | 25 +++++++------- > drivers/cxl/pci.c | 13 +++++--- > tools/testing/cxl/test/mem.c | 13 +++++--- > 4 files changed, 59 insertions(+), 69 deletions(-) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index 998e1df36db673c47c4e87b957df9c29bf3f291a..44618746ad79b0459501bb3001518f6b7d2ceaba 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -1068,6 +1068,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, "CXL"); > /** > * cxl_mem_get_partition_info - Get partition info > * @mds: The driver data for the operation > + * @dev_info: Device info results > * > * Retrieve the current partition info for the device specified. The active > * values are the current capacity in bytes. If not 0, the 'next' values are > @@ -1075,9 +1076,10 @@ EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, "CXL"); > * > * Return: 0 if no error: or the result of the mailbox command. > * > - * See CXL @8.2.9.5.2.1 Get Partition Info > + * See CXL 3.1 @8.2.9.9.2.1 Get Partition Info > */ > -static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds) > +static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds, > + struct cxl_mem_dev_info *dev_info) I was hoping this would get further away from new in/out arguments and look at centralizing all partition enumeration into one routine. That was the intent of cxl_mem_dpa_fetch() to capture all generic Memory Expander DPA boundary information. > { > struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox; > struct cxl_mbox_get_partition_info pi; > @@ -1093,9 +1095,9 @@ static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds) > if (rc) > return rc; > > - mds->active_volatile_bytes = > + dev_info->volatile_bytes = > le64_to_cpu(pi.active_volatile_cap) * CXL_CAPACITY_MULTIPLIER; > - mds->active_persistent_bytes = > + dev_info->persistent_bytes = > le64_to_cpu(pi.active_persistent_cap) * CXL_CAPACITY_MULTIPLIER; > > return 0; > @@ -1104,18 +1106,24 @@ static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds) > /** > * cxl_dev_state_identify() - Send the IDENTIFY command to the device. > * @mds: The driver data for the operation > + * @dev_info: Device info results > * > * Return: 0 if identify was executed successfully or media not ready. > * > - * This will dispatch the identify command to the device and on success populate > - * structures to be exported to sysfs. > + * This will dispatch the identify and partition info commands to the device > + * and on success populate structures required for the memory device to > + * operate. > + * > + * See CXL 3.1 @8.2.9.9.1.1 Identify Memory Device > */ > -int cxl_dev_state_identify(struct cxl_memdev_state *mds) > +int cxl_dev_state_identify(struct cxl_memdev_state *mds, > + struct cxl_mem_dev_info *dev_info) This function is tiny, and most of its work is just transfering parameters from device to an intermediate temporary object (@dev_info). I would say just subsume this function in its only caller and skip the @dev_info transfer step. > { > struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox; > - /* See CXL 2.0 Table 175 Identify Memory Device Output Payload */ > + struct device *dev = cxl_mbox->host; > struct cxl_mbox_identify id; > struct cxl_mbox_cmd mbox_cmd; > + u64 partition_align_bytes; > u32 val; > int rc; > > @@ -1131,14 +1139,22 @@ int cxl_dev_state_identify(struct cxl_memdev_state *mds) > if (rc < 0) > return rc; > > - mds->total_bytes = > + dev_info->total_bytes = > le64_to_cpu(id.total_capacity) * CXL_CAPACITY_MULTIPLIER; > - mds->volatile_only_bytes = > + dev_info->volatile_bytes = > le64_to_cpu(id.volatile_capacity) * CXL_CAPACITY_MULTIPLIER; > - mds->persistent_only_bytes = > + dev_info->persistent_bytes = > le64_to_cpu(id.persistent_capacity) * CXL_CAPACITY_MULTIPLIER; > - mds->partition_align_bytes = > + > + partition_align_bytes = > le64_to_cpu(id.partition_align) * CXL_CAPACITY_MULTIPLIER; > + if (partition_align_bytes != 0) { > + rc = cxl_mem_get_partition_info(mds, dev_info); > + if (rc) { > + dev_err(dev, "Failed to query partition information\n"); > + return rc; > + } > + } > > mds->lsa_size = le32_to_cpu(id.lsa_size); > memcpy(mds->firmware_version, id.fw_revision, > @@ -1237,7 +1253,7 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd) > return rc; > } > > -static void add_part(struct cxl_dpa_info *info, u64 start, u64 size, enum cxl_partition_mode mode) > +void cxl_add_partition(struct cxl_dpa_info *info, u64 start, u64 size, enum cxl_partition_mode mode) > { > int i = info->nr_partitions; > > @@ -1251,40 +1267,7 @@ static void add_part(struct cxl_dpa_info *info, u64 start, u64 size, enum cxl_pa > info->part[i].mode = mode; > info->nr_partitions++; > } > - > -int cxl_mem_dpa_fetch(struct cxl_memdev_state *mds, struct cxl_dpa_info *info) > -{ > - struct cxl_dev_state *cxlds = &mds->cxlds; > - struct device *dev = cxlds->dev; > - int rc; > - > - if (!cxlds->media_ready) { > - info->size = 0; > - return 0; > - } > - > - info->size = mds->total_bytes; > - > - if (mds->partition_align_bytes == 0) { > - add_part(info, 0, mds->volatile_only_bytes, CXL_PARTMODE_RAM); > - add_part(info, mds->volatile_only_bytes, > - mds->persistent_only_bytes, CXL_PARTMODE_PMEM); > - return 0; > - } > - > - rc = cxl_mem_get_partition_info(mds); > - if (rc) { > - dev_err(dev, "Failed to query partition information\n"); > - return rc; > - } > - > - add_part(info, 0, mds->active_volatile_bytes, CXL_PARTMODE_RAM); > - add_part(info, mds->active_volatile_bytes, mds->active_persistent_bytes, > - CXL_PARTMODE_PMEM); > - > - return 0; > -} > -EXPORT_SYMBOL_NS_GPL(cxl_mem_dpa_fetch, "CXL"); > +EXPORT_SYMBOL_NS_GPL(cxl_add_partition, "CXL"); I am not seeing the justification for both 'struct cxl_dpa_info' and 'struct cxl_mem_dev_info'. > > int cxl_set_timestamp(struct cxl_memdev_state *mds) > { > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > index 29817f533e5e408243d361c46ddbf0c295b4fda4..62e641d69eac975bae023f221c40713ad0317d1a 100644 > --- a/drivers/cxl/cxlmem.h > +++ b/drivers/cxl/cxlmem.h > @@ -542,12 +542,6 @@ static inline struct cxl_dev_state *mbox_to_cxlds(struct cxl_mailbox *cxl_mbox) > * @firmware_version: Firmware version for the memory device. > * @enabled_cmds: Hardware commands found enabled in CEL. > * @exclusive_cmds: Commands that are kernel-internal only > - * @total_bytes: sum of all possible capacities > - * @volatile_only_bytes: hard volatile capacity > - * @persistent_only_bytes: hard persistent capacity > - * @partition_align_bytes: alignment size for partition-able capacity > - * @active_volatile_bytes: sum of hard + soft volatile > - * @active_persistent_bytes: sum of hard + soft persistent > * @event: event log driver state > * @poison: poison driver state info > * @security: security driver state info > @@ -562,12 +556,6 @@ struct cxl_memdev_state { > char firmware_version[0x10]; > DECLARE_BITMAP(enabled_cmds, CXL_MEM_COMMAND_ID_MAX); > DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX); > - u64 total_bytes; > - u64 volatile_only_bytes; > - u64 persistent_only_bytes; > - u64 partition_align_bytes; > - u64 active_volatile_bytes; > - u64 active_persistent_bytes; > > struct cxl_event_state event; > struct cxl_poison_state poison; > @@ -885,10 +873,19 @@ enum { > > int cxl_internal_send_cmd(struct cxl_mailbox *cxl_mbox, > struct cxl_mbox_cmd *cmd); > -int cxl_dev_state_identify(struct cxl_memdev_state *mds); > + > +struct cxl_mem_dev_info { > + u64 total_bytes; > + u64 volatile_bytes; > + u64 persistent_bytes; > +}; > + > +int cxl_dev_state_identify(struct cxl_memdev_state *mds, > + struct cxl_mem_dev_info *dev_info); > +void cxl_add_partition(struct cxl_dpa_info *info, u64 start, u64 size, > + enum cxl_partition_mode mode); > int cxl_await_media_ready(struct cxl_dev_state *cxlds); > int cxl_enumerate_cmds(struct cxl_memdev_state *mds); > -int cxl_mem_dpa_fetch(struct cxl_memdev_state *mds, struct cxl_dpa_info *info); > struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev); > void set_exclusive_cxl_commands(struct cxl_memdev_state *mds, > unsigned long *cmds); > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index fa30e763a0ce4fd78441f486ce08c81e21207e29..b69ba756e7f722a327c42fb57f843635cf8367a2 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -903,6 +903,7 @@ __ATTRIBUTE_GROUPS(cxl_rcd); > static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > { > struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus); > + struct cxl_mem_dev_info dev_info = { 0 }; > struct cxl_dpa_info range_info = { 0 }; > struct cxl_memdev_state *mds; > struct cxl_dev_state *cxlds; > @@ -989,13 +990,17 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > if (rc) > return rc; > > - rc = cxl_dev_state_identify(mds); > + rc = cxl_dev_state_identify(mds, &dev_info); > if (rc) > return rc; > > - rc = cxl_mem_dpa_fetch(mds, &range_info); > - if (rc) > - return rc; > + if (cxlds->media_ready) { Why does media_ready affect partition boundary enumeration? > + range_info.size = dev_info.total_bytes; > + cxl_add_partition(&range_info, 0, dev_info.volatile_bytes, > + CXL_PARTMODE_RAM); > + cxl_add_partition(&range_info, dev_info.volatile_bytes, > + dev_info.persistent_bytes, CXL_PARTMODE_PMEM); > + } Why remove the cxl_mem_dpa_fetch() helper in favor of open-coding these cxl_add_partition() calls? > > rc = cxl_dpa_setup(cxlds, &range_info); > if (rc) > diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c > index ed365e083c8f545b6c1308f48b5f4f7bc51135e8..b88bc3fd8122e6dd2969d525e72f1ea8d5069913 100644 > --- a/tools/testing/cxl/test/mem.c > +++ b/tools/testing/cxl/test/mem.c > @@ -1477,6 +1477,7 @@ static int cxl_mock_mem_probe(struct platform_device *pdev) > struct cxl_dev_state *cxlds; > struct cxl_mockmem_data *mdata; > struct cxl_mailbox *cxl_mbox; > + struct cxl_mem_dev_info dev_info = { 0 }; > struct cxl_dpa_info range_info = { 0 }; > int rc; > > @@ -1534,13 +1535,17 @@ static int cxl_mock_mem_probe(struct platform_device *pdev) > return rc; > > cxlds->media_ready = true; > - rc = cxl_dev_state_identify(mds); > + rc = cxl_dev_state_identify(mds, &dev_info); > if (rc) > return rc; > > - rc = cxl_mem_dpa_fetch(mds, &range_info); > - if (rc) > - return rc; > + if (cxlds->media_ready) { > + range_info.size = dev_info.total_bytes; > + cxl_add_partition(&range_info, 0, dev_info.volatile_bytes, > + CXL_PARTMODE_RAM); > + cxl_add_partition(&range_info, dev_info.volatile_bytes, > + dev_info.persistent_bytes, CXL_PARTMODE_PMEM); > + } ...and here is duplicated logic that goes away if this stays centralized in cxl_mem_dpa_fetch().
Dan Williams wrote: > Ira Weiny wrote: Perhaps this would have been good to add to the commit message. <quote> The net-net of this change is to make partition set up 2 distinct steps. 1) query the device for total, ram, and pmem partition size information 2) create partitions using that information While doing so it avoids storing the total, ram, pmem sizes tuple in favor of a stack variable cxl_dev_info. </quote> [snip] > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > > index 998e1df36db673c47c4e87b957df9c29bf3f291a..44618746ad79b0459501bb3001518f6b7d2ceaba 100644 > > --- a/drivers/cxl/core/mbox.c > > +++ b/drivers/cxl/core/mbox.c > > @@ -1068,6 +1068,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, "CXL"); > > /** > > * cxl_mem_get_partition_info - Get partition info > > * @mds: The driver data for the operation > > + * @dev_info: Device info results > > * > > * Retrieve the current partition info for the device specified. The active > > * values are the current capacity in bytes. If not 0, the 'next' values are > > @@ -1075,9 +1076,10 @@ EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, "CXL"); > > * > > * Return: 0 if no error: or the result of the mailbox command. > > * > > - * See CXL @8.2.9.5.2.1 Get Partition Info > > + * See CXL 3.1 @8.2.9.9.2.1 Get Partition Info > > */ > > -static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds) > > +static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds, > > + struct cxl_mem_dev_info *dev_info) > > I was hoping this would get further away from new in/out arguments and > look at centralizing all partition enumeration into one routine. I don't understand? get partition info is only required if the partition align bytes is not 0. IOW if the device allows for partitions to be changed. cxl_mem_get_partition_info() is only called IFF that extra query is required. So this does centralize byte information queries into one routine. It leaves creating partitions to the device driver which moves us toward these being mailbox only calls... What I did fail to do is change mds to a mailbox. So add this hunk to this call with the corresponding change in the caller. diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index 44618746ad79..873793dab68e 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -1067,7 +1067,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, "CXL"); /** * cxl_mem_get_partition_info - Get partition info - * @mds: The driver data for the operation + * @mbox: Mailbox to query * @dev_info: Device info results * * Retrieve the current partition info for the device specified. The active @@ -1078,10 +1078,9 @@ EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, "CXL"); * * See CXL 3.1 @8.2.9.9.2.1 Get Partition Info */ -static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds, +static int cxl_mem_get_partition_info(struct cxl_mailbox *mbox, struct cxl_mem_dev_info *dev_info) { - struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox; struct cxl_mbox_get_partition_info pi; struct cxl_mbox_cmd mbox_cmd; int rc; @@ -1091,7 +1090,7 @@ static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds, .size_out = sizeof(pi), .payload_out = &pi, }; - rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd); + rc = cxl_internal_send_cmd(mbox, &mbox_cmd); if (rc) return rc; > That > was the intent of cxl_mem_dpa_fetch() to capture all generic Memory > Expander DPA boundary information. The problem with cxl_mem_dpa_fetch() is it mixes in the partition info mailbox call after you have already supposedly set up volatile/persistent byte settings. IOW I don't think it is clean to have the cxl_mem_get_partition_info() call hidden away in cxl_mem_dpa_fetch(). > > > { > > struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox; > > struct cxl_mbox_get_partition_info pi; > > @@ -1093,9 +1095,9 @@ static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds) > > if (rc) > > return rc; > > > > - mds->active_volatile_bytes = > > + dev_info->volatile_bytes = > > le64_to_cpu(pi.active_volatile_cap) * CXL_CAPACITY_MULTIPLIER; > > - mds->active_persistent_bytes = > > + dev_info->persistent_bytes = > > le64_to_cpu(pi.active_persistent_cap) * CXL_CAPACITY_MULTIPLIER; > > > > return 0; > > @@ -1104,18 +1106,24 @@ static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds) > > /** > > * cxl_dev_state_identify() - Send the IDENTIFY command to the device. > > * @mds: The driver data for the operation > > + * @dev_info: Device info results > > * > > * Return: 0 if identify was executed successfully or media not ready. > > * > > - * This will dispatch the identify command to the device and on success populate > > - * structures to be exported to sysfs. > > + * This will dispatch the identify and partition info commands to the device > > + * and on success populate structures required for the memory device to > > + * operate. > > + * > > + * See CXL 3.1 @8.2.9.9.1.1 Identify Memory Device > > */ > > -int cxl_dev_state_identify(struct cxl_memdev_state *mds) > > +int cxl_dev_state_identify(struct cxl_memdev_state *mds, > > + struct cxl_mem_dev_info *dev_info) > > This function is tiny, It's 47 lines long? > and most of its work is just transfering > parameters from device to an intermediate temporary object (@dev_info). Yes, by design. > I would say just subsume this function in its only caller and skip the > @dev_info transfer step. There are 2 callers including cxl_test. Further clean up here would be to add the other values to the dev_info tuple and make this a true mailbox call. Or split out the lsa and other values queries into another call. But that seemed overkill at this point. No one is needing those to be separated out. > > > { > > struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox; > > - /* See CXL 2.0 Table 175 Identify Memory Device Output Payload */ > > + struct device *dev = cxl_mbox->host; > > struct cxl_mbox_identify id; > > struct cxl_mbox_cmd mbox_cmd; > > + u64 partition_align_bytes; > > u32 val; > > int rc; > > > > @@ -1131,14 +1139,22 @@ int cxl_dev_state_identify(struct cxl_memdev_state *mds) > > if (rc < 0) > > return rc; > > > > - mds->total_bytes = > > + dev_info->total_bytes = > > le64_to_cpu(id.total_capacity) * CXL_CAPACITY_MULTIPLIER; > > - mds->volatile_only_bytes = > > + dev_info->volatile_bytes = > > le64_to_cpu(id.volatile_capacity) * CXL_CAPACITY_MULTIPLIER; > > - mds->persistent_only_bytes = > > + dev_info->persistent_bytes = > > le64_to_cpu(id.persistent_capacity) * CXL_CAPACITY_MULTIPLIER; > > - mds->partition_align_bytes = > > + > > + partition_align_bytes = > > le64_to_cpu(id.partition_align) * CXL_CAPACITY_MULTIPLIER; > > + if (partition_align_bytes != 0) { > > + rc = cxl_mem_get_partition_info(mds, dev_info); > > + if (rc) { > > + dev_err(dev, "Failed to query partition information\n"); > > + return rc; > > + } > > + } > > > > mds->lsa_size = le32_to_cpu(id.lsa_size); > > memcpy(mds->firmware_version, id.fw_revision, > > @@ -1237,7 +1253,7 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd) > > return rc; > > } > > > > -static void add_part(struct cxl_dpa_info *info, u64 start, u64 size, enum cxl_partition_mode mode) > > +void cxl_add_partition(struct cxl_dpa_info *info, u64 start, u64 size, enum cxl_partition_mode mode) > > { > > int i = info->nr_partitions; > > > > @@ -1251,40 +1267,7 @@ static void add_part(struct cxl_dpa_info *info, u64 start, u64 size, enum cxl_pa > > info->part[i].mode = mode; > > info->nr_partitions++; > > } > > - > > -int cxl_mem_dpa_fetch(struct cxl_memdev_state *mds, struct cxl_dpa_info *info) > > -{ > > - struct cxl_dev_state *cxlds = &mds->cxlds; > > - struct device *dev = cxlds->dev; > > - int rc; > > - > > - if (!cxlds->media_ready) { > > - info->size = 0; > > - return 0; > > - } > > - > > - info->size = mds->total_bytes; > > - > > - if (mds->partition_align_bytes == 0) { > > - add_part(info, 0, mds->volatile_only_bytes, CXL_PARTMODE_RAM); > > - add_part(info, mds->volatile_only_bytes, > > - mds->persistent_only_bytes, CXL_PARTMODE_PMEM); > > - return 0; > > - } > > - > > - rc = cxl_mem_get_partition_info(mds); > > - if (rc) { > > - dev_err(dev, "Failed to query partition information\n"); > > - return rc; > > - } > > - > > - add_part(info, 0, mds->active_volatile_bytes, CXL_PARTMODE_RAM); > > - add_part(info, mds->active_volatile_bytes, mds->active_persistent_bytes, > > - CXL_PARTMODE_PMEM); > > - > > - return 0; > > -} > > -EXPORT_SYMBOL_NS_GPL(cxl_mem_dpa_fetch, "CXL"); > > +EXPORT_SYMBOL_NS_GPL(cxl_add_partition, "CXL"); > > I am not seeing the justification for both 'struct cxl_dpa_info' and > 'struct cxl_mem_dev_info'. cxl_mem_dev_info is a convenient way to pass the (total, ram, pmem) byte tuple around and it gets declared on the stack so it is never stored. I'm open to different names... [snip] > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > > index fa30e763a0ce4fd78441f486ce08c81e21207e29..b69ba756e7f722a327c42fb57f843635cf8367a2 100644 > > --- a/drivers/cxl/pci.c > > +++ b/drivers/cxl/pci.c > > @@ -903,6 +903,7 @@ __ATTRIBUTE_GROUPS(cxl_rcd); > > static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > { > > struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus); > > + struct cxl_mem_dev_info dev_info = { 0 }; > > struct cxl_dpa_info range_info = { 0 }; > > struct cxl_memdev_state *mds; > > struct cxl_dev_state *cxlds; > > @@ -989,13 +990,17 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > if (rc) > > return rc; > > > > - rc = cxl_dev_state_identify(mds); > > + rc = cxl_dev_state_identify(mds, &dev_info); > > if (rc) > > return rc; > > > > - rc = cxl_mem_dpa_fetch(mds, &range_info); > > - if (rc) > > - return rc; > > + if (cxlds->media_ready) { > > Why does media_ready affect partition boundary enumeration? We want the driver to load without any partitions so that the device can be queried. See: commit e764f12208b99ac7892c4e3f6bf88d71ca71036f Author: Dave Jiang <dave.jiang@intel.com> Date: Thu May 18 16:38:20 2023 -0700 cxl: Move cxl_await_media_ready() to before capacity info retrieval Move cxl_await_media_ready() to cxl_pci probe before driver starts issuing IDENTIFY and retrieving memory device information to ensure that the device is ready to provide the information. Allow cxl_pci_probe() to succeed even if media is not ready. Cache the media failure in cxlds and don't ask the device for any media information. The rationale for proceeding in the !media_ready case is to allow for mailbox operations to interrogate and/or remediate the device. After media is repaired then rebinding the cxl_pci driver is expected to restart the capacity scan. Suggested-by: Dan Williams <dan.j.williams@intel.com> Fixes: b39cb1052a5c ("cxl/mem: Register CXL memX devices") Reviewed-by: Ira Weiny <ira.weiny@intel.com> Signed-off-by: Dave Jiang <dave.jiang@intel.com> Link: https://lore.kernel.org/r/168445310026.3251520.8124296540679268206.stgit@djiang5-mobl3 [djbw: fixup cxl_test] Signed-off-by: Dan Williams <dan.j.williams@intel.com> I explored getting rid of media_ready but I think that is a bigger fish to fry than I have time for ATM. And you have ideas there which I need to explore more. > > > + range_info.size = dev_info.total_bytes; > > + cxl_add_partition(&range_info, 0, dev_info.volatile_bytes, > > + CXL_PARTMODE_RAM); > > + cxl_add_partition(&range_info, dev_info.volatile_bytes, > > + dev_info.persistent_bytes, CXL_PARTMODE_PMEM); > > + } > > Why remove the cxl_mem_dpa_fetch() helper in favor of open-coding these > cxl_add_partition() calls? After removing the ugly hidden get partition info call in cxl_mem_dpa_fetch() cxl_mem_dpa_fetch boiled down to 2 cxl_add_partition() calls. That was very tiny. More importantly this exported a very clean cxl_add_parition() call for any driver (ie type 2) to call without creating special structures to pass to the cxl core. I can put cxl_mem_dpa_fetch() back if you want... I had changed it to cxl_mem_create_partitions() to make it more clear what it was actually doing before realizing it was so small it was probably best removed. It would be used both here and cxl_test. [snip] > > @@ -1534,13 +1535,17 @@ static int cxl_mock_mem_probe(struct platform_device *pdev) > > return rc; > > > > cxlds->media_ready = true; > > - rc = cxl_dev_state_identify(mds); > > + rc = cxl_dev_state_identify(mds, &dev_info); > > if (rc) > > return rc; > > > > - rc = cxl_mem_dpa_fetch(mds, &range_info); > > - if (rc) > > - return rc; > > + if (cxlds->media_ready) { > > + range_info.size = dev_info.total_bytes; > > + cxl_add_partition(&range_info, 0, dev_info.volatile_bytes, > > + CXL_PARTMODE_RAM); > > + cxl_add_partition(&range_info, dev_info.volatile_bytes, > > + dev_info.persistent_bytes, CXL_PARTMODE_PMEM); > > + } > > ...and here is duplicated logic that goes away if this stays centralized > in cxl_mem_dpa_fetch(). True but it was so little code. And exporting cxl_add_partition() seemed much cleaner as an example for type 2 devices. Ira
Ira Weiny wrote: > Dan Williams wrote: > > Ira Weiny wrote: > > Perhaps this would have been good to add to the commit message. > > <quote> > > The net-net of this change is to make partition set up 2 distinct steps. > > 1) query the device for total, ram, and pmem partition size information > 2) create partitions using that information > > While doing so it avoids storing the total, ram, pmem sizes tuple in favor of a > stack variable cxl_dev_info. > > </quote> > > [snip] > > > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > > > index 998e1df36db673c47c4e87b957df9c29bf3f291a..44618746ad79b0459501bb3001518f6b7d2ceaba 100644 > > > --- a/drivers/cxl/core/mbox.c > > > +++ b/drivers/cxl/core/mbox.c > > > @@ -1068,6 +1068,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, "CXL"); > > > /** > > > * cxl_mem_get_partition_info - Get partition info > > > * @mds: The driver data for the operation > > > + * @dev_info: Device info results > > > * > > > * Retrieve the current partition info for the device specified. The active > > > * values are the current capacity in bytes. If not 0, the 'next' values are > > > @@ -1075,9 +1076,10 @@ EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, "CXL"); > > > * > > > * Return: 0 if no error: or the result of the mailbox command. > > > * > > > - * See CXL @8.2.9.5.2.1 Get Partition Info > > > + * See CXL 3.1 @8.2.9.9.2.1 Get Partition Info > > > */ > > > -static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds) > > > +static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds, > > > + struct cxl_mem_dev_info *dev_info) > > > > I was hoping this would get further away from new in/out arguments and > > look at centralizing all partition enumeration into one routine. > > I don't understand? get partition info is only required if the partition align > bytes is not 0. IOW if the device allows for partitions to be changed. > cxl_mem_get_partition_info() is only called IFF that extra query is required. > So this does centralize byte information queries into one routine. It leaves > creating partitions to the device driver which moves us toward these being > mailbox only calls... The crux of the concern for me is less about the role of cxl_mem_get_partition_info() and more about the introduction of a new 'struct cxl_mem_dev_info' in/out parameter which is similar in function to 'struct cxl_dpa_info'. If you can find a way to avoid another level of indirection or otherwise consolidate all these steps into a straight line routine that does "all the DPA enumeration" things. > What I did fail to do is change mds to a mailbox. So add this hunk to this > call with the corresponding change in the caller. That's a whole separate conversion that can wait for an omnibus cleanup patch. > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index 44618746ad79..873793dab68e 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -1067,7 +1067,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, "CXL"); > > /** > * cxl_mem_get_partition_info - Get partition info > - * @mds: The driver data for the operation > + * @mbox: Mailbox to query > * @dev_info: Device info results > * > * Retrieve the current partition info for the device specified. The active > @@ -1078,10 +1078,9 @@ EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, "CXL"); > * > * See CXL 3.1 @8.2.9.9.2.1 Get Partition Info > */ > -static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds, > +static int cxl_mem_get_partition_info(struct cxl_mailbox *mbox, > struct cxl_mem_dev_info *dev_info) > { > - struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox; > struct cxl_mbox_get_partition_info pi; > struct cxl_mbox_cmd mbox_cmd; > int rc; > @@ -1091,7 +1090,7 @@ static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds, > .size_out = sizeof(pi), > .payload_out = &pi, > }; > - rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd); > + rc = cxl_internal_send_cmd(mbox, &mbox_cmd); > if (rc) > return rc; > > > > That > > was the intent of cxl_mem_dpa_fetch() to capture all generic Memory > > Expander DPA boundary information. > > The problem with cxl_mem_dpa_fetch() is it mixes in the partition info mailbox > call after you have already supposedly set up volatile/persistent byte > settings. > > IOW I don't think it is clean to have the cxl_mem_get_partition_info() call > hidden away in cxl_mem_dpa_fetch(). If it can save new in/out parameters and hide that thrash from cxl_pci_probe() outside of "get/set DPA information", then that's the final organization I would like to see for a cleanup like this. [..] > > Why does media_ready affect partition boundary enumeration? > > We want the driver to load without any partitions so that the device can be > queried. > [..] > I explored getting rid of media_ready but I think that is a bigger fish to fry > than I have time for ATM. And you have ideas there which I need to explore > more. It is jarring to see cxl_pci_probe() now handling this detail when previously cxl_pci_probe() just called a helper that either succeeds or fails without bothering cxl_pci_probe() with the details. Push / leave complexity and logic to leaf functions where possible. > > > + range_info.size = dev_info.total_bytes; > > > + cxl_add_partition(&range_info, 0, dev_info.volatile_bytes, > > > + CXL_PARTMODE_RAM); > > > + cxl_add_partition(&range_info, dev_info.volatile_bytes, > > > + dev_info.persistent_bytes, CXL_PARTMODE_PMEM); > > > + } > > > > Why remove the cxl_mem_dpa_fetch() helper in favor of open-coding these > > cxl_add_partition() calls? > > After removing the ugly hidden get partition info call in cxl_mem_dpa_fetch() > cxl_mem_dpa_fetch boiled down to 2 cxl_add_partition() calls. That was very > tiny. > > More importantly this exported a very clean cxl_add_parition() call for any > driver (ie type 2) to call without creating special structures to pass to the > cxl core. > > I can put cxl_mem_dpa_fetch() back if you want... I had changed it to > cxl_mem_create_partitions() to make it more clear what it was actually doing > before realizing it was so small it was probably best removed. > > It would be used both here and cxl_test. The name does not matter to me, although it is unfortunate that this helper lived for all of 2 commits in the history, it is more about adding a new potentially redundant in/out object and adding logic to cxl_pci_probe().
<snip> >>> I was hoping this would get further away from new in/out arguments and >>> look at centralizing all partition enumeration into one routine. >> I don't understand? get partition info is only required if the partition align >> bytes is not 0. IOW if the device allows for partitions to be changed. >> cxl_mem_get_partition_info() is only called IFF that extra query is required. >> So this does centralize byte information queries into one routine. It leaves >> creating partitions to the device driver which moves us toward these being >> mailbox only calls... > The crux of the concern for me is less about the role of > cxl_mem_get_partition_info() and more about the introduction of a new 'struct > cxl_mem_dev_info' in/out parameter which is similar in function to > 'struct cxl_dpa_info'. If you can find a way to avoid another level of > indirection or otherwise consolidate all these steps into a straight > line routine that does "all the DPA enumeration" things. > All this discussion after Dan's patches makes me feel miserable. I have already used those patches for the Type2 support, and I'm using a struct to be used by accel drivers without a mbox for setting up the current mds information, required for building up the DPA partitions used the new functions. In this case, a struct is needed for sure, because there are two alternatives which are more painful than using such a struct: - one alternative is to allow accel drivers to manipulate internal cxl structs what would require, to start with, to export them fully for accel drivers. Then those drivers doing non trivial work for something the current cxl core is already doing and, IMO, which could be reused hidden the complexity. - Another option, if not such new struct is used, is to pass the required data one by one, and although it could be fine by now, I think it is not as clean and it does not take into account potential changes hardcoded by accel drivers which would require further arguments. What I have now is quite similar to current pci driver, but using a new function for accel drivers (where that new struct is used) instead of cxl_dev_state_identify, although some accel drivers will just call that function if there exists an mbox. Then cxl_mem_dpa_fetch and cxl_dpa_setup will make all the work as they do for the pci driver. The change is simple and the code now cleaner than the previous versions where the DPA mess was still there. So, I'm happy with the DPA mess being solved, but ... ... what you seem to suggest now, and I mean both, Ira and Dan, is to optimize this which will make life harder for an accel driver. Or further helper functions will be needed, or the accel driver will need to do a lot of the work now performed by the core. I guess it is not a surprise if I speak up against any of these two possibilities. Maybe it is worth if you have a look at v10 that will be sent later today, early in the morning for you, where all this can hopefully be seen clearly.
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index 998e1df36db673c47c4e87b957df9c29bf3f291a..44618746ad79b0459501bb3001518f6b7d2ceaba 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -1068,6 +1068,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, "CXL"); /** * cxl_mem_get_partition_info - Get partition info * @mds: The driver data for the operation + * @dev_info: Device info results * * Retrieve the current partition info for the device specified. The active * values are the current capacity in bytes. If not 0, the 'next' values are @@ -1075,9 +1076,10 @@ EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, "CXL"); * * Return: 0 if no error: or the result of the mailbox command. * - * See CXL @8.2.9.5.2.1 Get Partition Info + * See CXL 3.1 @8.2.9.9.2.1 Get Partition Info */ -static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds) +static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds, + struct cxl_mem_dev_info *dev_info) { struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox; struct cxl_mbox_get_partition_info pi; @@ -1093,9 +1095,9 @@ static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds) if (rc) return rc; - mds->active_volatile_bytes = + dev_info->volatile_bytes = le64_to_cpu(pi.active_volatile_cap) * CXL_CAPACITY_MULTIPLIER; - mds->active_persistent_bytes = + dev_info->persistent_bytes = le64_to_cpu(pi.active_persistent_cap) * CXL_CAPACITY_MULTIPLIER; return 0; @@ -1104,18 +1106,24 @@ static int cxl_mem_get_partition_info(struct cxl_memdev_state *mds) /** * cxl_dev_state_identify() - Send the IDENTIFY command to the device. * @mds: The driver data for the operation + * @dev_info: Device info results * * Return: 0 if identify was executed successfully or media not ready. * - * This will dispatch the identify command to the device and on success populate - * structures to be exported to sysfs. + * This will dispatch the identify and partition info commands to the device + * and on success populate structures required for the memory device to + * operate. + * + * See CXL 3.1 @8.2.9.9.1.1 Identify Memory Device */ -int cxl_dev_state_identify(struct cxl_memdev_state *mds) +int cxl_dev_state_identify(struct cxl_memdev_state *mds, + struct cxl_mem_dev_info *dev_info) { struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox; - /* See CXL 2.0 Table 175 Identify Memory Device Output Payload */ + struct device *dev = cxl_mbox->host; struct cxl_mbox_identify id; struct cxl_mbox_cmd mbox_cmd; + u64 partition_align_bytes; u32 val; int rc; @@ -1131,14 +1139,22 @@ int cxl_dev_state_identify(struct cxl_memdev_state *mds) if (rc < 0) return rc; - mds->total_bytes = + dev_info->total_bytes = le64_to_cpu(id.total_capacity) * CXL_CAPACITY_MULTIPLIER; - mds->volatile_only_bytes = + dev_info->volatile_bytes = le64_to_cpu(id.volatile_capacity) * CXL_CAPACITY_MULTIPLIER; - mds->persistent_only_bytes = + dev_info->persistent_bytes = le64_to_cpu(id.persistent_capacity) * CXL_CAPACITY_MULTIPLIER; - mds->partition_align_bytes = + + partition_align_bytes = le64_to_cpu(id.partition_align) * CXL_CAPACITY_MULTIPLIER; + if (partition_align_bytes != 0) { + rc = cxl_mem_get_partition_info(mds, dev_info); + if (rc) { + dev_err(dev, "Failed to query partition information\n"); + return rc; + } + } mds->lsa_size = le32_to_cpu(id.lsa_size); memcpy(mds->firmware_version, id.fw_revision, @@ -1237,7 +1253,7 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd) return rc; } -static void add_part(struct cxl_dpa_info *info, u64 start, u64 size, enum cxl_partition_mode mode) +void cxl_add_partition(struct cxl_dpa_info *info, u64 start, u64 size, enum cxl_partition_mode mode) { int i = info->nr_partitions; @@ -1251,40 +1267,7 @@ static void add_part(struct cxl_dpa_info *info, u64 start, u64 size, enum cxl_pa info->part[i].mode = mode; info->nr_partitions++; } - -int cxl_mem_dpa_fetch(struct cxl_memdev_state *mds, struct cxl_dpa_info *info) -{ - struct cxl_dev_state *cxlds = &mds->cxlds; - struct device *dev = cxlds->dev; - int rc; - - if (!cxlds->media_ready) { - info->size = 0; - return 0; - } - - info->size = mds->total_bytes; - - if (mds->partition_align_bytes == 0) { - add_part(info, 0, mds->volatile_only_bytes, CXL_PARTMODE_RAM); - add_part(info, mds->volatile_only_bytes, - mds->persistent_only_bytes, CXL_PARTMODE_PMEM); - return 0; - } - - rc = cxl_mem_get_partition_info(mds); - if (rc) { - dev_err(dev, "Failed to query partition information\n"); - return rc; - } - - add_part(info, 0, mds->active_volatile_bytes, CXL_PARTMODE_RAM); - add_part(info, mds->active_volatile_bytes, mds->active_persistent_bytes, - CXL_PARTMODE_PMEM); - - return 0; -} -EXPORT_SYMBOL_NS_GPL(cxl_mem_dpa_fetch, "CXL"); +EXPORT_SYMBOL_NS_GPL(cxl_add_partition, "CXL"); int cxl_set_timestamp(struct cxl_memdev_state *mds) { diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index 29817f533e5e408243d361c46ddbf0c295b4fda4..62e641d69eac975bae023f221c40713ad0317d1a 100644 --- a/drivers/cxl/cxlmem.h +++ b/drivers/cxl/cxlmem.h @@ -542,12 +542,6 @@ static inline struct cxl_dev_state *mbox_to_cxlds(struct cxl_mailbox *cxl_mbox) * @firmware_version: Firmware version for the memory device. * @enabled_cmds: Hardware commands found enabled in CEL. * @exclusive_cmds: Commands that are kernel-internal only - * @total_bytes: sum of all possible capacities - * @volatile_only_bytes: hard volatile capacity - * @persistent_only_bytes: hard persistent capacity - * @partition_align_bytes: alignment size for partition-able capacity - * @active_volatile_bytes: sum of hard + soft volatile - * @active_persistent_bytes: sum of hard + soft persistent * @event: event log driver state * @poison: poison driver state info * @security: security driver state info @@ -562,12 +556,6 @@ struct cxl_memdev_state { char firmware_version[0x10]; DECLARE_BITMAP(enabled_cmds, CXL_MEM_COMMAND_ID_MAX); DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX); - u64 total_bytes; - u64 volatile_only_bytes; - u64 persistent_only_bytes; - u64 partition_align_bytes; - u64 active_volatile_bytes; - u64 active_persistent_bytes; struct cxl_event_state event; struct cxl_poison_state poison; @@ -885,10 +873,19 @@ enum { int cxl_internal_send_cmd(struct cxl_mailbox *cxl_mbox, struct cxl_mbox_cmd *cmd); -int cxl_dev_state_identify(struct cxl_memdev_state *mds); + +struct cxl_mem_dev_info { + u64 total_bytes; + u64 volatile_bytes; + u64 persistent_bytes; +}; + +int cxl_dev_state_identify(struct cxl_memdev_state *mds, + struct cxl_mem_dev_info *dev_info); +void cxl_add_partition(struct cxl_dpa_info *info, u64 start, u64 size, + enum cxl_partition_mode mode); int cxl_await_media_ready(struct cxl_dev_state *cxlds); int cxl_enumerate_cmds(struct cxl_memdev_state *mds); -int cxl_mem_dpa_fetch(struct cxl_memdev_state *mds, struct cxl_dpa_info *info); struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev); void set_exclusive_cxl_commands(struct cxl_memdev_state *mds, unsigned long *cmds); diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index fa30e763a0ce4fd78441f486ce08c81e21207e29..b69ba756e7f722a327c42fb57f843635cf8367a2 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -903,6 +903,7 @@ __ATTRIBUTE_GROUPS(cxl_rcd); static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) { struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus); + struct cxl_mem_dev_info dev_info = { 0 }; struct cxl_dpa_info range_info = { 0 }; struct cxl_memdev_state *mds; struct cxl_dev_state *cxlds; @@ -989,13 +990,17 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (rc) return rc; - rc = cxl_dev_state_identify(mds); + rc = cxl_dev_state_identify(mds, &dev_info); if (rc) return rc; - rc = cxl_mem_dpa_fetch(mds, &range_info); - if (rc) - return rc; + if (cxlds->media_ready) { + range_info.size = dev_info.total_bytes; + cxl_add_partition(&range_info, 0, dev_info.volatile_bytes, + CXL_PARTMODE_RAM); + cxl_add_partition(&range_info, dev_info.volatile_bytes, + dev_info.persistent_bytes, CXL_PARTMODE_PMEM); + } rc = cxl_dpa_setup(cxlds, &range_info); if (rc) diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c index ed365e083c8f545b6c1308f48b5f4f7bc51135e8..b88bc3fd8122e6dd2969d525e72f1ea8d5069913 100644 --- a/tools/testing/cxl/test/mem.c +++ b/tools/testing/cxl/test/mem.c @@ -1477,6 +1477,7 @@ static int cxl_mock_mem_probe(struct platform_device *pdev) struct cxl_dev_state *cxlds; struct cxl_mockmem_data *mdata; struct cxl_mailbox *cxl_mbox; + struct cxl_mem_dev_info dev_info = { 0 }; struct cxl_dpa_info range_info = { 0 }; int rc; @@ -1534,13 +1535,17 @@ static int cxl_mock_mem_probe(struct platform_device *pdev) return rc; cxlds->media_ready = true; - rc = cxl_dev_state_identify(mds); + rc = cxl_dev_state_identify(mds, &dev_info); if (rc) return rc; - rc = cxl_mem_dpa_fetch(mds, &range_info); - if (rc) - return rc; + if (cxlds->media_ready) { + range_info.size = dev_info.total_bytes; + cxl_add_partition(&range_info, 0, dev_info.volatile_bytes, + CXL_PARTMODE_RAM); + cxl_add_partition(&range_info, dev_info.volatile_bytes, + dev_info.persistent_bytes, CXL_PARTMODE_PMEM); + } rc = cxl_dpa_setup(cxlds, &range_info); if (rc)
As was mentioned by Dan[1] cxl_memdev_state stores values which are only used during device probe. This clutters the data structure and is a hindrance on code maintenance. Those values are best handled with temporary variables. Adjust the query of memory devices to read byte sizes in one call which takes partition information into account. Use the values to create partitions for device state initialization. Take care to separate the mailbox queries from the initialization of device state to steer the mbox code toward taking mailbox objects rather than memdev states. Update spec references while changing these calls. Link: https://lore.kernel.org/all/67871f05cd767_20f32947f@dwillia2-xfh.jf.intel.com.notmuch/ [1] Signed-off-by: Ira Weiny <ira.weiny@intel.com> --- drivers/cxl/core/mbox.c | 77 +++++++++++++++++--------------------------- drivers/cxl/cxlmem.h | 25 +++++++------- drivers/cxl/pci.c | 13 +++++--- tools/testing/cxl/test/mem.c | 13 +++++--- 4 files changed, 59 insertions(+), 69 deletions(-)