| Message ID | 20210506223654.1310516-3-ira.weiny@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Map register blocks individually | expand |
On Thu, May 6, 2021 at 3:37 PM <ira.weiny@intel.com> wrote: > > From: Ira Weiny <ira.weiny@intel.com> > > In order to remap individual register sets each bar region must be > reserved prior to mapping. Because the details of individual register > sets are contained within the BARs themselves, the bar must be mapped 2 > times, once to extract this information and a second time for each > register set. > > Rather than attempt to reserve each BAR individually and track if that > bar has been reserved. Open code pcim_iomap_regions() by first > reserving all memory regions on the device and then mapping the bars > individually as needed. > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > --- > drivers/cxl/pci.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 191603b4e10b..40016709b310 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -926,9 +926,9 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 > { > struct pci_dev *pdev = cxlm->pdev; > struct device *dev = &pdev->dev; > + void __iomem *rc; > u64 offset; > u8 bar; > - int rc; > > offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK); > bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo); > @@ -940,13 +940,14 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 > return (void __iomem *)ERR_PTR(-ENXIO); > } > > - rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev)); > - if (rc) { > + rc = pcim_iomap(pdev, bar, 0); It is odd that the temporary region pinning uses non-pcim, but the temporary mapping using pcim. > + if (!rc) { > dev_err(dev, "failed to map registers\n"); > - return (void __iomem *)ERR_PTR(rc); > + return (void __iomem *)ERR_PTR(-ENOMEM); I think since this support is a bug/compatibility fix it should probably be rebased to current cxl.git#next. If you still end up needing to return an __iomem ERR_PTR() then use IOMEM_ERR_PTR. > } > > - dev_dbg(dev, "Mapped CXL Memory Device resource\n"); > + dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ 0x%llx\n", s/0x%llx/%#llx/ > + bar, offset); > > return pcim_iomap_table(pdev)[bar] + offset; > } > @@ -999,6 +1000,9 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) > return -ENXIO; > } > > + if (pci_request_mem_regions(pdev, pci_name(pdev))) This tells me this patch is too fine grained and can't stand alone because it is missing the corresponding call to pci_release_mem_regions(). Ideally this would be kept in the same scope as the temporary io mapping. > + return -ENODEV; > + > /* Get the size of the Register Locator DVSEC */ > pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, ®loc_size); > regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size); > -- > 2.28.0.rc0.12.gb6a658bd00c9 >
On Wed, May 19, 2021 at 06:00:51PM -0700, Dan Williams wrote: > On Thu, May 6, 2021 at 3:37 PM <ira.weiny@intel.com> wrote: > > > > From: Ira Weiny <ira.weiny@intel.com> > > > > In order to remap individual register sets each bar region must be > > reserved prior to mapping. Because the details of individual register > > sets are contained within the BARs themselves, the bar must be mapped 2 > > times, once to extract this information and a second time for each > > register set. > > > > Rather than attempt to reserve each BAR individually and track if that > > bar has been reserved. Open code pcim_iomap_regions() by first > > reserving all memory regions on the device and then mapping the bars > > individually as needed. > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > > drivers/cxl/pci.c | 14 +++++++++----- > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > > index 191603b4e10b..40016709b310 100644 > > --- a/drivers/cxl/pci.c > > +++ b/drivers/cxl/pci.c > > @@ -926,9 +926,9 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 > > { > > struct pci_dev *pdev = cxlm->pdev; > > struct device *dev = &pdev->dev; > > + void __iomem *rc; > > u64 offset; > > u8 bar; > > - int rc; > > > > offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK); > > bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo); > > @@ -940,13 +940,14 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 > > return (void __iomem *)ERR_PTR(-ENXIO); > > } > > > > - rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev)); > > - if (rc) { > > + rc = pcim_iomap(pdev, bar, 0); > > It is odd that the temporary region pinning uses non-pcim, but the > temporary mapping using pcim. The idea of this series was to have only the final mappings be managed. But this particular patch does not make that change. Therefore I continue using pcim_iomap() for this patch. > > > + if (!rc) { > > dev_err(dev, "failed to map registers\n"); > > - return (void __iomem *)ERR_PTR(rc); > > + return (void __iomem *)ERR_PTR(-ENOMEM); > > I think since this support is a bug/compatibility fix it should > probably be rebased to current cxl.git#next. If you still end up > needing to return an __iomem ERR_PTR() then use IOMEM_ERR_PTR. Actually the type of rc has changed and I think it is best to just 'return rc;' here. Also, in a side conversation with Dan it seems the name would be more clear. So I will change it. > > > } > > > > - dev_dbg(dev, "Mapped CXL Memory Device resource\n"); > > + dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ 0x%llx\n", > > s/0x%llx/%#llx/ ok > > > + bar, offset); > > > > return pcim_iomap_table(pdev)[bar] + offset; > > } > > @@ -999,6 +1000,9 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) > > return -ENXIO; > > } > > > > + if (pci_request_mem_regions(pdev, pci_name(pdev))) > > This tells me this patch is too fine grained and can't stand alone > because it is missing the corresponding call to > pci_release_mem_regions(). Ideally this would be kept in the same > scope as the temporary io mapping. Ok, I was a bit confused why I did not need this but I don't. Dan and I discussed this offline and because pcim_enable_device() is called all pci functions are automatically managed. So the core will call pci_release_mem_regions() automatically for me. It took a bit of time for us to understand where the release call was. I'll add a comment to this to make this more clear. Ira > > > + return -ENODEV; > > + > > /* Get the size of the Register Locator DVSEC */ > > pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, ®loc_size); > > regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size); > > -- > > 2.28.0.rc0.12.gb6a658bd00c9 > >
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index 191603b4e10b..40016709b310 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -926,9 +926,9 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 { struct pci_dev *pdev = cxlm->pdev; struct device *dev = &pdev->dev; + void __iomem *rc; u64 offset; u8 bar; - int rc; offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK); bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo); @@ -940,13 +940,14 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 return (void __iomem *)ERR_PTR(-ENXIO); } - rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev)); - if (rc) { + rc = pcim_iomap(pdev, bar, 0); + if (!rc) { dev_err(dev, "failed to map registers\n"); - return (void __iomem *)ERR_PTR(rc); + return (void __iomem *)ERR_PTR(-ENOMEM); } - dev_dbg(dev, "Mapped CXL Memory Device resource\n"); + dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ 0x%llx\n", + bar, offset); return pcim_iomap_table(pdev)[bar] + offset; } @@ -999,6 +1000,9 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) return -ENXIO; } + if (pci_request_mem_regions(pdev, pci_name(pdev))) + return -ENODEV; + /* Get the size of the Register Locator DVSEC */ pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, ®loc_size); regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size);