Re: [RFC type cxl initialization 1/2] cxl: add type2 device basic support

[Ben Cheatham <benjamin.cheatham@amd.com>]

On 2/20/25 2:00 PM, alejandro.lucero-palau@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> Differentiate CXL memory expanders (type 3) from CXL device accelerators
> (type 2) with a new function for initializing cxl_dev_state and a macro
> for helping accel drivers to embed cxl_dev_state inside a private
> struct.
> 
> Move structs to include/cxl as the size of the accel driver private
> struct embedding cxl_dev_state needs to know the size of this struct.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/core/mbox.c   |   3 +-
>  drivers/cxl/core/memdev.c |  25 +++++
>  drivers/cxl/core/pci.c    |   1 +
>  drivers/cxl/core/regs.c   |   1 +
>  drivers/cxl/cxl.h         |  98 +----------------
>  drivers/cxl/cxlmem.h      | 108 +-----------------
>  drivers/cxl/cxlpci.h      |  21 ----
>  drivers/cxl/pci.c         |  17 +--
>  include/cxl/cxl.h         | 225 ++++++++++++++++++++++++++++++++++++++
>  include/cxl/pci.h         |  23 ++++
>  10 files changed, 293 insertions(+), 229 deletions(-)

[snip]

> +/**
> + * struct cxl_memdev - CXL bus object representing a Type-3 Memory Device
> + * @dev: driver core device object
> + * @cdev: char dev core object for ioctl operations
> + * @cxlds: The device state backing this device
> + * @detach_work: active memdev lost a port in its ancestry
> + * @cxl_nvb: coordinate removal of @cxl_nvd if present
> + * @cxl_nvd: optional bridge to an nvdimm if the device supports pmem
> + * @endpoint: connection to the CXL port topology for this memory device
> + * @id: id number of this memdev instance.
> + * @depth: endpoint port depth
> + */
> +struct cxl_memdev {
> +	struct device dev;
> +	struct cdev cdev;
> +	struct cxl_dev_state *cxlds;
> +	struct work_struct detach_work;
> +	struct cxl_nvdimm_bridge *cxl_nvb;
> +	struct cxl_nvdimm *cxl_nvd;
> +	struct cxl_port *endpoint;
> +	int id;
> +	int depth;
> +};

This can stay in cxlmem.h, only a pointer is being used in cxl_dev_state.

> +
> +#define CXL_NR_PARTITIONS_MAX 2
> +
> +/**
> + * struct cxl_dev_state - The driver device state
> + *
> + * cxl_dev_state represents the CXL driver/device state.  It provides an
> + * interface to mailbox commands as well as some cached data about the device.
> + * Currently only memory devices are represented.
> + *
> + * @dev: The device associated with this CXL state
> + * @cxlmd: The device representing the CXL.mem capabilities of @dev
> + * @reg_map: component and ras register mapping parameters
> + * @regs: Parsed register blocks
> + * @cxl_dvsec: Offset to the PCIe device DVSEC
> + * @rcd: operating in RCD mode (CXL 3.0 9.11.8 CXL Devices Attached to an RCH)
> + * @media_ready: Indicate whether the device media is usable
> + * @dpa_res: Overall DPA resource tree for the device
> + * @part: DPA partition array
> + * @nr_partitions: Number of DPA partitions
> + * @serial: PCIe Device Serial Number
> + * @type: Generic Memory Class device or Vendor Specific Memory device
> + * @cxl_mbox: CXL mailbox context
> + */
> +struct cxl_dev_state {
> +	struct device *dev;
> +	struct cxl_memdev *cxlmd;
> +	struct cxl_register_map reg_map;
> +	struct cxl_regs regs;
> +	int cxl_dvsec;
> +	bool rcd;
> +	bool media_ready;
> +	struct resource dpa_res;
> +	struct cxl_dpa_partition part[CXL_NR_PARTITIONS_MAX];
> +	unsigned int nr_partitions;
> +	u64 serial;
> +	enum cxl_devtype type;
> +	struct cxl_mailbox cxl_mbox;
> +};
> +
> +struct cxl_dev_state *_cxl_dev_state_create(struct device *dev,
> +					   u64 serial, u16 dvsec,
> +					   size_t size);
> +
> +#define cxl_dev_state_create(parent, serial, dvsec, drv_struct, member) \
> +	({								      \
> +	 	static_assert(__same_type(struct cxl_dev_state, 	      \
> +					((drv_struct *)NULL)->member));	      \
> +		static_assert(offsetof(drv_struct, member) == 0);	      \
> +		(drv_struct *)_cxl_dev_state_create(parent, serial, dvsec,    \
> +						    sizeof(drv_struct));\
> +	})

This should take a CXL device type parameter as well, then you can also use it in
cxl_memdev_state_create() like this:

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 2c49e33851b2..14d7f93e4584 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1484,24 +1484,31 @@ int cxl_mailbox_init(struct cxl_mailbox *cxl_mbox, struct device *host)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_mailbox_init, "CXL");

+static void cxl_memdev_state_destroy(void *cxlmds)
+{
+       kfree(cxlmds);
+}
+
 struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev, u64 serial,
                                                 u16 dvsec)
 {
        struct cxl_memdev_state *mds;
        int rc;

-       mds = devm_kzalloc(dev, sizeof(*mds), GFP_KERNEL);
+       mds = cxl_dev_state_create(dev, serial, dvsec,
+                                  CXL_DEVTYPE_CLASSMEM,
+                                  struct cxl_memdev_state,
+                                  cxlds);
        if (!mds) {
                dev_err(dev, "No memory available\n");
                return ERR_PTR(-ENOMEM);
        }

+       rc = devm_add_action_or_reset(dev, cxl_memdev_state_destroy, mds);
+       if (rc)
+               return ERR_PTR(rc);
+
        mutex_init(&mds->event.log_lock);
-       mds->cxlds.dev = dev;
-       mds->cxlds.reg_map.host = dev;
-       mds->cxlds.cxl_mbox.host = dev;
-       mds->cxlds.reg_map.resource = CXL_RESOURCE_NONE;
-       mds->cxlds.type = CXL_DEVTYPE_CLASSMEM;


Looking at the above, it occurs to me that it may be good to either:

a) Have a devm version of cxl_dev_state_create() or,
b) Have an init macro (in addition) that takes a pointer so that you can use an
allocator of your choice

Neither are necessary, but would be more ergonomic IMO.

Thanks,
Ben