Messages in this thread | | | Date | Thu, 20 May 2021 12:44:32 -0700 | From | Ira Weiny <> | Subject | Re: [PATCH 2/4] cxl/mem: Reserve all device regions at once |
| |
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 > >
| |