On Mon, 11 Jan 2021 14:51:09 -0800 Ben Widawsky <[email protected]> wrote:
> All the necessary bits are initialized in order to find and map the > register space for CXL Memory Devices. This is accomplished by using the > Register Locator DVSEC (CXL 2.0 - 8.1.9.1) to determine which PCI BAR to > use, and how much of an offset from that BAR should be added. > > If the memory device registers are found and mapped a new internal data > structure tracking device state is allocated. > > Signed-off-by: Ben Widawsky <[email protected]> Some issues with managed allocations being manually freed in remove. It shouldn't be necessary to do that. > --- > drivers/cxl/cxl.h | 17 ++++++++ > drivers/cxl/mem.c | 100 +++++++++++++++++++++++++++++++++++++++++++++- > drivers/cxl/pci.h | 14 +++++++ > 3 files changed, 130 insertions(+), 1 deletion(-) > create mode 100644 drivers/cxl/cxl.h > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > new file mode 100644 > index 000000000000..d81d0ba4617c > --- /dev/null > +++ b/drivers/cxl/cxl.h > @@ -0,0 +1,17 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* Copyright(c) 2020 Intel Corporation. */ > + > +#ifndef __CXL_H__ > +#define __CXL_H__ > + > +/** > + * struct cxl_mem - A CXL memory device > + * @pdev: The PCI device associated with this CXL device. > + * @regs: IO mappings to the device's MMIO > + */ > +struct cxl_mem { > + struct pci_dev *pdev; > + void __iomem *regs; > +}; > + > +#endif > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index 005404888942..8301db34d2ff 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -5,6 +5,58 @@ > #include <linux/io.h> > #include "acpi.h" > #include "pci.h" > +#include "cxl.h" > + > +/** > + * cxl_mem_create() - Create a new &struct cxl_mem. > + * @pdev: The pci device associated with the new &struct cxl_mem. > + * @reg_lo: Lower 32b of the register locator > + * @reg_hi: Upper 32b of the register locator. > + * > + * Return: The new &struct cxl_mem on success, NULL on failure. > + * > + * Map the BAR for a CXL memory device. This BAR has the memory device's > + * registers for the device as specified in CXL specification. > + */ > +static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo, > + u32 reg_hi) > +{ > + struct device *dev = &pdev->dev; > + struct cxl_mem *cxlm; > + void __iomem *regs; > + u64 offset; > + u8 bar; > + int rc; > + > + offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK); > + bar = (reg_lo >> CXL_REGLOC_BIR_SHIFT) & CXL_REGLOC_BIR_MASK; > + > + /* Basic sanity check that BAR is big enough */ > + if (pci_resource_len(pdev, bar) < offset) { > + dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar, > + &pdev->resource[bar], (unsigned long long)offset); > + return NULL; > + } > + > + rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev)); > + if (rc != 0) { > + dev_err(dev, "failed to map registers\n"); > + return NULL; > + } > + > + cxlm = devm_kzalloc(&pdev->dev, sizeof(*cxlm), GFP_KERNEL); > + if (!cxlm) { > + dev_err(dev, "No memory available\n"); > + return NULL; > + } > + > + regs = pcim_iomap_table(pdev)[bar]; > + cxlm->pdev = pdev; > + cxlm->regs = regs + offset; > + > + dev_dbg(dev, "Mapped CXL Memory Device resource\n"); > + return cxlm; > +} > > static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) > { > @@ -33,7 +85,8 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) > static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id > *id) > { > struct device *dev = &pdev->dev; > - int rc, regloc; > + struct cxl_mem *cxlm; > + int rc, regloc, i; > > rc = cxl_bus_acquire(pdev); > if (rc != 0) { > @@ -41,15 +94,59 @@ static int cxl_mem_probe(struct pci_dev *pdev, const > struct pci_device_id *id) > return rc; > } > > + rc = pcim_enable_device(pdev); > + if (rc) > + return rc; > + > regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC); > if (!regloc) { > dev_err(dev, "register location dvsec not found\n"); > return -ENXIO; > } > + regloc += 0xc; /* Skip DVSEC + reserved fields */ > + > + rc = -ENXIO; > + for (i = regloc; i < regloc + 0x24; i += 8) { > + u32 reg_lo, reg_hi; > + u8 reg_type; > + > + /* "register low and high" contain other bits */ > + pci_read_config_dword(pdev, i, ®_lo); > + pci_read_config_dword(pdev, i + 4, ®_hi); > + > + reg_type = > + (reg_lo >> CXL_REGLOC_RBI_SHIFT) & CXL_REGLOC_RBI_MASK; > + > + if (reg_type == CXL_REGLOC_RBI_MEMDEV) { > + rc = 0; > + cxlm = cxl_mem_create(pdev, reg_lo, reg_hi); > + if (!cxlm) > + rc = -ENODEV; > + break; > + } > + } > + > + if (rc) > + return rc; > > + pci_set_drvdata(pdev, cxlm); After below cleanup, not needed yet.. > return 0; > } > > +static void cxl_mem_remove(struct pci_dev *pdev) > +{ > + struct cxl_mem *cxlm; > + > + cxlm = pci_get_drvdata(pdev); > + if (!cxlm) > + return; > + > + kfree(cxlm); There is bunch of unwinding here that I'd expect to see in error paths for probe but it's not there... Which made me wonder. So pcim_iounmap_regions is a managed interface, why are call it by hand? Same is true of the allocation of cxlm above. So currently this remove isn't doing anything useful. > + > + pcim_iounmap_regions(pdev, ~0); > + pci_set_drvdata(pdev, NULL); This hasn't been needed for a long time. Example of removal of similar... http://patchwork.ozlabs.org/project/netdev/patch/005801ceaec1$6b8d3320$42a79960$%[email protected]/ > +} > + > static const struct pci_device_id cxl_mem_pci_tbl[] = { > /* PCI class code for CXL.mem Type-3 Devices */ > { PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, > @@ -62,6 +159,7 @@ static struct pci_driver cxl_mem_driver = { > .name = KBUILD_MODNAME, > .id_table = cxl_mem_pci_tbl, > .probe = cxl_mem_probe, > + .remove = cxl_mem_remove, > }; > > MODULE_LICENSE("GPL v2"); > diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h > index a8a9935fa90b..df222edb6ac3 100644 > --- a/drivers/cxl/pci.h > +++ b/drivers/cxl/pci.h > @@ -17,4 +17,18 @@ > > #define PCI_DVSEC_ID_CXL_REGLOC 0x8 > > +/* BAR Indicator Register (BIR) */ > +#define CXL_REGLOC_BIR_SHIFT 0 > +#define CXL_REGLOC_BIR_MASK 0x7 > + > +/* Register Block Identifier (RBI) */ > +#define CXL_REGLOC_RBI_SHIFT 8 > +#define CXL_REGLOC_RBI_MASK 0xff > +#define CXL_REGLOC_RBI_EMPTY 0 > +#define CXL_REGLOC_RBI_COMPONENT 1 > +#define CXL_REGLOC_RBI_VIRT 2 > +#define CXL_REGLOC_RBI_MEMDEV 3 > + > +#define CXL_REGLOC_ADDR_MASK 0xffff0000 > + > #endif /* __CXL_PCI_H__ */
