From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DB8A1C433E6 for ; Tue, 12 Jan 2021 19:15:31 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 9F0A823107 for ; Tue, 12 Jan 2021 19:15:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2406510AbhALTPF (ORCPT ); Tue, 12 Jan 2021 14:15:05 -0500 Received: from frasgout.his.huawei.com ([185.176.79.56]:2326 "EHLO frasgout.his.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2406329AbhALTPE (ORCPT ); Tue, 12 Jan 2021 14:15:04 -0500 Received: from fraeml744-chm.china.huawei.com (unknown [172.18.147.206]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4DFg9S12ZNz67Xf7; Wed, 13 Jan 2021 03:09:16 +0800 (CST) Received: from lhreml710-chm.china.huawei.com (10.201.108.61) by fraeml744-chm.china.huawei.com (10.206.15.225) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2106.2; Tue, 12 Jan 2021 20:14:22 +0100 Received: from localhost (10.47.65.219) by lhreml710-chm.china.huawei.com (10.201.108.61) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2106.2; Tue, 12 Jan 2021 19:14:21 +0000 Date: Tue, 12 Jan 2021 19:13:42 +0000 From: Jonathan Cameron To: Ben Widawsky CC: , , , "linux-acpi@vger.kernel.org, Ira Weiny" , Dan Williams , "Vishal Verma" , "Kelley, Sean V" , Rafael Wysocki , "Bjorn Helgaas" , Jon Masters , Chris Browy , Randy Dunlap , "Christoph Hellwig" , Subject: Re: [RFC PATCH v3 05/16] cxl/mem: Map memory device registers Message-ID: <20210112191342.00006aad@Huawei.com> In-Reply-To: <20210111225121.820014-6-ben.widawsky@intel.com> References: <20210111225121.820014-1-ben.widawsky@intel.com> <20210111225121.820014-6-ben.widawsky@intel.com> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; i686-w64-mingw32) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.47.65.219] X-ClientProxiedBy: lhreml752-chm.china.huawei.com (10.201.108.202) To lhreml710-chm.china.huawei.com (10.201.108.61) X-CFilter-Loop: Reflected Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 11 Jan 2021 14:51:09 -0800 Ben Widawsky 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 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 > #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$%25han@samsung.com/ > +} > + > 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__ */