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

Message ID	20241118164434.7551-14-alejandro.lucero-palau@amd.com (mailing list archive)
State	Not Applicable
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

Context	Check	Description
netdev/series_format	fail	Series longer than 15 patches
netdev/tree_selection	success	Guessed tree name to be net-next
netdev/ynl	success	Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present	success	Fixes tag not required for -next series
netdev/header_inline	success	No static functions without inline keyword in header files
netdev/build_32bit	success	Errors and warnings before: 5 this patch: 5
netdev/build_tools	success	Errors and warnings before: 0 (+0) this patch: 0 (+0)
netdev/cc_maintainers	warning	6 maintainers not CCed: alison.schofield@intel.com dave.jiang@intel.com dave@stgolabs.net jonathan.cameron@huawei.com ira.weiny@intel.com vishal.l.verma@intel.com
netdev/build_clang	success	Errors and warnings before: 5 this patch: 5
netdev/verify_signedoff	success	Signed-off-by tag matches author and committer
netdev/deprecated_api	success	None detected
netdev/check_selftest	success	No net selftest shell script
netdev/verify_fixes	success	No Fixes tag
netdev/build_allmodconfig_warn	success	Errors and warnings before: 6 this patch: 6
netdev/checkpatch	warning	WARNING: line length of 81 exceeds 80 columns WARNING: line length of 82 exceeds 80 columns
netdev/build_clang_rust	success	No Rust files in patch. Skipping build
netdev/kdoc	success	Errors and warnings before: 1 this patch: 1
netdev/source_inline	success	Was 0 now: 0

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

Alejandro Lucero Palau Nov. 27, 2024, 4:09 p.m. UTC | #8

On 11/22/24 20:45, Ben Cheatham wrote:
> 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?


Yes, it makes sense.


>> +	.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.


This function is invoked for a Type2, as the cxl_mem device is created 
and the attr group attached by default.

So this is needed or the reference will be pointing to unknown data and 
the kernel, if we are lucky, getting a null pointer or a wrong pointer 
making things worse.


>> +
>>   	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

Checks

Commit Message

Comments

Patch