Re: [PATCH v1 1/3] cxl: support Type2 when initializing cxl_dev_state

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

On 2/24/26 02:02, Alison Schofield wrote:
> On Mon, Feb 23, 2026 at 02:26:31PM +0000, alejandro.lucero-palau@amd.com wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> In preparation for type2 drivers add function and macro for
>> differentiating CXL memory expanders (type 3) from CXL device
>> accelerators (type 2) helping accel drivers to embed cxl_dev_state
>> inside a private struct.
>>
>> Update type3 driver for using this same initialization.
> I've got 2 simple nits that could be fixed-up upon applying,
> and one question below.
>
> Reviewed-by: Alison Schofield <alison.schofield@intel.com>


Thanks Allison.


The nit one will be fixed.


About the second one, I think it is not a problem as commented below.


> snip
>
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index f547d8ac34c7..ad131f053bf5 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -656,6 +656,30 @@ static void detach_memdev(struct work_struct *work)
>>   
>>   static struct lock_class_key cxl_memdev_key;
>>   
>> +struct cxl_dev_state *_devm_cxl_dev_state_create(struct device *dev,
>> +						 enum cxl_devtype type,
>> +						 u64 serial, u16 dvsec,
>> +						 size_t size, bool has_mbox)
>> +{
>> +	struct cxl_dev_state *cxlds = devm_kzalloc(dev, size, GFP_KERNEL);
>> +
>> +	if (!cxlds)
>> +		return NULL;
>> +
>> +	cxlds->dev = dev;
>> +	cxlds->type = type;
>> +	cxlds->serial = serial;
>> +	cxlds->cxl_dvsec =dvsec;
> Nit one: space around =


Ups. I'll fix it.


>
>> +	cxlds->reg_map.host = dev;
>> +	cxlds->reg_map.resource = CXL_RESOURCE_NONE;
>> +
>> +	if (has_mbox)
>> +		cxlds->cxl_mbox.host = dev;
>> +
>> +	return cxlds;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(_devm_cxl_dev_state_create, "CXL");
> Why do we name this with leading underscore when we intend to EXPORT it?
> Thinking leading underscores are meant for internal helpers?


Because the idea is to use the macro and not this function directly.


>
>> +
>>   static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>>   					   const struct file_operations *fops,
>>   					   const struct cxl_memdev_attach *attach)
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>> index e21d744d639b..9daf4bc42692 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -523,6 +523,37 @@ to_cxl_memdev_state(struct cxl_dev_state *cxlds)
>>   	return container_of(cxlds, struct cxl_memdev_state, cxlds);
>>   }
>>   
>> +struct cxl_dev_state *_devm_cxl_dev_state_create(struct device *dev,
>> +						 enum cxl_devtype type,
>> +						 u64 serial, u16 dvsec,
>> +						 size_t size, bool has_mbox);
>> +
>> +/**
>> + * cxl_dev_state_create - safely create and cast a cxl dev state embedded in a
>> + * driver specific struct.
> Nit two: function name is actually devm_cxl_dev_state_create


The macro but once translated by the compiler the function name with the 
underscore is used.


>
>> + *
>> + * @parent: device behind the request
>> + * @type: CXL device type
>> + * @serial: device identification
>> + * @dvsec: dvsec capability offset
>> + * @drv_struct: driver struct embedding a cxl_dev_state struct
>> + * @member: drv_struct member as cxl_dev_state
>> + * @mbox: true if mailbox supported
>> + *
>> + * Returns a pointer to the drv_struct allocated and embedding a cxl_dev_state
>> + * struct initialized.
>> + *
>> + * Introduced for Type2 driver support.
>> + */
>> +#define devm_cxl_dev_state_create(parent, type, serial, dvsec, drv_struct, member, mbox)	\
>> +	({										\
>> +		static_assert(__same_type(struct cxl_dev_state,				\
>> +			      ((drv_struct *)NULL)->member));				\
>> +		static_assert(offsetof(drv_struct, member) == 0);			\
>> +		(drv_struct *)_devm_cxl_dev_state_create(parent, type, serial, dvsec,	\
>> +						      sizeof(drv_struct), mbox);	\
>> +	})
>> +
> snip