Message-ID: <67a2adbdde7c6_2d2c294b5@dwillia2-xfh.jf.intel.com.notmuch>
Date: Tue, 4 Feb 2025 16:15:57 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Ira Weiny <ira.weiny@...el.com>, Dan Williams <dan.j.williams@...el.com>,
	Dave Jiang <dave.jiang@...el.com>, Davidlohr Bueso <dave@...olabs.net>,
	Jonathan Cameron <jonathan.cameron@...wei.com>, Alison Schofield
	<alison.schofield@...el.com>, Vishal Verma <vishal.l.verma@...el.com>
CC: <linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Ira Weiny
	<ira.weiny@...el.com>
Subject: Re: [PATCH RFC 2/2] cxl/memdev: Remove temporary variables from
 cxl_memdev_state

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

Powered by blists - more mailing lists