Message-ID: <58981468-ca67-0bb4-86b9-5cb2c3678737@amd.com>
Date: Wed, 8 Jan 2025 14:11:46 +0000
From: Alejandro Lucero Palau <alucerop@....com>
To: Dan Williams <dan.j.williams@...el.com>, alejandro.lucero-palau@....com,
 linux-cxl@...r.kernel.org, netdev@...r.kernel.org, martin.habets@...inx.com,
 edward.cree@....com, davem@...emloft.net, kuba@...nel.org,
 pabeni@...hat.com, edumazet@...gle.com, dave.jiang@...el.com
Subject: Re: [PATCH v8 01/27] cxl: add type2 device basic support


On 1/7/25 23:42, Dan Williams wrote:
> alejandro.lucero-palau@ wrote:
>> From: Alejandro Lucero <alucerop@....com>
>>
>> Differentiate CXL memory expanders (type 3) from CXL device accelerators
>> (type 2) with a new function for initializing cxl_dev_state.
>>
>> Create accessors to cxl_dev_state to be used by accel drivers.
>>
>> Based on previous work by Dan Williams [1]
>>
>> Link: [1] https://lore.kernel.org/linux-cxl/168592160379.1948938.12863272903570476312.stgit@dwillia2-xfh.jf.intel.com/
>> Signed-off-by: Alejandro Lucero <alucerop@....com>
>> Co-developed-by: Dan Williams <dan.j.williams@...el.com>
>> Reviewed-by: Dave Jiang <dave.jiang@...el.com>
>> Reviewed-by: Fan Ni <fan.ni@...sung.com>
> This patch causes
>> ---
>>   drivers/cxl/core/memdev.c | 51 +++++++++++++++++++++++++++++++++++++++
>>   drivers/cxl/core/pci.c    |  1 +
>>   drivers/cxl/cxlpci.h      | 16 ------------
>>   drivers/cxl/pci.c         | 13 +++++++---
>>   include/cxl/cxl.h         | 21 ++++++++++++++++
>>   include/cxl/pci.h         | 23 ++++++++++++++++++
>>   6 files changed, 105 insertions(+), 20 deletions(-)
>>   create mode 100644 include/cxl/cxl.h
>>   create mode 100644 include/cxl/pci.h
>>
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index ae3dfcbe8938..99f533caae1e 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -7,6 +7,7 @@
>>   #include <linux/slab.h>
>>   #include <linux/idr.h>
>>   #include <linux/pci.h>
>> +#include <cxl/cxl.h>
>>   #include <cxlmem.h>
>>   #include "trace.h"
>>   #include "core.h"
>> @@ -616,6 +617,25 @@ static void detach_memdev(struct work_struct *work)
>>   
>>   static struct lock_class_key cxl_memdev_key;
>>   
>> +struct cxl_dev_state *cxl_accel_state_create(struct device *dev)
> Lets just call this cxl_dev_state_create and have cxl_memdev_state use
> it internally for the truly common init functionality.
>
> Move the cxlds->type setting to a passed in parameter as that appears to
> be the only common init piece that needs to change to make this usable
> by cxl_memdev_state_create().
>
> That would also fix the missing initialization of these values the
> cxl_memdev_state_create() currently handles:
>
>          mds->cxlds.reg_map.resource = CXL_RESOURCE_NONE;
>          mds->ram_perf.qos_class = CXL_QOS_CLASS_INVALID;
>          mds->pmem_perf.qos_class = CXL_QOS_CLASS_INVALID;
>

Ok. It makes sense.


>> +{
>> +	struct cxl_dev_state *cxlds;
>> +
>> +	cxlds = kzalloc(sizeof(*cxlds), GFP_KERNEL);
>> +	if (!cxlds)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	cxlds->dev = dev;
>> +	cxlds->type = CXL_DEVTYPE_DEVMEM;
>> +
>> +	cxlds->dpa_res = DEFINE_RES_MEM_NAMED(0, 0, "dpa");
>> +	cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, 0, "ram");
>> +	cxlds->pmem_res = DEFINE_RES_MEM_NAMED(0, 0, "pmem");
>> +
>> +	return cxlds;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_accel_state_create, "CXL");
> So, this is the only new function I would expect in this patch based on
> the changelog...
>
>> +
>>   static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>>   					   const struct file_operations *fops)
>>   {
>> @@ -693,6 +713,37 @@ static int cxl_memdev_open(struct inode *inode, struct file *file)
>>   	return 0;
>>   }
>>   
>> +void cxl_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec)
>> +{
>> +	cxlds->cxl_dvsec = dvsec;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_set_dvsec, "CXL");
>> +
>> +void cxl_set_serial(struct cxl_dev_state *cxlds, u64 serial)
>> +{
>> +	cxlds->serial = serial;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_set_serial, "CXL");
> What are these doing in this patch? Why are new exports needed for such
> trivial functions? If they are common values to move to init time I would
> just make them common argument to cxl_dev_state_create().


I was told to merge those simple changes in this one instead of 
additional patches.

And I have no problem dropping them and use extra  args.

I'll do so it v10.


>> +
>> +int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res,
> Additionally, why does this take a 'struct resource' rather than a 'struct resource *'?


The driver does not need the resource but for this initialization, so it 
is a locally allocated resource which will not exist later on.

It is a small struct so I guess your concern is not with the stack, 
maybe about security. If it is due to some rule to avoid it which I'm 
not familiar with, it has gone undetected through a lot of eyes ...


>> +		     enum cxl_resource type)
>> +{
>> +	switch (type) {
>> +	case CXL_RES_DPA:
>> +		cxlds->dpa_res = res;
>> +		return 0;
>> +	case CXL_RES_RAM:
>> +		cxlds->ram_res = res;
>> +		return 0;
>> +	case CXL_RES_PMEM:
>> +		cxlds->pmem_res = res;
>> +		return 0;
> This appears to misunderstand the relationship between these resources.
> dpa_res is the overall device internal DPA address space resource tree.
> ram_res and pmem_res are shortcuts to get to the volatile and pmem
> partitions of the dpa space. I can imagine it would ever be desirable to
> trust the caller to fully initialize all the values of the resource,
> especially 'parent', 'sibling', and 'child' which should only be touched
> under the resource lock in the common case.


No, I'm aware of this, but also I think there is a need for setting them 
independently, and the reason behind this code.

Maybe you have in mind some complex devices requiring another approach 
for this set up.

Powered by blists - more mailing lists