Re: [RFC PATCH 02/13] cxl: add type2 device basic support

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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);