Re: [PATCH RFC 2/2] cxl/memdev: Remove temporary variables from cxl_memdev_state

Dan Williams
2025-02-05T00:16:07Z

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().