| Message ID | 20240516081202.27023-3-alucerop@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | RFC: add Type2 device support | expand |
On Thu, 16 May 2024 09:11:51 +0100 <alucerop@amd.com> wrote: > From: Alejandro Lucero <alucerop@amd.com> > > Differientiating Type3, aka memory expanders, from Type2, aka device > accelerators, with a new function for initializing cxl_dev_state. > > Adding a type2 driver for a CXL emulated device inside CXL kernel > testing infrastructure as a client for the functionality added. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> If you are going to keep Dan's sign-off it needs to be clear why. Either put the author to Dan and swap these two, or use a Co-developed tag. A few drive my trivial comments. > diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c > new file mode 100644 > index 000000000000..863ce7dc28ef > --- /dev/null > +++ b/tools/testing/cxl/type2/pci_type2.c > @@ -0,0 +1,80 @@ > +#include <linux/module.h> > +#include <linux/pci.h> > +#include <linux/cxl.h> > +#include <linux/cxlpci.h> > +#include <linux/cxlmem.h> > + > +struct cxl_dev_state *cxlds; > + > +#define CXL_TYPE2_MEM_SIZE (1024*1024*256) > + > +static int type2_pci_probe(struct pci_dev *pci_dev, > + const struct pci_device_id *entry) > + > +{ > + u16 dvsec; > + > + dvsec = pci_find_dvsec_capability(pci_dev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE); Add a line break somewhere in there as no real reason it needs to be all in eonline. > + Trivial: Drop this blank line to keep error check associated with the call. > + if (!dvsec) { > + pci_info(pci_dev, "No CXL capability (vendor: %x\n", pci_dev->vendor); > + return 0; Returned, so no point in the else. > + } else { > + pci_info(pci_dev, "CXL CXL_DVSEC_PCIE_DEVICE capability found"); > + } > + > + cxlds = cxl_accel_state_create(&pci_dev->dev); > + if (IS_ERR(cxlds)) > + return PTR_ERR(cxlds); > + > + pci_info(pci_dev, "Initializing cxlds..."); > + cxlds->cxl_dvsec = dvsec; > + cxlds->serial = pci_dev->dev.id; > + > + /* Should not this be based on DVSEC range size registers */ > + cxlds->dpa_res = DEFINE_RES_MEM(0, CXL_TYPE2_MEM_SIZE); > + cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, CXL_TYPE2_MEM_SIZE, "ram"); > + > + return 0; > +} > + > +static void type2_pci_remove(struct pci_dev *pci_dev) > +{ > + > +} > + > +/* PCI device ID table */ > +static const struct pci_device_id type2_pci_table[] = { > + {PCI_DEVICE(PCI_VENDOR_ID_AMD, 0xbabe)}, > + {0} /* end of list */ {} is more common. > +}; > + > +static struct pci_driver type2_pci_driver = { > + .name = KBUILD_MODNAME, > + .id_table = type2_pci_table, > + .probe = type2_pci_probe, > + .remove = type2_pci_remove, Add remove only when it does something. > +}; > + > +static int __init type2_cxl_init(void) > +{ > + int rc; > + > + rc = pci_register_driver(&type2_pci_driver); > + > + return rc; > +} > + > +static void __exit type2_cxl_exit(void) > +{ > + pci_unregister_driver(&type2_pci_driver); Unless you are adding more in here later in the series there is a macro to cover all this. module_pci_driver() > +} > + > +module_init(type2_cxl_init); > +module_exit(type2_cxl_exit); > + > +MODULE_AUTHOR("Alejadro Lucero <alucerop@amd.com>"); > +MODULE_DESCRIPTION("CXL Type2 device support, driver test"); > +MODULE_LICENSE("GPL"); > +MODULE_IMPORT_NS(CXL); > +MODULE_DEVICE_TABLE(pci, type2_pci_table);
On 5/17/24 15:30, Jonathan Cameron wrote: > On Thu, 16 May 2024 09:11:51 +0100 > <alucerop@amd.com> wrote: > >> From: Alejandro Lucero <alucerop@amd.com> >> >> Differientiating Type3, aka memory expanders, from Type2, aka device >> accelerators, with a new function for initializing cxl_dev_state. >> >> Adding a type2 driver for a CXL emulated device inside CXL kernel >> testing infrastructure as a client for the functionality added. >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> > If you are going to keep Dan's sign-off it needs to be clear why. > Either put the author to Dan and swap these two, or use a > Co-developed tag. Yes, Dan commented on this an he prefers the co-developed flag, so I'll change to that. > A few drive my trivial comments. > >> diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c >> new file mode 100644 >> index 000000000000..863ce7dc28ef >> --- /dev/null >> +++ b/tools/testing/cxl/type2/pci_type2.c >> @@ -0,0 +1,80 @@ >> +#include <linux/module.h> >> +#include <linux/pci.h> >> +#include <linux/cxl.h> >> +#include <linux/cxlpci.h> >> +#include <linux/cxlmem.h> >> + >> +struct cxl_dev_state *cxlds; >> + >> +#define CXL_TYPE2_MEM_SIZE (1024*1024*256) >> + >> +static int type2_pci_probe(struct pci_dev *pci_dev, >> + const struct pci_device_id *entry) >> + >> +{ >> + u16 dvsec; >> + >> + dvsec = pci_find_dvsec_capability(pci_dev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE); > Add a line break somewhere in there as no real reason it needs to be all in eonline. Sure. >> + > Trivial: Drop this blank line to keep error check associated with the call. OK >> + if (!dvsec) { >> + pci_info(pci_dev, "No CXL capability (vendor: %x\n", pci_dev->vendor); >> + return 0; > Returned, so no point in the else. Right. >> + } else { >> + pci_info(pci_dev, "CXL CXL_DVSEC_PCIE_DEVICE capability found"); >> + } >> + >> + cxlds = cxl_accel_state_create(&pci_dev->dev); >> + if (IS_ERR(cxlds)) >> + return PTR_ERR(cxlds); >> + >> + pci_info(pci_dev, "Initializing cxlds..."); >> + cxlds->cxl_dvsec = dvsec; >> + cxlds->serial = pci_dev->dev.id; >> + >> + /* Should not this be based on DVSEC range size registers */ >> + cxlds->dpa_res = DEFINE_RES_MEM(0, CXL_TYPE2_MEM_SIZE); >> + cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, CXL_TYPE2_MEM_SIZE, "ram"); >> + >> + return 0; >> +} >> + >> +static void type2_pci_remove(struct pci_dev *pci_dev) >> +{ >> + >> +} >> + >> +/* PCI device ID table */ >> +static const struct pci_device_id type2_pci_table[] = { >> + {PCI_DEVICE(PCI_VENDOR_ID_AMD, 0xbabe)}, >> + {0} /* end of list */ > {} is more common. OK >> +}; >> + >> +static struct pci_driver type2_pci_driver = { >> + .name = KBUILD_MODNAME, >> + .id_table = type2_pci_table, >> + .probe = type2_pci_probe, >> + .remove = type2_pci_remove, > Add remove only when it does something. OK >> +}; >> + >> +static int __init type2_cxl_init(void) >> +{ >> + int rc; >> + >> + rc = pci_register_driver(&type2_pci_driver); >> + >> + return rc; >> +} >> + >> +static void __exit type2_cxl_exit(void) >> +{ >> + pci_unregister_driver(&type2_pci_driver); > Unless you are adding more in here later in the series there is a macro > to cover all this. module_pci_driver() > I did not know about it. I'll use it. Thanks >> +} >> + >> +module_init(type2_cxl_init); >> +module_exit(type2_cxl_exit); >> + >> +MODULE_AUTHOR("Alejadro Lucero <alucerop@amd.com>"); >> +MODULE_DESCRIPTION("CXL Type2 device support, driver test"); >> +MODULE_LICENSE("GPL"); >> +MODULE_IMPORT_NS(CXL); >> +MODULE_DEVICE_TABLE(pci, type2_pci_table);
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c index 07cd0b8b026f..0336b3f14f4a 100644 --- a/drivers/cxl/core/memdev.c +++ b/drivers/cxl/core/memdev.c @@ -659,6 +659,21 @@ static void detach_memdev(struct work_struct *work) static struct lock_class_key cxl_memdev_key; +struct cxl_dev_state *cxl_accel_state_create(struct device *dev) +{ + struct cxl_dev_state *cxlds; + + cxlds = devm_kzalloc(dev, sizeof(*cxlds), GFP_KERNEL); + if (!cxlds) + return ERR_PTR(-ENOMEM); + + cxlds->dev = dev; + cxlds->type = CXL_DEVTYPE_DEVMEM; + + return cxlds; +} +EXPORT_SYMBOL_NS_GPL(cxl_accel_state_create, CXL); + static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds, const struct file_operations *fops) { diff --git a/include/linux/cxlmem.h b/include/linux/cxlmem.h index 0d26a45a4af2..e8d12b543db1 100644 --- a/include/linux/cxlmem.h +++ b/include/linux/cxlmem.h @@ -859,4 +859,6 @@ struct cxl_hdm { struct seq_file; struct dentry *cxl_debugfs_create_dir(const char *dir); void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds); + +struct cxl_dev_state *cxl_accel_state_create(struct device *dev); #endif /* __CXL_MEM_H__ */ diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild index 030b388800f0..a285719c4db6 100644 --- a/tools/testing/cxl/Kbuild +++ b/tools/testing/cxl/Kbuild @@ -69,3 +69,4 @@ cxl_core-y += cxl_core_exports.o KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes -Wmissing-declarations, $(KBUILD_CFLAGS)) obj-m += test/ +obj-m += type2/ diff --git a/tools/testing/cxl/type2/Kbuild b/tools/testing/cxl/type2/Kbuild new file mode 100644 index 000000000000..a96ad4d64924 --- /dev/null +++ b/tools/testing/cxl/type2/Kbuild @@ -0,0 +1,7 @@ +# SPDX-License-Identifier: GPL-2.0 + +obj-m += pci_type2.o + +cxl_pci_type2-y := cxl_pci_type2.o + +KBUILD_CFLAGS := $(filter-out -Wmissing-prototypes -Wmissing-declarations, $(KBUILD_CFLAGS)) diff --git a/tools/testing/cxl/type2/pci_type2.c b/tools/testing/cxl/type2/pci_type2.c new file mode 100644 index 000000000000..863ce7dc28ef --- /dev/null +++ b/tools/testing/cxl/type2/pci_type2.c @@ -0,0 +1,80 @@ +#include <linux/module.h> +#include <linux/pci.h> +#include <linux/cxl.h> +#include <linux/cxlpci.h> +#include <linux/cxlmem.h> + +struct cxl_dev_state *cxlds; + +#define CXL_TYPE2_MEM_SIZE (1024*1024*256) + +static int type2_pci_probe(struct pci_dev *pci_dev, + const struct pci_device_id *entry) + +{ + u16 dvsec; + + dvsec = pci_find_dvsec_capability(pci_dev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE); + + if (!dvsec) { + pci_info(pci_dev, "No CXL capability (vendor: %x\n", pci_dev->vendor); + return 0; + } else { + pci_info(pci_dev, "CXL CXL_DVSEC_PCIE_DEVICE capability found"); + } + + cxlds = cxl_accel_state_create(&pci_dev->dev); + if (IS_ERR(cxlds)) + return PTR_ERR(cxlds); + + pci_info(pci_dev, "Initializing cxlds..."); + cxlds->cxl_dvsec = dvsec; + cxlds->serial = pci_dev->dev.id; + + /* Should not this be based on DVSEC range size registers */ + cxlds->dpa_res = DEFINE_RES_MEM(0, CXL_TYPE2_MEM_SIZE); + cxlds->ram_res = DEFINE_RES_MEM_NAMED(0, CXL_TYPE2_MEM_SIZE, "ram"); + + return 0; +} + +static void type2_pci_remove(struct pci_dev *pci_dev) +{ + +} + +/* PCI device ID table */ +static const struct pci_device_id type2_pci_table[] = { + {PCI_DEVICE(PCI_VENDOR_ID_AMD, 0xbabe)}, + {0} /* end of list */ +}; + +static struct pci_driver type2_pci_driver = { + .name = KBUILD_MODNAME, + .id_table = type2_pci_table, + .probe = type2_pci_probe, + .remove = type2_pci_remove, +}; + +static int __init type2_cxl_init(void) +{ + int rc; + + rc = pci_register_driver(&type2_pci_driver); + + return rc; +} + +static void __exit type2_cxl_exit(void) +{ + pci_unregister_driver(&type2_pci_driver); +} + +module_init(type2_cxl_init); +module_exit(type2_cxl_exit); + +MODULE_AUTHOR("Alejadro Lucero <alucerop@amd.com>"); +MODULE_DESCRIPTION("CXL Type2 device support, driver test"); +MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS(CXL); +MODULE_DEVICE_TABLE(pci, type2_pci_table);