[v5,13/27] cxl: prepare memdev creation for type2

Message ID	20241118164434.7551-14-alejandro.lucero-palau@amd.com
State	New
Headers	show Received: from NAM02-DM3-obe.outbound.protection.outlook.com (mail-dm3nam02on2085.outbound.protection.outlook.com [40.107.95.85]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 08BC01C2DCF; Mon, 18 Nov 2024 16:45:11 +0000 (UTC) Received-SPF: Pass (protection.outlook.com: domain of amd.com designates 165.204.84.12 as permitted sender) receiver=protection.outlook.com; client-ip=165.204.84.12; helo=SATLEXMB03.amd.com; pr=C From: <alejandro.lucero-palau@amd.com> To: <linux-cxl@vger.kernel.org>, <netdev@vger.kernel.org>, <dan.j.williams@intel.com>, <martin.habets@xilinx.com>, <edward.cree@amd.com>, <davem@davemloft.net>, <kuba@kernel.org>, <pabeni@redhat.com>, <edumazet@google.com> CC: Alejandro Lucero <alucerop@amd.com> Subject: [PATCH v5 13/27] cxl: prepare memdev creation for type2 Date: Mon, 18 Nov 2024 16:44:20 +0000 Message-ID: <20241118164434.7551-14-alejandro.lucero-palau@amd.com> In-Reply-To: <20241118164434.7551-1-alejandro.lucero-palau@amd.com> References: <20241118164434.7551-1-alejandro.lucero-palau@amd.com> Precedence: bulk MIME-Version: 1.0 Content-Type: text/plain Received-SPF: None (SATLEXMB05.amd.com: alejandro.lucero-palau@amd.com does not designate permitted sender hosts)
Series	cxl: add type2 device basic support \| expand [v5,00/27] cxl: add type2 device basic support [v5,01/27] cxl: add type2 device basic support [v5,02/27] sfc: add cxl support using new CXL API [v5,03/27] cxl: add capabilities field to cxl_dev_state and cxl_port [v5,04/27] cxl/pci: add check for validating capabilities [v5,05/27] cxl: move pci generic code [v5,06/27] cxl: add function for type2 cxl regs setup [v5,07/27] sfc: use cxl api for regs setup and checking [v5,08/27] cxl: add functions for resource request/release by a driver [v5,09/27] sfc: request cxl ram resource [v5,10/27] cxl: harden resource_contains checks to handle zero size resources [v5,11/27] cxl: add function for setting media ready by a driver [v5,12/27] sfc: set cxl media ready [v5,13/27] cxl: prepare memdev creation for type2 [v5,14/27] sfc: create type2 cxl memdev [v5,15/27] cxl: define a driver interface for HPA free space enumeration [v5,16/27] sfc: obtain root decoder with enough HPA free space [v5,17/27] cxl: define a driver interface for DPA allocation [v5,18/27] sfc: get endpoint decoder [v5,19/27] cxl: make region type based on endpoint type [v5,20/27] cxl/region: factor out interleave ways setup [v5,21/27] cxl/region: factor out interleave granularity setup [v5,22/27] cxl: allow region creation by type2 drivers [v5,23/27] sfc: create cxl region [v5,24/27] cxl: add region flag for precluding a device memory to be used for dax [v5,25/27] sfc: specify avoid dax when cxl region is created [v5,26/27] cxl: add function for obtaining params from a region [v5,27/27] sfc: support pio mapping based on cxl

Lucero Palau, Alejandro Nov. 18, 2024, 4:44 p.m. UTC

From: Alejandro Lucero <alucerop@amd.com>

Current cxl core is relying on a CXL_DEVTYPE_CLASSMEM type device when
creating a memdev leading to problems when obtaining cxl_memdev_state
references from a CXL_DEVTYPE_DEVMEM type. This last device type is
managed by a specific vendor driver and does not need same sysfs files
since not userspace intervention is expected.

Create a new cxl_mem device type with no attributes for Type2.

Avoid debugfs files relying on existence of clx_memdev_state.

Make devm_cxl_add_memdev accesible from a accel driver.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
---
 drivers/cxl/core/cdat.c   |  3 +++
 drivers/cxl/core/memdev.c | 15 +++++++++++++--
 drivers/cxl/core/region.c |  3 ++-
 drivers/cxl/mem.c         | 25 +++++++++++++++++++------
 include/cxl/cxl.h         |  2 ++
 5 files changed, 39 insertions(+), 9 deletions(-)

Dave Jiang Nov. 19, 2024, 6:24 p.m. UTC | #1

On 11/18/24 9:44 AM, alejandro.lucero-palau@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> Current cxl core is relying on a CXL_DEVTYPE_CLASSMEM type device when
> creating a memdev leading to problems when obtaining cxl_memdev_state
> references from a CXL_DEVTYPE_DEVMEM type. This last device type is
> managed by a specific vendor driver and does not need same sysfs files
> since not userspace intervention is expected.
> 
> Create a new cxl_mem device type with no attributes for Type2.
> 
> Avoid debugfs files relying on existence of clx_memdev_state.
> 
> Make devm_cxl_add_memdev accesible from a accel driver.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/core/cdat.c   |  3 +++
>  drivers/cxl/core/memdev.c | 15 +++++++++++++--
>  drivers/cxl/core/region.c |  3 ++-
>  drivers/cxl/mem.c         | 25 +++++++++++++++++++------
>  include/cxl/cxl.h         |  2 ++
>  5 files changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index e9cd7939c407..192cff18ea25 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -577,6 +577,9 @@ static struct cxl_dpa_perf *cxled_get_dpa_perf(struct cxl_endpoint_decoder *cxle
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
>  	struct cxl_dpa_perf *perf;
>  
> +	if (!mds)
> +		return ERR_PTR(-EINVAL);
> +
>  	switch (mode) {
>  	case CXL_DECODER_RAM:
>  		perf = &mds->ram_perf;
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index d746c8a1021c..df31eea0c06b 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -547,9 +547,17 @@ static const struct device_type cxl_memdev_type = {
>  	.groups = cxl_memdev_attribute_groups,
>  };
>  
> +static const struct device_type cxl_accel_memdev_type = {
> +	.name = "cxl_memdev",
> +	.release = cxl_memdev_release,
> +	.devnode = cxl_memdev_devnode,
> +};
> +
>  bool is_cxl_memdev(const struct device *dev)
>  {
> -	return dev->type == &cxl_memdev_type;
> +	return (dev->type == &cxl_memdev_type ||
> +		dev->type == &cxl_accel_memdev_type);
> +
>  }
>  EXPORT_SYMBOL_NS_GPL(is_cxl_memdev, CXL);

Does type2 device also exports a CDAT?

I'm also wondering if we should have distinctive helpers:
is_cxl_type3_memdev()
is_cxl_type2_memdev()

and is_cxl_memdev() is just calling those two helpers above. 

And if no CDAT is exported, we should change the is_cxl_memdev() to is_cxl_type3_memdev() in read_cdat_data(). 

DJ

>  
> @@ -660,7 +668,10 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>  	dev->parent = cxlds->dev;
>  	dev->bus = &cxl_bus_type;
>  	dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
> -	dev->type = &cxl_memdev_type;
> +	if (cxlds->type == CXL_DEVTYPE_DEVMEM)
> +		dev->type = &cxl_accel_memdev_type;
> +	else
> +		dev->type = &cxl_memdev_type;
>  	device_set_pm_not_required(dev);
>  	INIT_WORK(&cxlmd->detach_work, detach_memdev);
>  
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index dff618c708dc..622e3bb2e04b 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1948,7 +1948,8 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		return -EINVAL;
>  	}
>  
> -	cxl_region_perf_data_calculate(cxlr, cxled);
> +	if (cxlr->type == CXL_DECODER_HOSTONLYMEM)
> +		cxl_region_perf_data_calculate(cxlr, cxled);
>  
>  	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
>  		int i;
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index a9fd5cd5a0d2..cb771bf196cd 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -130,12 +130,18 @@ static int cxl_mem_probe(struct device *dev)
>  	dentry = cxl_debugfs_create_dir(dev_name(dev));
>  	debugfs_create_devm_seqfile(dev, "dpamem", dentry, cxl_mem_dpa_show);
>  
> -	if (test_bit(CXL_POISON_ENABLED_INJECT, mds->poison.enabled_cmds))
> -		debugfs_create_file("inject_poison", 0200, dentry, cxlmd,
> -				    &cxl_poison_inject_fops);
> -	if (test_bit(CXL_POISON_ENABLED_CLEAR, mds->poison.enabled_cmds))
> -		debugfs_create_file("clear_poison", 0200, dentry, cxlmd,
> -				    &cxl_poison_clear_fops);
> +	/*
> +	 * Avoid poison debugfs files for Type2 devices as they rely on
> +	 * cxl_memdev_state.
> +	 */
> +	if (mds) {
> +		if (test_bit(CXL_POISON_ENABLED_INJECT, mds->poison.enabled_cmds))
> +			debugfs_create_file("inject_poison", 0200, dentry, cxlmd,
> +					    &cxl_poison_inject_fops);
> +		if (test_bit(CXL_POISON_ENABLED_CLEAR, mds->poison.enabled_cmds))
> +			debugfs_create_file("clear_poison", 0200, dentry, cxlmd,
> +					    &cxl_poison_clear_fops);
> +	}
>  
>  	rc = devm_add_action_or_reset(dev, remove_debugfs, dentry);
>  	if (rc)
> @@ -219,6 +225,13 @@ static umode_t cxl_mem_visible(struct kobject *kobj, struct attribute *a, int n)
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
>  
> +	/*
> +	 * Avoid poison sysfs files for Type2 devices as they rely on
> +	 * cxl_memdev_state.
> +	 */
> +	if (!mds)
> +		return 0;
> +
>  	if (a == &dev_attr_trigger_poison_list.attr)
>  		if (!test_bit(CXL_POISON_ENABLED_LIST,
>  			      mds->poison.enabled_cmds))
> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
> index 6033ce84b3d3..5608ed0f5f15 100644
> --- a/include/cxl/cxl.h
> +++ b/include/cxl/cxl.h
> @@ -57,4 +57,6 @@ int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds);
>  int cxl_request_resource(struct cxl_dev_state *cxlds, enum cxl_resource type);
>  int cxl_release_resource(struct cxl_dev_state *cxlds, enum cxl_resource type);
>  void cxl_set_media_ready(struct cxl_dev_state *cxlds);
> +struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
> +				       struct cxl_dev_state *cxlds);
>  #endif

Zhi Wang Nov. 19, 2024, 8:06 p.m. UTC | #2

On Tue, 19 Nov 2024 11:24:44 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> 
> 
> On 11/18/24 9:44 AM, alejandro.lucero-palau@amd.com wrote:
> > From: Alejandro Lucero <alucerop@amd.com>
> > 
> > Current cxl core is relying on a CXL_DEVTYPE_CLASSMEM type device
> > when creating a memdev leading to problems when obtaining
> > cxl_memdev_state references from a CXL_DEVTYPE_DEVMEM type. This
> > last device type is managed by a specific vendor driver and does
> > not need same sysfs files since not userspace intervention is
> > expected.
> > 
> > Create a new cxl_mem device type with no attributes for Type2.
> > 
> > Avoid debugfs files relying on existence of clx_memdev_state.
> > 
> > Make devm_cxl_add_memdev accesible from a accel driver.
> > 
> > Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> > ---
> >  drivers/cxl/core/cdat.c   |  3 +++
> >  drivers/cxl/core/memdev.c | 15 +++++++++++++--
> >  drivers/cxl/core/region.c |  3 ++-
> >  drivers/cxl/mem.c         | 25 +++++++++++++++++++------
> >  include/cxl/cxl.h         |  2 ++
> >  5 files changed, 39 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> > index e9cd7939c407..192cff18ea25 100644
> > --- a/drivers/cxl/core/cdat.c
> > +++ b/drivers/cxl/core/cdat.c
> > @@ -577,6 +577,9 @@ static struct cxl_dpa_perf
> > *cxled_get_dpa_perf(struct cxl_endpoint_decoder *cxle struct
> > cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); struct
> > cxl_dpa_perf *perf; 
> > +	if (!mds)
> > +		return ERR_PTR(-EINVAL);
> > +
> >  	switch (mode) {
> >  	case CXL_DECODER_RAM:
> >  		perf = &mds->ram_perf;
> > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> > index d746c8a1021c..df31eea0c06b 100644
> > --- a/drivers/cxl/core/memdev.c
> > +++ b/drivers/cxl/core/memdev.c
> > @@ -547,9 +547,17 @@ static const struct device_type
> > cxl_memdev_type = { .groups = cxl_memdev_attribute_groups,
> >  };
> >  
> > +static const struct device_type cxl_accel_memdev_type = {
> > +	.name = "cxl_memdev",
> > +	.release = cxl_memdev_release,
> > +	.devnode = cxl_memdev_devnode,
> > +};
> > +
> >  bool is_cxl_memdev(const struct device *dev)
> >  {
> > -	return dev->type == &cxl_memdev_type;
> > +	return (dev->type == &cxl_memdev_type ||
> > +		dev->type == &cxl_accel_memdev_type);
> > +
> >  }
> >  EXPORT_SYMBOL_NS_GPL(is_cxl_memdev, CXL);
> 
> Does type2 device also exports a CDAT?
> 

Yes. Type2 can also export a CDAT.

> I'm also wondering if we should have distinctive helpers:
> is_cxl_type3_memdev()
> is_cxl_type2_memdev()
> 
> and is_cxl_memdev() is just calling those two helpers above. 
> 
> And if no CDAT is exported, we should change the is_cxl_memdev() to
> is_cxl_type3_memdev() in read_cdat_data(). 
> 
> DJ
> 
> >  
> > @@ -660,7 +668,10 @@ static struct cxl_memdev
> > *cxl_memdev_alloc(struct cxl_dev_state *cxlds, dev->parent =
> > cxlds->dev; dev->bus = &cxl_bus_type;
> >  	dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
> > -	dev->type = &cxl_memdev_type;
> > +	if (cxlds->type == CXL_DEVTYPE_DEVMEM)
> > +		dev->type = &cxl_accel_memdev_type;
> > +	else
> > +		dev->type = &cxl_memdev_type;
> >  	device_set_pm_not_required(dev);
> >  	INIT_WORK(&cxlmd->detach_work, detach_memdev);
> >  
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index dff618c708dc..622e3bb2e04b 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1948,7 +1948,8 @@ static int cxl_region_attach(struct
> > cxl_region *cxlr, return -EINVAL;
> >  	}
> >  
> > -	cxl_region_perf_data_calculate(cxlr, cxled);
> > +	if (cxlr->type == CXL_DECODER_HOSTONLYMEM)
> > +		cxl_region_perf_data_calculate(cxlr, cxled);
> >  
> >  	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> >  		int i;
> > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > index a9fd5cd5a0d2..cb771bf196cd 100644
> > --- a/drivers/cxl/mem.c
> > +++ b/drivers/cxl/mem.c
> > @@ -130,12 +130,18 @@ static int cxl_mem_probe(struct device *dev)
> >  	dentry = cxl_debugfs_create_dir(dev_name(dev));
> >  	debugfs_create_devm_seqfile(dev, "dpamem", dentry,
> > cxl_mem_dpa_show); 
> > -	if (test_bit(CXL_POISON_ENABLED_INJECT,
> > mds->poison.enabled_cmds))
> > -		debugfs_create_file("inject_poison", 0200, dentry,
> > cxlmd,
> > -				    &cxl_poison_inject_fops);
> > -	if (test_bit(CXL_POISON_ENABLED_CLEAR,
> > mds->poison.enabled_cmds))
> > -		debugfs_create_file("clear_poison", 0200, dentry,
> > cxlmd,
> > -				    &cxl_poison_clear_fops);
> > +	/*
> > +	 * Avoid poison debugfs files for Type2 devices as they
> > rely on
> > +	 * cxl_memdev_state.
> > +	 */
> > +	if (mds) {
> > +		if (test_bit(CXL_POISON_ENABLED_INJECT,
> > mds->poison.enabled_cmds))
> > +			debugfs_create_file("inject_poison", 0200,
> > dentry, cxlmd,
> > +
> > &cxl_poison_inject_fops);
> > +		if (test_bit(CXL_POISON_ENABLED_CLEAR,
> > mds->poison.enabled_cmds))
> > +			debugfs_create_file("clear_poison", 0200,
> > dentry, cxlmd,
> > +
> > &cxl_poison_clear_fops);
> > +	}
> >  
> >  	rc = devm_add_action_or_reset(dev, remove_debugfs, dentry);
> >  	if (rc)
> > @@ -219,6 +225,13 @@ static umode_t cxl_mem_visible(struct kobject
> > *kobj, struct attribute *a, int n) struct cxl_memdev *cxlmd =
> > to_cxl_memdev(dev); struct cxl_memdev_state *mds =
> > to_cxl_memdev_state(cxlmd->cxlds); 
> > +	/*
> > +	 * Avoid poison sysfs files for Type2 devices as they rely
> > on
> > +	 * cxl_memdev_state.
> > +	 */
> > +	if (!mds)
> > +		return 0;
> > +
> >  	if (a == &dev_attr_trigger_poison_list.attr)
> >  		if (!test_bit(CXL_POISON_ENABLED_LIST,
> >  			      mds->poison.enabled_cmds))
> > diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
> > index 6033ce84b3d3..5608ed0f5f15 100644
> > --- a/include/cxl/cxl.h
> > +++ b/include/cxl/cxl.h
> > @@ -57,4 +57,6 @@ int cxl_pci_accel_setup_regs(struct pci_dev
> > *pdev, struct cxl_dev_state *cxlds); int
> > cxl_request_resource(struct cxl_dev_state *cxlds, enum cxl_resource
> > type); int cxl_release_resource(struct cxl_dev_state *cxlds, enum
> > cxl_resource type); void cxl_set_media_ready(struct cxl_dev_state
> > *cxlds); +struct cxl_memdev *devm_cxl_add_memdev(struct device
> > *host,
> > +				       struct cxl_dev_state
> > *cxlds); #endif
> 
>

Dave Jiang Nov. 19, 2024, 9:27 p.m. UTC | #3

On 11/19/24 1:06 PM, Zhi Wang wrote:
> On Tue, 19 Nov 2024 11:24:44 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>>
>>
>> On 11/18/24 9:44 AM, alejandro.lucero-palau@amd.com wrote:
>>> From: Alejandro Lucero <alucerop@amd.com>
>>>
>>> Current cxl core is relying on a CXL_DEVTYPE_CLASSMEM type device
>>> when creating a memdev leading to problems when obtaining
>>> cxl_memdev_state references from a CXL_DEVTYPE_DEVMEM type. This
>>> last device type is managed by a specific vendor driver and does
>>> not need same sysfs files since not userspace intervention is
>>> expected.
>>>
>>> Create a new cxl_mem device type with no attributes for Type2.
>>>
>>> Avoid debugfs files relying on existence of clx_memdev_state.
>>>
>>> Make devm_cxl_add_memdev accesible from a accel driver.
>>>
>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>> ---
>>>  drivers/cxl/core/cdat.c   |  3 +++
>>>  drivers/cxl/core/memdev.c | 15 +++++++++++++--
>>>  drivers/cxl/core/region.c |  3 ++-
>>>  drivers/cxl/mem.c         | 25 +++++++++++++++++++------
>>>  include/cxl/cxl.h         |  2 ++
>>>  5 files changed, 39 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>>> index e9cd7939c407..192cff18ea25 100644
>>> --- a/drivers/cxl/core/cdat.c
>>> +++ b/drivers/cxl/core/cdat.c
>>> @@ -577,6 +577,9 @@ static struct cxl_dpa_perf
>>> *cxled_get_dpa_perf(struct cxl_endpoint_decoder *cxle struct
>>> cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); struct
>>> cxl_dpa_perf *perf; 
>>> +	if (!mds)
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>>  	switch (mode) {
>>>  	case CXL_DECODER_RAM:
>>>  		perf = &mds->ram_perf;
>>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>>> index d746c8a1021c..df31eea0c06b 100644
>>> --- a/drivers/cxl/core/memdev.c
>>> +++ b/drivers/cxl/core/memdev.c
>>> @@ -547,9 +547,17 @@ static const struct device_type
>>> cxl_memdev_type = { .groups = cxl_memdev_attribute_groups,
>>>  };
>>>  
>>> +static const struct device_type cxl_accel_memdev_type = {
>>> +	.name = "cxl_memdev",
>>> +	.release = cxl_memdev_release,
>>> +	.devnode = cxl_memdev_devnode,
>>> +};
>>> +
>>>  bool is_cxl_memdev(const struct device *dev)
>>>  {
>>> -	return dev->type == &cxl_memdev_type;
>>> +	return (dev->type == &cxl_memdev_type ||
>>> +		dev->type == &cxl_accel_memdev_type);
>>> +
>>>  }
>>>  EXPORT_SYMBOL_NS_GPL(is_cxl_memdev, CXL);
>>
>> Does type2 device also exports a CDAT?
>>
> 
> Yes. Type2 can also export a CDAT.

Thanks! Probably should have the split out helpers regardless.

> 
>> I'm also wondering if we should have distinctive helpers:
>> is_cxl_type3_memdev()
>> is_cxl_type2_memdev()
>>
>> and is_cxl_memdev() is just calling those two helpers above. 
>>
>> And if no CDAT is exported, we should change the is_cxl_memdev() to
>> is_cxl_type3_memdev() in read_cdat_data(). 
>>
>> DJ
>>
>>>  
>>> @@ -660,7 +668,10 @@ static struct cxl_memdev
>>> *cxl_memdev_alloc(struct cxl_dev_state *cxlds, dev->parent =
>>> cxlds->dev; dev->bus = &cxl_bus_type;
>>>  	dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
>>> -	dev->type = &cxl_memdev_type;
>>> +	if (cxlds->type == CXL_DEVTYPE_DEVMEM)
>>> +		dev->type = &cxl_accel_memdev_type;
>>> +	else
>>> +		dev->type = &cxl_memdev_type;
>>>  	device_set_pm_not_required(dev);
>>>  	INIT_WORK(&cxlmd->detach_work, detach_memdev);
>>>  
>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>> index dff618c708dc..622e3bb2e04b 100644
>>> --- a/drivers/cxl/core/region.c
>>> +++ b/drivers/cxl/core/region.c
>>> @@ -1948,7 +1948,8 @@ static int cxl_region_attach(struct
>>> cxl_region *cxlr, return -EINVAL;
>>>  	}
>>>  
>>> -	cxl_region_perf_data_calculate(cxlr, cxled);
>>> +	if (cxlr->type == CXL_DECODER_HOSTONLYMEM)
>>> +		cxl_region_perf_data_calculate(cxlr, cxled);
>>>  
>>>  	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
>>>  		int i;
>>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>>> index a9fd5cd5a0d2..cb771bf196cd 100644
>>> --- a/drivers/cxl/mem.c
>>> +++ b/drivers/cxl/mem.c
>>> @@ -130,12 +130,18 @@ static int cxl_mem_probe(struct device *dev)
>>>  	dentry = cxl_debugfs_create_dir(dev_name(dev));
>>>  	debugfs_create_devm_seqfile(dev, "dpamem", dentry,
>>> cxl_mem_dpa_show); 
>>> -	if (test_bit(CXL_POISON_ENABLED_INJECT,
>>> mds->poison.enabled_cmds))
>>> -		debugfs_create_file("inject_poison", 0200, dentry,
>>> cxlmd,
>>> -				    &cxl_poison_inject_fops);
>>> -	if (test_bit(CXL_POISON_ENABLED_CLEAR,
>>> mds->poison.enabled_cmds))
>>> -		debugfs_create_file("clear_poison", 0200, dentry,
>>> cxlmd,
>>> -				    &cxl_poison_clear_fops);
>>> +	/*
>>> +	 * Avoid poison debugfs files for Type2 devices as they
>>> rely on
>>> +	 * cxl_memdev_state.
>>> +	 */
>>> +	if (mds) {
>>> +		if (test_bit(CXL_POISON_ENABLED_INJECT,
>>> mds->poison.enabled_cmds))
>>> +			debugfs_create_file("inject_poison", 0200,
>>> dentry, cxlmd,
>>> +
>>> &cxl_poison_inject_fops);
>>> +		if (test_bit(CXL_POISON_ENABLED_CLEAR,
>>> mds->poison.enabled_cmds))
>>> +			debugfs_create_file("clear_poison", 0200,
>>> dentry, cxlmd,
>>> +
>>> &cxl_poison_clear_fops);
>>> +	}
>>>  
>>>  	rc = devm_add_action_or_reset(dev, remove_debugfs, dentry);
>>>  	if (rc)
>>> @@ -219,6 +225,13 @@ static umode_t cxl_mem_visible(struct kobject
>>> *kobj, struct attribute *a, int n) struct cxl_memdev *cxlmd =
>>> to_cxl_memdev(dev); struct cxl_memdev_state *mds =
>>> to_cxl_memdev_state(cxlmd->cxlds); 
>>> +	/*
>>> +	 * Avoid poison sysfs files for Type2 devices as they rely
>>> on
>>> +	 * cxl_memdev_state.
>>> +	 */
>>> +	if (!mds)
>>> +		return 0;
>>> +
>>>  	if (a == &dev_attr_trigger_poison_list.attr)
>>>  		if (!test_bit(CXL_POISON_ENABLED_LIST,
>>>  			      mds->poison.enabled_cmds))
>>> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
>>> index 6033ce84b3d3..5608ed0f5f15 100644
>>> --- a/include/cxl/cxl.h
>>> +++ b/include/cxl/cxl.h
>>> @@ -57,4 +57,6 @@ int cxl_pci_accel_setup_regs(struct pci_dev
>>> *pdev, struct cxl_dev_state *cxlds); int
>>> cxl_request_resource(struct cxl_dev_state *cxlds, enum cxl_resource
>>> type); int cxl_release_resource(struct cxl_dev_state *cxlds, enum
>>> cxl_resource type); void cxl_set_media_ready(struct cxl_dev_state
>>> *cxlds); +struct cxl_memdev *devm_cxl_add_memdev(struct device
>>> *host,
>>> +				       struct cxl_dev_state
>>> *cxlds); #endif
>>
>>
>

Alejandro Lucero Palau Nov. 20, 2024, 1:57 p.m. UTC | #4

On 11/19/24 21:27, Dave Jiang wrote:
>
> On 11/19/24 1:06 PM, Zhi Wang wrote:
>> On Tue, 19 Nov 2024 11:24:44 -0700
>> Dave Jiang <dave.jiang@intel.com> wrote:
>>
>>>
>>> On 11/18/24 9:44 AM, alejandro.lucero-palau@amd.com wrote:
>>>> From: Alejandro Lucero <alucerop@amd.com>
>>>>
>>>> Current cxl core is relying on a CXL_DEVTYPE_CLASSMEM type device
>>>> when creating a memdev leading to problems when obtaining
>>>> cxl_memdev_state references from a CXL_DEVTYPE_DEVMEM type. This
>>>> last device type is managed by a specific vendor driver and does
>>>> not need same sysfs files since not userspace intervention is
>>>> expected.
>>>>
>>>> Create a new cxl_mem device type with no attributes for Type2.
>>>>
>>>> Avoid debugfs files relying on existence of clx_memdev_state.
>>>>
>>>> Make devm_cxl_add_memdev accesible from a accel driver.
>>>>
>>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>>> ---
>>>>   drivers/cxl/core/cdat.c   |  3 +++
>>>>   drivers/cxl/core/memdev.c | 15 +++++++++++++--
>>>>   drivers/cxl/core/region.c |  3 ++-
>>>>   drivers/cxl/mem.c         | 25 +++++++++++++++++++------
>>>>   include/cxl/cxl.h         |  2 ++
>>>>   5 files changed, 39 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>>>> index e9cd7939c407..192cff18ea25 100644
>>>> --- a/drivers/cxl/core/cdat.c
>>>> +++ b/drivers/cxl/core/cdat.c
>>>> @@ -577,6 +577,9 @@ static struct cxl_dpa_perf
>>>> *cxled_get_dpa_perf(struct cxl_endpoint_decoder *cxle struct
>>>> cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); struct
>>>> cxl_dpa_perf *perf;
>>>> +	if (!mds)
>>>> +		return ERR_PTR(-EINVAL);
>>>> +
>>>>   	switch (mode) {
>>>>   	case CXL_DECODER_RAM:
>>>>   		perf = &mds->ram_perf;
>>>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>>>> index d746c8a1021c..df31eea0c06b 100644
>>>> --- a/drivers/cxl/core/memdev.c
>>>> +++ b/drivers/cxl/core/memdev.c
>>>> @@ -547,9 +547,17 @@ static const struct device_type
>>>> cxl_memdev_type = { .groups = cxl_memdev_attribute_groups,
>>>>   };
>>>>   
>>>> +static const struct device_type cxl_accel_memdev_type = {
>>>> +	.name = "cxl_memdev",
>>>> +	.release = cxl_memdev_release,
>>>> +	.devnode = cxl_memdev_devnode,
>>>> +};
>>>> +
>>>>   bool is_cxl_memdev(const struct device *dev)
>>>>   {
>>>> -	return dev->type == &cxl_memdev_type;
>>>> +	return (dev->type == &cxl_memdev_type ||
>>>> +		dev->type == &cxl_accel_memdev_type);
>>>> +
>>>>   }
>>>>   EXPORT_SYMBOL_NS_GPL(is_cxl_memdev, CXL);
>>> Does type2 device also exports a CDAT?
>>>
>> Yes. Type2 can also export a CDAT.
> Thanks! Probably should have the split out helpers regardless.


Maybe, but should not we wait until that is required? I did not see the 
need for adding them with this patchset.


>>> I'm also wondering if we should have distinctive helpers:
>>> is_cxl_type3_memdev()
>>> is_cxl_type2_memdev()
>>>
>>> and is_cxl_memdev() is just calling those two helpers above.
>>>
>>> And if no CDAT is exported, we should change the is_cxl_memdev() to
>>> is_cxl_type3_memdev() in read_cdat_data().
>>>
>>> DJ
>>>
>>>>   
>>>> @@ -660,7 +668,10 @@ static struct cxl_memdev
>>>> *cxl_memdev_alloc(struct cxl_dev_state *cxlds, dev->parent =
>>>> cxlds->dev; dev->bus = &cxl_bus_type;
>>>>   	dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
>>>> -	dev->type = &cxl_memdev_type;
>>>> +	if (cxlds->type == CXL_DEVTYPE_DEVMEM)
>>>> +		dev->type = &cxl_accel_memdev_type;
>>>> +	else
>>>> +		dev->type = &cxl_memdev_type;
>>>>   	device_set_pm_not_required(dev);
>>>>   	INIT_WORK(&cxlmd->detach_work, detach_memdev);
>>>>   
>>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>>> index dff618c708dc..622e3bb2e04b 100644
>>>> --- a/drivers/cxl/core/region.c
>>>> +++ b/drivers/cxl/core/region.c
>>>> @@ -1948,7 +1948,8 @@ static int cxl_region_attach(struct
>>>> cxl_region *cxlr, return -EINVAL;
>>>>   	}
>>>>   
>>>> -	cxl_region_perf_data_calculate(cxlr, cxled);
>>>> +	if (cxlr->type == CXL_DECODER_HOSTONLYMEM)
>>>> +		cxl_region_perf_data_calculate(cxlr, cxled);
>>>>   
>>>>   	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
>>>>   		int i;
>>>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>>>> index a9fd5cd5a0d2..cb771bf196cd 100644
>>>> --- a/drivers/cxl/mem.c
>>>> +++ b/drivers/cxl/mem.c
>>>> @@ -130,12 +130,18 @@ static int cxl_mem_probe(struct device *dev)
>>>>   	dentry = cxl_debugfs_create_dir(dev_name(dev));
>>>>   	debugfs_create_devm_seqfile(dev, "dpamem", dentry,
>>>> cxl_mem_dpa_show);
>>>> -	if (test_bit(CXL_POISON_ENABLED_INJECT,
>>>> mds->poison.enabled_cmds))
>>>> -		debugfs_create_file("inject_poison", 0200, dentry,
>>>> cxlmd,
>>>> -				    &cxl_poison_inject_fops);
>>>> -	if (test_bit(CXL_POISON_ENABLED_CLEAR,
>>>> mds->poison.enabled_cmds))
>>>> -		debugfs_create_file("clear_poison", 0200, dentry,
>>>> cxlmd,
>>>> -				    &cxl_poison_clear_fops);
>>>> +	/*
>>>> +	 * Avoid poison debugfs files for Type2 devices as they
>>>> rely on
>>>> +	 * cxl_memdev_state.
>>>> +	 */
>>>> +	if (mds) {
>>>> +		if (test_bit(CXL_POISON_ENABLED_INJECT,
>>>> mds->poison.enabled_cmds))
>>>> +			debugfs_create_file("inject_poison", 0200,
>>>> dentry, cxlmd,
>>>> +
>>>> &cxl_poison_inject_fops);
>>>> +		if (test_bit(CXL_POISON_ENABLED_CLEAR,
>>>> mds->poison.enabled_cmds))
>>>> +			debugfs_create_file("clear_poison", 0200,
>>>> dentry, cxlmd,
>>>> +
>>>> &cxl_poison_clear_fops);
>>>> +	}
>>>>   
>>>>   	rc = devm_add_action_or_reset(dev, remove_debugfs, dentry);
>>>>   	if (rc)
>>>> @@ -219,6 +225,13 @@ static umode_t cxl_mem_visible(struct kobject
>>>> *kobj, struct attribute *a, int n) struct cxl_memdev *cxlmd =
>>>> to_cxl_memdev(dev); struct cxl_memdev_state *mds =
>>>> to_cxl_memdev_state(cxlmd->cxlds);
>>>> +	/*
>>>> +	 * Avoid poison sysfs files for Type2 devices as they rely
>>>> on
>>>> +	 * cxl_memdev_state.
>>>> +	 */
>>>> +	if (!mds)
>>>> +		return 0;
>>>> +
>>>>   	if (a == &dev_attr_trigger_poison_list.attr)
>>>>   		if (!test_bit(CXL_POISON_ENABLED_LIST,
>>>>   			      mds->poison.enabled_cmds))
>>>> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
>>>> index 6033ce84b3d3..5608ed0f5f15 100644
>>>> --- a/include/cxl/cxl.h
>>>> +++ b/include/cxl/cxl.h
>>>> @@ -57,4 +57,6 @@ int cxl_pci_accel_setup_regs(struct pci_dev
>>>> *pdev, struct cxl_dev_state *cxlds); int
>>>> cxl_request_resource(struct cxl_dev_state *cxlds, enum cxl_resource
>>>> type); int cxl_release_resource(struct cxl_dev_state *cxlds, enum
>>>> cxl_resource type); void cxl_set_media_ready(struct cxl_dev_state
>>>> *cxlds); +struct cxl_memdev *devm_cxl_add_memdev(struct device
>>>> *host,
>>>> +				       struct cxl_dev_state
>>>> *cxlds); #endif
>>>

Dave Jiang Nov. 20, 2024, 5:15 p.m. UTC | #5

On 11/20/24 6:57 AM, Alejandro Lucero Palau wrote:
> 
> On 11/19/24 21:27, Dave Jiang wrote:
>>
>> On 11/19/24 1:06 PM, Zhi Wang wrote:
>>> On Tue, 19 Nov 2024 11:24:44 -0700
>>> Dave Jiang <dave.jiang@intel.com> wrote:
>>>
>>>>
>>>> On 11/18/24 9:44 AM, alejandro.lucero-palau@amd.com wrote:
>>>>> From: Alejandro Lucero <alucerop@amd.com>
>>>>>
>>>>> Current cxl core is relying on a CXL_DEVTYPE_CLASSMEM type device
>>>>> when creating a memdev leading to problems when obtaining
>>>>> cxl_memdev_state references from a CXL_DEVTYPE_DEVMEM type. This
>>>>> last device type is managed by a specific vendor driver and does
>>>>> not need same sysfs files since not userspace intervention is
>>>>> expected.
>>>>>
>>>>> Create a new cxl_mem device type with no attributes for Type2.
>>>>>
>>>>> Avoid debugfs files relying on existence of clx_memdev_state.
>>>>>
>>>>> Make devm_cxl_add_memdev accesible from a accel driver.
>>>>>
>>>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>>>> ---
>>>>>   drivers/cxl/core/cdat.c   |  3 +++
>>>>>   drivers/cxl/core/memdev.c | 15 +++++++++++++--
>>>>>   drivers/cxl/core/region.c |  3 ++-
>>>>>   drivers/cxl/mem.c         | 25 +++++++++++++++++++------
>>>>>   include/cxl/cxl.h         |  2 ++
>>>>>   5 files changed, 39 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>>>>> index e9cd7939c407..192cff18ea25 100644
>>>>> --- a/drivers/cxl/core/cdat.c
>>>>> +++ b/drivers/cxl/core/cdat.c
>>>>> @@ -577,6 +577,9 @@ static struct cxl_dpa_perf
>>>>> *cxled_get_dpa_perf(struct cxl_endpoint_decoder *cxle struct
>>>>> cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); struct
>>>>> cxl_dpa_perf *perf;
>>>>> +    if (!mds)
>>>>> +        return ERR_PTR(-EINVAL);
>>>>> +
>>>>>       switch (mode) {
>>>>>       case CXL_DECODER_RAM:
>>>>>           perf = &mds->ram_perf;
>>>>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>>>>> index d746c8a1021c..df31eea0c06b 100644
>>>>> --- a/drivers/cxl/core/memdev.c
>>>>> +++ b/drivers/cxl/core/memdev.c
>>>>> @@ -547,9 +547,17 @@ static const struct device_type
>>>>> cxl_memdev_type = { .groups = cxl_memdev_attribute_groups,
>>>>>   };
>>>>>   +static const struct device_type cxl_accel_memdev_type = {
>>>>> +    .name = "cxl_memdev",
>>>>> +    .release = cxl_memdev_release,
>>>>> +    .devnode = cxl_memdev_devnode,
>>>>> +};
>>>>> +
>>>>>   bool is_cxl_memdev(const struct device *dev)
>>>>>   {
>>>>> -    return dev->type == &cxl_memdev_type;
>>>>> +    return (dev->type == &cxl_memdev_type ||
>>>>> +        dev->type == &cxl_accel_memdev_type);
>>>>> +
>>>>>   }
>>>>>   EXPORT_SYMBOL_NS_GPL(is_cxl_memdev, CXL);
>>>> Does type2 device also exports a CDAT?
>>>>
>>> Yes. Type2 can also export a CDAT.
>> Thanks! Probably should have the split out helpers regardless.
> 
> 
> Maybe, but should not we wait until that is required? I did not see the need for adding them with this patchset.

Sure. I think my concern is with paths that apply only to one type but not the other. If you have not encountered any then we can wait.

DJ

> 
> 
>>>> I'm also wondering if we should have distinctive helpers:
>>>> is_cxl_type3_memdev()
>>>> is_cxl_type2_memdev()
>>>>
>>>> and is_cxl_memdev() is just calling those two helpers above.
>>>>
>>>> And if no CDAT is exported, we should change the is_cxl_memdev() to
>>>> is_cxl_type3_memdev() in read_cdat_data().
>>>>
>>>> DJ
>>>>
>>>>>   @@ -660,7 +668,10 @@ static struct cxl_memdev
>>>>> *cxl_memdev_alloc(struct cxl_dev_state *cxlds, dev->parent =
>>>>> cxlds->dev; dev->bus = &cxl_bus_type;
>>>>>       dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
>>>>> -    dev->type = &cxl_memdev_type;
>>>>> +    if (cxlds->type == CXL_DEVTYPE_DEVMEM)
>>>>> +        dev->type = &cxl_accel_memdev_type;
>>>>> +    else
>>>>> +        dev->type = &cxl_memdev_type;
>>>>>       device_set_pm_not_required(dev);
>>>>>       INIT_WORK(&cxlmd->detach_work, detach_memdev);
>>>>>   diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>>>> index dff618c708dc..622e3bb2e04b 100644
>>>>> --- a/drivers/cxl/core/region.c
>>>>> +++ b/drivers/cxl/core/region.c
>>>>> @@ -1948,7 +1948,8 @@ static int cxl_region_attach(struct
>>>>> cxl_region *cxlr, return -EINVAL;
>>>>>       }
>>>>>   -    cxl_region_perf_data_calculate(cxlr, cxled);
>>>>> +    if (cxlr->type == CXL_DECODER_HOSTONLYMEM)
>>>>> +        cxl_region_perf_data_calculate(cxlr, cxled);
>>>>>         if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
>>>>>           int i;
>>>>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>>>>> index a9fd5cd5a0d2..cb771bf196cd 100644
>>>>> --- a/drivers/cxl/mem.c
>>>>> +++ b/drivers/cxl/mem.c
>>>>> @@ -130,12 +130,18 @@ static int cxl_mem_probe(struct device *dev)
>>>>>       dentry = cxl_debugfs_create_dir(dev_name(dev));
>>>>>       debugfs_create_devm_seqfile(dev, "dpamem", dentry,
>>>>> cxl_mem_dpa_show);
>>>>> -    if (test_bit(CXL_POISON_ENABLED_INJECT,
>>>>> mds->poison.enabled_cmds))
>>>>> -        debugfs_create_file("inject_poison", 0200, dentry,
>>>>> cxlmd,
>>>>> -                    &cxl_poison_inject_fops);
>>>>> -    if (test_bit(CXL_POISON_ENABLED_CLEAR,
>>>>> mds->poison.enabled_cmds))
>>>>> -        debugfs_create_file("clear_poison", 0200, dentry,
>>>>> cxlmd,
>>>>> -                    &cxl_poison_clear_fops);
>>>>> +    /*
>>>>> +     * Avoid poison debugfs files for Type2 devices as they
>>>>> rely on
>>>>> +     * cxl_memdev_state.
>>>>> +     */
>>>>> +    if (mds) {
>>>>> +        if (test_bit(CXL_POISON_ENABLED_INJECT,
>>>>> mds->poison.enabled_cmds))
>>>>> +            debugfs_create_file("inject_poison", 0200,
>>>>> dentry, cxlmd,
>>>>> +
>>>>> &cxl_poison_inject_fops);
>>>>> +        if (test_bit(CXL_POISON_ENABLED_CLEAR,
>>>>> mds->poison.enabled_cmds))
>>>>> +            debugfs_create_file("clear_poison", 0200,
>>>>> dentry, cxlmd,
>>>>> +
>>>>> &cxl_poison_clear_fops);
>>>>> +    }
>>>>>         rc = devm_add_action_or_reset(dev, remove_debugfs, dentry);
>>>>>       if (rc)
>>>>> @@ -219,6 +225,13 @@ static umode_t cxl_mem_visible(struct kobject
>>>>> *kobj, struct attribute *a, int n) struct cxl_memdev *cxlmd =
>>>>> to_cxl_memdev(dev); struct cxl_memdev_state *mds =
>>>>> to_cxl_memdev_state(cxlmd->cxlds);
>>>>> +    /*
>>>>> +     * Avoid poison sysfs files for Type2 devices as they rely
>>>>> on
>>>>> +     * cxl_memdev_state.
>>>>> +     */
>>>>> +    if (!mds)
>>>>> +        return 0;
>>>>> +
>>>>>       if (a == &dev_attr_trigger_poison_list.attr)
>>>>>           if (!test_bit(CXL_POISON_ENABLED_LIST,
>>>>>                     mds->poison.enabled_cmds))
>>>>> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
>>>>> index 6033ce84b3d3..5608ed0f5f15 100644
>>>>> --- a/include/cxl/cxl.h
>>>>> +++ b/include/cxl/cxl.h
>>>>> @@ -57,4 +57,6 @@ int cxl_pci_accel_setup_regs(struct pci_dev
>>>>> *pdev, struct cxl_dev_state *cxlds); int
>>>>> cxl_request_resource(struct cxl_dev_state *cxlds, enum cxl_resource
>>>>> type); int cxl_release_resource(struct cxl_dev_state *cxlds, enum
>>>>> cxl_resource type); void cxl_set_media_ready(struct cxl_dev_state
>>>>> *cxlds); +struct cxl_memdev *devm_cxl_add_memdev(struct device
>>>>> *host,
>>>>> +                       struct cxl_dev_state
>>>>> *cxlds); #endif
>>>>

Zhi Wang Nov. 21, 2024, 7:43 a.m. UTC | #6

On Wed, 20 Nov 2024 10:15:59 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> 
> 
> On 11/20/24 6:57 AM, Alejandro Lucero Palau wrote:
> > 
> > On 11/19/24 21:27, Dave Jiang wrote:
> >>
> >> On 11/19/24 1:06 PM, Zhi Wang wrote:
> >>> On Tue, 19 Nov 2024 11:24:44 -0700
> >>> Dave Jiang <dave.jiang@intel.com> wrote:
> >>>
> >>>>
> >>>> On 11/18/24 9:44 AM, alejandro.lucero-palau@amd.com wrote:
> >>>>> From: Alejandro Lucero <alucerop@amd.com>
> >>>>>
> >>>>> Current cxl core is relying on a CXL_DEVTYPE_CLASSMEM type device
> >>>>> when creating a memdev leading to problems when obtaining
> >>>>> cxl_memdev_state references from a CXL_DEVTYPE_DEVMEM type. This
> >>>>> last device type is managed by a specific vendor driver and does
> >>>>> not need same sysfs files since not userspace intervention is
> >>>>> expected.
> >>>>>
> >>>>> Create a new cxl_mem device type with no attributes for Type2.
> >>>>>
> >>>>> Avoid debugfs files relying on existence of clx_memdev_state.
> >>>>>
> >>>>> Make devm_cxl_add_memdev accesible from a accel driver.
> >>>>>
> >>>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> >>>>> ---
> >>>>>   drivers/cxl/core/cdat.c   |  3 +++
> >>>>>   drivers/cxl/core/memdev.c | 15 +++++++++++++--
> >>>>>   drivers/cxl/core/region.c |  3 ++-
> >>>>>   drivers/cxl/mem.c         | 25 +++++++++++++++++++------
> >>>>>   include/cxl/cxl.h         |  2 ++
> >>>>>   5 files changed, 39 insertions(+), 9 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> >>>>> index e9cd7939c407..192cff18ea25 100644
> >>>>> --- a/drivers/cxl/core/cdat.c
> >>>>> +++ b/drivers/cxl/core/cdat.c
> >>>>> @@ -577,6 +577,9 @@ static struct cxl_dpa_perf
> >>>>> *cxled_get_dpa_perf(struct cxl_endpoint_decoder *cxle struct
> >>>>> cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); struct
> >>>>> cxl_dpa_perf *perf;
> >>>>> +    if (!mds)
> >>>>> +        return ERR_PTR(-EINVAL);
> >>>>> +
> >>>>>       switch (mode) {
> >>>>>       case CXL_DECODER_RAM:
> >>>>>           perf = &mds->ram_perf;
> >>>>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> >>>>> index d746c8a1021c..df31eea0c06b 100644
> >>>>> --- a/drivers/cxl/core/memdev.c
> >>>>> +++ b/drivers/cxl/core/memdev.c
> >>>>> @@ -547,9 +547,17 @@ static const struct device_type
> >>>>> cxl_memdev_type = { .groups = cxl_memdev_attribute_groups,
> >>>>>   };
> >>>>>   +static const struct device_type cxl_accel_memdev_type = {
> >>>>> +    .name = "cxl_memdev",
> >>>>> +    .release = cxl_memdev_release,
> >>>>> +    .devnode = cxl_memdev_devnode,
> >>>>> +};
> >>>>> +
> >>>>>   bool is_cxl_memdev(const struct device *dev)
> >>>>>   {
> >>>>> -    return dev->type == &cxl_memdev_type;
> >>>>> +    return (dev->type == &cxl_memdev_type ||
> >>>>> +        dev->type == &cxl_accel_memdev_type);
> >>>>> +
> >>>>>   }
> >>>>>   EXPORT_SYMBOL_NS_GPL(is_cxl_memdev, CXL);
> >>>> Does type2 device also exports a CDAT?
> >>>>
> >>> Yes. Type2 can also export a CDAT.
> >> Thanks! Probably should have the split out helpers regardless.
> > 
> > 
> > Maybe, but should not we wait until that is required? I did not see the need for adding them with this patchset.
> 
> Sure. I think my concern is with paths that apply only to one type but not the other. If you have not encountered any then we can wait.
> 

Agree.

I was thinking that for long-term, maybe CDAT routines shouldn't be tied to
device type, for me, it is like a cap that can be probed-and-used. E.g.
when talking with DOE, the core knows if CDAT is available or not, similar
case when the core tries to reach it via mailbox.

Z.
> DJ
> 
> > 
> > 
> >>>> I'm also wondering if we should have distinctive helpers:
> >>>> is_cxl_type3_memdev()
> >>>> is_cxl_type2_memdev()
> >>>>
> >>>> and is_cxl_memdev() is just calling those two helpers above.
> >>>>
> >>>> And if no CDAT is exported, we should change the is_cxl_memdev() to
> >>>> is_cxl_type3_memdev() in read_cdat_data().
> >>>>
> >>>> DJ
> >>>>
> >>>>>   @@ -660,7 +668,10 @@ static struct cxl_memdev
> >>>>> *cxl_memdev_alloc(struct cxl_dev_state *cxlds, dev->parent =
> >>>>> cxlds->dev; dev->bus = &cxl_bus_type;
> >>>>>       dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
> >>>>> -    dev->type = &cxl_memdev_type;
> >>>>> +    if (cxlds->type == CXL_DEVTYPE_DEVMEM)
> >>>>> +        dev->type = &cxl_accel_memdev_type;
> >>>>> +    else
> >>>>> +        dev->type = &cxl_memdev_type;
> >>>>>       device_set_pm_not_required(dev);
> >>>>>       INIT_WORK(&cxlmd->detach_work, detach_memdev);
> >>>>>   diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> >>>>> index dff618c708dc..622e3bb2e04b 100644
> >>>>> --- a/drivers/cxl/core/region.c
> >>>>> +++ b/drivers/cxl/core/region.c
> >>>>> @@ -1948,7 +1948,8 @@ static int cxl_region_attach(struct
> >>>>> cxl_region *cxlr, return -EINVAL;
> >>>>>       }
> >>>>>   -    cxl_region_perf_data_calculate(cxlr, cxled);
> >>>>> +    if (cxlr->type == CXL_DECODER_HOSTONLYMEM)
> >>>>> +        cxl_region_perf_data_calculate(cxlr, cxled);
> >>>>>         if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> >>>>>           int i;
> >>>>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> >>>>> index a9fd5cd5a0d2..cb771bf196cd 100644
> >>>>> --- a/drivers/cxl/mem.c
> >>>>> +++ b/drivers/cxl/mem.c
> >>>>> @@ -130,12 +130,18 @@ static int cxl_mem_probe(struct device *dev)
> >>>>>       dentry = cxl_debugfs_create_dir(dev_name(dev));
> >>>>>       debugfs_create_devm_seqfile(dev, "dpamem", dentry,
> >>>>> cxl_mem_dpa_show);
> >>>>> -    if (test_bit(CXL_POISON_ENABLED_INJECT,
> >>>>> mds->poison.enabled_cmds))
> >>>>> -        debugfs_create_file("inject_poison", 0200, dentry,
> >>>>> cxlmd,
> >>>>> -                    &cxl_poison_inject_fops);
> >>>>> -    if (test_bit(CXL_POISON_ENABLED_CLEAR,
> >>>>> mds->poison.enabled_cmds))
> >>>>> -        debugfs_create_file("clear_poison", 0200, dentry,
> >>>>> cxlmd,
> >>>>> -                    &cxl_poison_clear_fops);
> >>>>> +    /*
> >>>>> +     * Avoid poison debugfs files for Type2 devices as they
> >>>>> rely on
> >>>>> +     * cxl_memdev_state.
> >>>>> +     */
> >>>>> +    if (mds) {
> >>>>> +        if (test_bit(CXL_POISON_ENABLED_INJECT,
> >>>>> mds->poison.enabled_cmds))
> >>>>> +            debugfs_create_file("inject_poison", 0200,
> >>>>> dentry, cxlmd,
> >>>>> +
> >>>>> &cxl_poison_inject_fops);
> >>>>> +        if (test_bit(CXL_POISON_ENABLED_CLEAR,
> >>>>> mds->poison.enabled_cmds))
> >>>>> +            debugfs_create_file("clear_poison", 0200,
> >>>>> dentry, cxlmd,
> >>>>> +
> >>>>> &cxl_poison_clear_fops);
> >>>>> +    }
> >>>>>         rc = devm_add_action_or_reset(dev, remove_debugfs, dentry);
> >>>>>       if (rc)
> >>>>> @@ -219,6 +225,13 @@ static umode_t cxl_mem_visible(struct kobject
> >>>>> *kobj, struct attribute *a, int n) struct cxl_memdev *cxlmd =
> >>>>> to_cxl_memdev(dev); struct cxl_memdev_state *mds =
> >>>>> to_cxl_memdev_state(cxlmd->cxlds);
> >>>>> +    /*
> >>>>> +     * Avoid poison sysfs files for Type2 devices as they rely
> >>>>> on
> >>>>> +     * cxl_memdev_state.
> >>>>> +     */
> >>>>> +    if (!mds)
> >>>>> +        return 0;
> >>>>> +
> >>>>>       if (a == &dev_attr_trigger_poison_list.attr)
> >>>>>           if (!test_bit(CXL_POISON_ENABLED_LIST,
> >>>>>                     mds->poison.enabled_cmds))
> >>>>> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
> >>>>> index 6033ce84b3d3..5608ed0f5f15 100644
> >>>>> --- a/include/cxl/cxl.h
> >>>>> +++ b/include/cxl/cxl.h
> >>>>> @@ -57,4 +57,6 @@ int cxl_pci_accel_setup_regs(struct pci_dev
> >>>>> *pdev, struct cxl_dev_state *cxlds); int
> >>>>> cxl_request_resource(struct cxl_dev_state *cxlds, enum cxl_resource
> >>>>> type); int cxl_release_resource(struct cxl_dev_state *cxlds, enum
> >>>>> cxl_resource type); void cxl_set_media_ready(struct cxl_dev_state
> >>>>> *cxlds); +struct cxl_memdev *devm_cxl_add_memdev(struct device
> >>>>> *host,
> >>>>> +                       struct cxl_dev_state
> >>>>> *cxlds); #endif
> >>>>
> 
>

Ben Cheatham Nov. 22, 2024, 8:45 p.m. UTC | #7

On 11/18/24 10:44 AM, alejandro.lucero-palau@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> Current cxl core is relying on a CXL_DEVTYPE_CLASSMEM type device when
> creating a memdev leading to problems when obtaining cxl_memdev_state
> references from a CXL_DEVTYPE_DEVMEM type. This last device type is
> managed by a specific vendor driver and does not need same sysfs files
> since not userspace intervention is expected.
> 
> Create a new cxl_mem device type with no attributes for Type2.
> 
> Avoid debugfs files relying on existence of clx_memdev_state.
> 
> Make devm_cxl_add_memdev accesible from a accel driver.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/core/cdat.c   |  3 +++
>  drivers/cxl/core/memdev.c | 15 +++++++++++++--
>  drivers/cxl/core/region.c |  3 ++-
>  drivers/cxl/mem.c         | 25 +++++++++++++++++++------
>  include/cxl/cxl.h         |  2 ++
>  5 files changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index e9cd7939c407..192cff18ea25 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -577,6 +577,9 @@ static struct cxl_dpa_perf *cxled_get_dpa_perf(struct cxl_endpoint_decoder *cxle
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
>  	struct cxl_dpa_perf *perf;
>  
> +	if (!mds)
> +		return ERR_PTR(-EINVAL);
> +
>  	switch (mode) {
>  	case CXL_DECODER_RAM:
>  		perf = &mds->ram_perf;
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index d746c8a1021c..df31eea0c06b 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -547,9 +547,17 @@ static const struct device_type cxl_memdev_type = {
>  	.groups = cxl_memdev_attribute_groups,
>  };
>  
> +static const struct device_type cxl_accel_memdev_type = {
> +	.name = "cxl_memdev",

I would like to see a different name than cxl_memdev here, since this is technically
a different type and I could see it being confusing sysfs-wise. Maybe "cxl_acceldev"
or "cxl_accel_memdev" instead?

> +	.release = cxl_memdev_release,
> +	.devnode = cxl_memdev_devnode,
> +};
> +
>  bool is_cxl_memdev(const struct device *dev)
>  {
> -	return dev->type == &cxl_memdev_type;
> +	return (dev->type == &cxl_memdev_type ||
> +		dev->type == &cxl_accel_memdev_type);
> +
>  }
>  EXPORT_SYMBOL_NS_GPL(is_cxl_memdev, CXL);
>  
> @@ -660,7 +668,10 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>  	dev->parent = cxlds->dev;
>  	dev->bus = &cxl_bus_type;
>  	dev->devt = MKDEV(cxl_mem_major, cxlmd->id);
> -	dev->type = &cxl_memdev_type;
> +	if (cxlds->type == CXL_DEVTYPE_DEVMEM)
> +		dev->type = &cxl_accel_memdev_type;
> +	else
> +		dev->type = &cxl_memdev_type;
>  	device_set_pm_not_required(dev);
>  	INIT_WORK(&cxlmd->detach_work, detach_memdev);
>  
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index dff618c708dc..622e3bb2e04b 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1948,7 +1948,8 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		return -EINVAL;
>  	}
>  
> -	cxl_region_perf_data_calculate(cxlr, cxled);
> +	if (cxlr->type == CXL_DECODER_HOSTONLYMEM)
> +		cxl_region_perf_data_calculate(cxlr, cxled);
>  
>  	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
>  		int i;
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index a9fd5cd5a0d2..cb771bf196cd 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -130,12 +130,18 @@ static int cxl_mem_probe(struct device *dev)
>  	dentry = cxl_debugfs_create_dir(dev_name(dev));
>  	debugfs_create_devm_seqfile(dev, "dpamem", dentry, cxl_mem_dpa_show);
>  
> -	if (test_bit(CXL_POISON_ENABLED_INJECT, mds->poison.enabled_cmds))
> -		debugfs_create_file("inject_poison", 0200, dentry, cxlmd,
> -				    &cxl_poison_inject_fops);
> -	if (test_bit(CXL_POISON_ENABLED_CLEAR, mds->poison.enabled_cmds))
> -		debugfs_create_file("clear_poison", 0200, dentry, cxlmd,
> -				    &cxl_poison_clear_fops);
> +	/*
> +	 * Avoid poison debugfs files for Type2 devices as they rely on
> +	 * cxl_memdev_state.
> +	 */
> +	if (mds) {
> +		if (test_bit(CXL_POISON_ENABLED_INJECT, mds->poison.enabled_cmds))
> +			debugfs_create_file("inject_poison", 0200, dentry, cxlmd,
> +					    &cxl_poison_inject_fops);
> +		if (test_bit(CXL_POISON_ENABLED_CLEAR, mds->poison.enabled_cmds))
> +			debugfs_create_file("clear_poison", 0200, dentry, cxlmd,
> +					    &cxl_poison_clear_fops);
> +	}
>  
>  	rc = devm_add_action_or_reset(dev, remove_debugfs, dentry);
>  	if (rc)
> @@ -219,6 +225,13 @@ static umode_t cxl_mem_visible(struct kobject *kobj, struct attribute *a, int n)
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
>  
> +	/*
> +	 * Avoid poison sysfs files for Type2 devices as they rely on
> +	 * cxl_memdev_state.
> +	 */
> +	if (!mds)
> +		return 0;

cxl_accel_memdev don't use the same attributes, so I imagine this modification isn't needed?
I'm probably just missing something here.

> +
>  	if (a == &dev_attr_trigger_poison_list.attr)
>  		if (!test_bit(CXL_POISON_ENABLED_LIST,
>  			      mds->poison.enabled_cmds))
> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
> index 6033ce84b3d3..5608ed0f5f15 100644
> --- a/include/cxl/cxl.h
> +++ b/include/cxl/cxl.h
> @@ -57,4 +57,6 @@ int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds);
>  int cxl_request_resource(struct cxl_dev_state *cxlds, enum cxl_resource type);
>  int cxl_release_resource(struct cxl_dev_state *cxlds, enum cxl_resource type);
>  void cxl_set_media_ready(struct cxl_dev_state *cxlds);
> +struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
> +				       struct cxl_dev_state *cxlds);
>  #endif

[v5,13/27] cxl: prepare memdev creation for type2

Commit Message

Comments

Patch