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

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

On 3/3/25 2:51 PM, Dan Williams wrote:
> Ben Cheatham wrote:
>> 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>
> [..]
>> 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
> 
> The caller can always wrap the result with devm_add_action_or_reset().
> No need to force comsumer drivers into an allocation regime.
Sounds good to me, figured I would suggest it more as a convenience option
than anything.

Thanks,
Ben