Re: [PATCH v11 01/23] cxl: add type2 device basic support

[Alejandro Lucero Palau <alucerop@amd.com>]

On 3/11/25 20:05, Ben Cheatham wrote:
> On 3/10/25 4:03 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.
>>
>> Use same new initialization with the type3 pci driver.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/cxl/core/mbox.c   |  12 +--
>>   drivers/cxl/core/memdev.c |  32 ++++++
>>   drivers/cxl/core/pci.c    |   1 +
>>   drivers/cxl/core/regs.c   |   1 +
>>   drivers/cxl/cxl.h         |  97 +-----------------
>>   drivers/cxl/cxlmem.h      |  88 ++--------------
>>   drivers/cxl/cxlpci.h      |  21 ----
>>   drivers/cxl/pci.c         |  17 ++--
>>   include/cxl/cxl.h         | 206 ++++++++++++++++++++++++++++++++++++++
>>   include/cxl/pci.h         |  23 +++++
>>   10 files changed, 285 insertions(+), 213 deletions(-)
>>   create mode 100644 include/cxl/cxl.h
>>   create mode 100644 include/cxl/pci.h
>>
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
>> index d72764056ce6..20df6f78f148 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -1484,23 +1484,21 @@ int cxl_mailbox_init(struct cxl_mailbox *cxl_mbox, struct device *host)
>>   }
>>   EXPORT_SYMBOL_NS_GPL(cxl_mailbox_init, "CXL");
>>   
>> -struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev)
>> +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 = (struct cxl_memdev_state *)
>> +		_cxl_dev_state_create(dev, CXL_DEVTYPE_CLASSMEM, serial, dvsec,
>> +				      sizeof(struct cxl_memdev_state), true);
> I would use sizeof(*mds) instead of sizeof(struct cxl_memdev_state) above.


Yes.


>
> What's the reason to not use the cxl_dev_state_create() macro here instead? Based on the commit
> message I'm assuming it's because it's meant for accelerator drivers and this is a type 3 driver,
> but I'm going to suggest using it here anyway (and maybe amending the commit message).


The reason is the macro does checks we know for sure are not necessary 
for the cxl core pci driver for Type3.

But I have no problem using it here as well. If there are more opinions 
like yours, I'll do.


>>   	if (!mds) {
>>   		dev_err(dev, "No memory available\n");
>>   		return ERR_PTR(-ENOMEM);
>>   	}
>>   
>>   	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;
>>   
> [snip]
>
>> +/**
>> + * 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
>> + * @cxlfs: CXL features 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;
>> +#ifdef CONFIG_CXL_FEATURES
>> +	struct cxl_features_state *cxlfs;
>> +#endif
>> +};
> What happened to the comments for private/public fields for this struct that Dan suggested in
> your RFC? If you don't think they're needed that's fine, I'm just curious!


I just forgot to add them. :-(

I'll do so in v12 ...

Thanks!