Message-ID: <67a50c15de8e1_305d76294d2@iweiny-mobl.notmuch>
Date: Thu, 6 Feb 2025 13:23:01 -0600
From: Ira Weiny <ira.weiny@...el.com>
To: Alejandro Lucero Palau <alucerop@....com>, Ira Weiny
	<ira.weiny@...el.com>, <linux-cxl@...r.kernel.org>, <netdev@...r.kernel.org>,
	<dan.j.williams@...el.com>, <edward.cree@....com>, <davem@...emloft.net>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <edumazet@...gle.com>,
	<dave.jiang@...el.com>
Subject: Re: [PATCH v10 09/26] cxl: support device identification without
 mailbox

Alejandro Lucero Palau wrote:
> 
> On 2/5/25 21:45, Ira Weiny wrote:
> > alucerop@ wrote:
> >> From: Alejandro Lucero <alucerop@....com>
> >>
> >> Type3 relies on mailbox CXL_MBOX_OP_IDENTIFY command for initializing
> >> memdev state params.
> >>
> >> Allow a Type2 driver to initialize same params using an info struct and
> >> assume partition alignment not required by now.
> >>
> >> Signed-off-by: Alejandro Lucero <alucerop@....com>
> > This is exactly the type of thing I was hoping to avoid by removing these
> > members from the mds.  There is no reason you should have to fake these
> > values within an mds just to create partitions in the device state.
> 
> 
> Let's be practical here.
> 
> 
> A type2 without a mailbox needs to give that information for building up 
> the DPA partitions. Before it was about dealing with DPA resources from 
> the accel driver, but I do not think an accel driver should handle any 
> partition setup at all.

I 100% totally agree!  However, the dev state is where those partitions are
managed.  Not the memdev state.

> Mainly because there is code now doing that in 
> the cxl core which can be used for accel drivers without requiring too 
> much effort. You can see what the sfc driver does now, and it is 
> equivalent to the current pci driver. An accel driver with a device 
> supporting a mailbox will do exactly the same than the pci driver.
> 

I agree that the effort you made in these patches was not huge.  Changing the
types around and defining mds_info is not hard.  But the final result is odd
and does not fix a couple of the issues Dan had with the core architecture.
First of which is the carrying of initialization values in the memdev
state:[1]

[1]

	> @@ -473,7 +488,9 @@ static inline struct cxl_dev_state *mbox_to_cxlds(struct cxl_mailbox *cxl_mbox)
	>   * @dcd_cmds: List of DCD commands implemented by 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
	> + * @total_bytes: length of all possible capacities
	> + * @static_bytes: length of possible static RAM and PMEM partitions
	> + * @dynamic_bytes: length of possible DC partitions (DC Regions)
	>   * @volatile_only_bytes: hard volatile capacity
	>   * @persistent_only_bytes: hard persistent capacity
	
	I have regrets that cxl_memdev_state permanently carries runtime
	storage for init time variables, lets not continue down that path
	with DCD enabling.

	-- https://lore.kernel.org/all/67871f05cd767_20f32947f@dwillia2-xfh.jf.intel.com.notmuch/

> For avoiding the mds fields the weight should not be on the accel 
> driver.

I agree.  So why would you want to use the mds fields at all?

I proposed a helper function to create cxl_dpa_info [cxl_add_partition] and Dan
proposed a function to create the partitions from cxl_dpa_info
[cxl_dpa_setup].[2]

[2]

   void cxl_add_partition(struct cxl_dpa_info *info, u64 start, u64 size, enum cxl_partition_mode mode)
   int cxl_dpa_setup(struct cxl_dev_state *cxlds, const struct cxl_dpa_info *info)

	-- https://lore.kernel.org/all/20250128-rfc-rearch-mem-res-v1-2-26d1ca151376@intel.com/

What more do you need?

> This patch adds a way for giving the required (and little) info 
> to the core for building the partitions.

The second issue with your patch set is in the addition of struct mds_info.
This has the same issue which Dan objected to about creating a temporary
variable[3] but this is worse than my proposal in that your set continues to
carry the initialization state around in the memdev forever.

[3]

	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.

	-- https://lore.kernel.org/all/67a28921ca0b5_2d2c29434@dwillia2-xfh.jf.intel.com.notmuch/


Note to Dan.  I think doing 'all the DPA enumeration' things is the issue
here.  DCD further complicates this because it adds an additional DPA
discovery mechanism.  In summary we have:

	1) Identify Memory Device (existing)
	2) Hard coded values (Alejandro's type 2 set)
	3) Get dynamic capacity configuration (DCD set)

It is conceivable that a device might want to do some random combination of
those.  But the combinations we have in front of us are:

	A) 1 only
	B) 2 only
	C) 1 & 3

I'm not sure it is worth having a single call which attempts to enumerate the
dpa info.  I'll explore having a call which does A & C for mailbox supported
devices.  But B was specifically in my mind when I came up with the
cxl_add_partition() call.  And I felt using it in A and C would work just
fine.

> So if you or Dan suggest this 
> is wrong and the accel driver should deal with the intrinsics of DPA 
> partitions, I will fight against it :-)

I don't want an accel driver to deal with the intrinsics of the DPA
partitions at all!  But it should be able to specify the size parameters
separate from creating dummy memdev state objects with values it does not
care about.

> 
> I'm quite happy with the DPA partition work,

As am I.  I'm just trying to go a step further so it fits a bit cleaner
when DCD comes along.  I do apologize for the delay and churn in your set.
That was not my intention.  But I thought the alterations of the memdev
state were a good clean up.

> with the result of current 
> v10 being simpler and cleaner. But it is time to get the patchsets 
> depending on that cleaning work going forward.

Agreed.

If Dan likes what you have here I will adjust the DCD work.

Ira

Powered by blists - more mailing lists