| Message ID | 20240907081836.5801-10-alejandro.lucero-palau@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: add Type2 device support | expand |
On 9/7/24 1:18 AM, alejandro.lucero-palau@amd.com wrote: > From: Alejandro Lucero <alucerop@amd.com> > > Add memdev creation from sfc driver. > > 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. This patch checks for the > right device type in those functions using cxl_memdev_state. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > --- > drivers/cxl/core/cdat.c | 3 +++ > drivers/cxl/core/memdev.c | 9 +++++++++ > drivers/cxl/mem.c | 17 +++++++++++------ > drivers/net/ethernet/sfc/efx_cxl.c | 7 +++++++ > include/linux/cxl/cxl.h | 2 ++ > 5 files changed, 32 insertions(+), 6 deletions(-) > > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c > index bb83867d9fec..0d4679c137d4 100644 > --- a/drivers/cxl/core/cdat.c > +++ b/drivers/cxl/core/cdat.c > @@ -558,6 +558,9 @@ void cxl_region_perf_data_calculate(struct cxl_region *cxlr, > }; > struct cxl_dpa_perf *perf; > > + if (!mds) > + return; > + > switch (cxlr->mode) { > case CXL_DECODER_RAM: > perf = &mds->ram_perf; > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c > index 836faf09b328..5f8418620b70 100644 > --- a/drivers/cxl/core/memdev.c > +++ b/drivers/cxl/core/memdev.c > @@ -468,6 +468,9 @@ static umode_t cxl_ram_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); > > + if (!mds) > + return 0; I think instead of altering the sysfs visible attributes, what you really want is to not register the unwanted group attributes. Or basically only register the cxl_memdev_attribute_group and not the other ones. Otherwise it gets really messy. And whomever adds future attributes need to also remember the special case. I think you can refer to core/port.c and look at the different decoder types as inspiration for creating different memdev types where it has cxl_decoder_base_attribute_group and then tack on specific attribute groups. DJ > + > if (a == &dev_attr_ram_qos_class.attr) > if (mds->ram_perf.qos_class == CXL_QOS_CLASS_INVALID) > return 0; > @@ -487,6 +490,9 @@ static umode_t cxl_pmem_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); > > + if (!mds) > + return 0; > + > if (a == &dev_attr_pmem_qos_class.attr) > if (mds->pmem_perf.qos_class == CXL_QOS_CLASS_INVALID) > return 0; > @@ -507,6 +513,9 @@ static umode_t cxl_memdev_security_visible(struct kobject *kobj, > struct cxl_memdev *cxlmd = to_cxl_memdev(dev); > struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); > > + if (!mds) > + return 0; > + > if (a == &dev_attr_security_sanitize.attr && > !test_bit(CXL_SEC_ENABLED_SANITIZE, mds->security.enabled_cmds)) > return 0; > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index 7de232eaeb17..5c7ad230bccb 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -131,12 +131,14 @@ 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); > + 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) > @@ -222,6 +224,9 @@ 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); > > + 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/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c > index 14fab41fe10a..899bc823a212 100644 > --- a/drivers/net/ethernet/sfc/efx_cxl.c > +++ b/drivers/net/ethernet/sfc/efx_cxl.c > @@ -83,6 +83,13 @@ int efx_cxl_init(struct efx_nic *efx) > */ > cxl_set_media_ready(cxl->cxlds); > > + cxl->cxlmd = devm_cxl_add_memdev(&pci_dev->dev, cxl->cxlds); > + if (IS_ERR(cxl->cxlmd)) { > + pci_err(pci_dev, "CXL accel memdev creation failed"); > + rc = PTR_ERR(cxl->cxlmd); > + goto err; > + } > + > return 0; > err: > kfree(cxl->cxlds); > diff --git a/include/linux/cxl/cxl.h b/include/linux/cxl/cxl.h > index 08723b2d75bc..fc0859f841dc 100644 > --- a/include/linux/cxl/cxl.h > +++ b/include/linux/cxl/cxl.h > @@ -55,4 +55,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
On 9/12/24 19:19, Dave Jiang wrote: > > On 9/7/24 1:18 AM, alejandro.lucero-palau@amd.com wrote: >> From: Alejandro Lucero <alucerop@amd.com> >> >> Add memdev creation from sfc driver. >> >> 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. This patch checks for the >> right device type in those functions using cxl_memdev_state. >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> --- >> drivers/cxl/core/cdat.c | 3 +++ >> drivers/cxl/core/memdev.c | 9 +++++++++ >> drivers/cxl/mem.c | 17 +++++++++++------ >> drivers/net/ethernet/sfc/efx_cxl.c | 7 +++++++ >> include/linux/cxl/cxl.h | 2 ++ >> 5 files changed, 32 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c >> index bb83867d9fec..0d4679c137d4 100644 >> --- a/drivers/cxl/core/cdat.c >> +++ b/drivers/cxl/core/cdat.c >> @@ -558,6 +558,9 @@ void cxl_region_perf_data_calculate(struct cxl_region *cxlr, >> }; >> struct cxl_dpa_perf *perf; >> >> + if (!mds) >> + return; >> + >> switch (cxlr->mode) { >> case CXL_DECODER_RAM: >> perf = &mds->ram_perf; >> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c >> index 836faf09b328..5f8418620b70 100644 >> --- a/drivers/cxl/core/memdev.c >> +++ b/drivers/cxl/core/memdev.c >> @@ -468,6 +468,9 @@ static umode_t cxl_ram_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); >> >> + if (!mds) >> + return 0; > I think instead of altering the sysfs visible attributes, what you really want is to not register the unwanted group attributes. Or basically only register the cxl_memdev_attribute_group and not the other ones. Otherwise it gets really messy. And whomever adds future attributes need to also remember the special case. I think you can refer to core/port.c and look at the different decoder types as inspiration for creating different memdev types where it has cxl_decoder_base_attribute_group and then tack on specific attribute groups. > > DJ I think you are right. It was a quick fix for having things working and not thought about it in detail. I'll do and fix it for v4. Thanks! >> + >> if (a == &dev_attr_ram_qos_class.attr) >> if (mds->ram_perf.qos_class == CXL_QOS_CLASS_INVALID) >> return 0; >> @@ -487,6 +490,9 @@ static umode_t cxl_pmem_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); >> >> + if (!mds) >> + return 0; >> + >> if (a == &dev_attr_pmem_qos_class.attr) >> if (mds->pmem_perf.qos_class == CXL_QOS_CLASS_INVALID) >> return 0; >> @@ -507,6 +513,9 @@ static umode_t cxl_memdev_security_visible(struct kobject *kobj, >> struct cxl_memdev *cxlmd = to_cxl_memdev(dev); >> struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); >> >> + if (!mds) >> + return 0; >> + >> if (a == &dev_attr_security_sanitize.attr && >> !test_bit(CXL_SEC_ENABLED_SANITIZE, mds->security.enabled_cmds)) >> return 0; >> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c >> index 7de232eaeb17..5c7ad230bccb 100644 >> --- a/drivers/cxl/mem.c >> +++ b/drivers/cxl/mem.c >> @@ -131,12 +131,14 @@ 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); >> + 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) >> @@ -222,6 +224,9 @@ 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); >> >> + 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/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c >> index 14fab41fe10a..899bc823a212 100644 >> --- a/drivers/net/ethernet/sfc/efx_cxl.c >> +++ b/drivers/net/ethernet/sfc/efx_cxl.c >> @@ -83,6 +83,13 @@ int efx_cxl_init(struct efx_nic *efx) >> */ >> cxl_set_media_ready(cxl->cxlds); >> >> + cxl->cxlmd = devm_cxl_add_memdev(&pci_dev->dev, cxl->cxlds); >> + if (IS_ERR(cxl->cxlmd)) { >> + pci_err(pci_dev, "CXL accel memdev creation failed"); >> + rc = PTR_ERR(cxl->cxlmd); >> + goto err; >> + } >> + >> return 0; >> err: >> kfree(cxl->cxlds); >> diff --git a/include/linux/cxl/cxl.h b/include/linux/cxl/cxl.h >> index 08723b2d75bc..fc0859f841dc 100644 >> --- a/include/linux/cxl/cxl.h >> +++ b/include/linux/cxl/cxl.h >> @@ -55,4 +55,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
diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c index bb83867d9fec..0d4679c137d4 100644 --- a/drivers/cxl/core/cdat.c +++ b/drivers/cxl/core/cdat.c @@ -558,6 +558,9 @@ void cxl_region_perf_data_calculate(struct cxl_region *cxlr, }; struct cxl_dpa_perf *perf; + if (!mds) + return; + switch (cxlr->mode) { case CXL_DECODER_RAM: perf = &mds->ram_perf; diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c index 836faf09b328..5f8418620b70 100644 --- a/drivers/cxl/core/memdev.c +++ b/drivers/cxl/core/memdev.c @@ -468,6 +468,9 @@ static umode_t cxl_ram_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); + if (!mds) + return 0; + if (a == &dev_attr_ram_qos_class.attr) if (mds->ram_perf.qos_class == CXL_QOS_CLASS_INVALID) return 0; @@ -487,6 +490,9 @@ static umode_t cxl_pmem_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); + if (!mds) + return 0; + if (a == &dev_attr_pmem_qos_class.attr) if (mds->pmem_perf.qos_class == CXL_QOS_CLASS_INVALID) return 0; @@ -507,6 +513,9 @@ static umode_t cxl_memdev_security_visible(struct kobject *kobj, struct cxl_memdev *cxlmd = to_cxl_memdev(dev); struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds); + if (!mds) + return 0; + if (a == &dev_attr_security_sanitize.attr && !test_bit(CXL_SEC_ENABLED_SANITIZE, mds->security.enabled_cmds)) return 0; diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c index 7de232eaeb17..5c7ad230bccb 100644 --- a/drivers/cxl/mem.c +++ b/drivers/cxl/mem.c @@ -131,12 +131,14 @@ 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); + 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) @@ -222,6 +224,9 @@ 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); + 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/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c index 14fab41fe10a..899bc823a212 100644 --- a/drivers/net/ethernet/sfc/efx_cxl.c +++ b/drivers/net/ethernet/sfc/efx_cxl.c @@ -83,6 +83,13 @@ int efx_cxl_init(struct efx_nic *efx) */ cxl_set_media_ready(cxl->cxlds); + cxl->cxlmd = devm_cxl_add_memdev(&pci_dev->dev, cxl->cxlds); + if (IS_ERR(cxl->cxlmd)) { + pci_err(pci_dev, "CXL accel memdev creation failed"); + rc = PTR_ERR(cxl->cxlmd); + goto err; + } + return 0; err: kfree(cxl->cxlds); diff --git a/include/linux/cxl/cxl.h b/include/linux/cxl/cxl.h index 08723b2d75bc..fc0859f841dc 100644 --- a/include/linux/cxl/cxl.h +++ b/include/linux/cxl/cxl.h @@ -55,4 +55,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