| Message ID | 20210407222625.320177-5-ben.widawsky@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Enumerate HDM Decoder registers | expand |
On Wed, 7 Apr 2021 15:26:22 -0700 Ben Widawsky <ben.widawsky@intel.com> wrote: > @cxlm.base only existed to support holding the base found in the > register block mapping code, and pass it along to the register setup > code. Now that the register setup function has all logic around managing > the registers, from DVSEC to iomapping up to populating our CXL specific > information, it is easy to turn the @base values into local variables > and remove them from our device driver state. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> Patch is basically fine, but I do wonder if you could avoid the nasty casting in and out of __iomem in the error paths. It's a common enough idiom though so I'm not htat fussed. Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/mem.c | 24 +++++++++++------------- > drivers/cxl/mem.h | 2 -- > 2 files changed, 11 insertions(+), 15 deletions(-) > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index 04b4f7445083..60b95c524c3e 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -922,11 +922,10 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev) > return cxlm; > } > > -static int cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi) > +static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi) > { > struct pci_dev *pdev = cxlm->pdev; > struct device *dev = &pdev->dev; > - void __iomem *regs; > u64 offset; > u8 bar; > int rc; > @@ -938,20 +937,18 @@ static int cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi) > 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 -ENXIO; > + return (void __iomem *)ERR_PTR(-ENXIO); > } > > rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev)); > if (rc) { > dev_err(dev, "failed to map registers\n"); > - return rc; > + return (void __iomem *)ERR_PTR(rc); The casting is fairly horrible, perhaps just pass in a void __iomem ** and pass base back through that? > } > - regs = pcim_iomap_table(pdev)[bar]; > - > - cxlm->base = regs + offset; > > dev_dbg(dev, "Mapped CXL Memory Device resource\n"); > - return 0; > + > + return pcim_iomap_table(pdev)[bar] + offset; > } > > static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) > @@ -993,7 +990,8 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) > struct pci_dev *pdev = cxlm->pdev; > struct device *dev = &pdev->dev; > u32 regloc_size, regblocks; > - int rc, regloc, i; > + void __iomem *base; > + int regloc, i; > > regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_OFFSET); > if (!regloc) { > @@ -1019,9 +1017,9 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) > reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo); > > if (reg_type == CXL_REGLOC_RBI_MEMDEV) { > - rc = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi); > - if (rc) > - return rc; > + base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi); > + if (IS_ERR(base)) > + return PTR_ERR(base); > break; > } > } > @@ -1031,7 +1029,7 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) > return -ENXIO; > } > > - cxl_setup_device_regs(dev, cxlm->base, ®s->device_regs); > + cxl_setup_device_regs(dev, base, ®s->device_regs); > > if (!regs->status || !regs->mbox || !regs->memdev) { > dev_err(dev, "registers not found: %s%s%s\n", > diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h > index 8bad7166adba..bfcfef461b16 100644 > --- a/drivers/cxl/mem.h > +++ b/drivers/cxl/mem.h > @@ -49,7 +49,6 @@ struct cxl_memdev { > /** > * struct cxl_mem - A CXL memory device > * @pdev: The PCI device associated with this CXL device. > - * @base: IO mappings to the device's MMIO > * @cxlmd: Logical memory device chardev / interface > * @regs: Parsed register blocks > * @payload_size: Size of space for payload > @@ -62,7 +61,6 @@ struct cxl_memdev { > */ > struct cxl_mem { > struct pci_dev *pdev; > - void __iomem *base; > struct cxl_memdev *cxlmd; > > struct cxl_regs regs;
On 21-04-08 18:26:35, Jonathan Cameron wrote: > On Wed, 7 Apr 2021 15:26:22 -0700 > Ben Widawsky <ben.widawsky@intel.com> wrote: > > > @cxlm.base only existed to support holding the base found in the > > register block mapping code, and pass it along to the register setup > > code. Now that the register setup function has all logic around managing > > the registers, from DVSEC to iomapping up to populating our CXL specific > > information, it is easy to turn the @base values into local variables > > and remove them from our device driver state. > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > Patch is basically fine, but I do wonder if you could avoid the > nasty casting in and out of __iomem in the error paths. > > It's a common enough idiom though so I'm not htat fussed. > > Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > --- > > drivers/cxl/mem.c | 24 +++++++++++------------- > > drivers/cxl/mem.h | 2 -- > > 2 files changed, 11 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > index 04b4f7445083..60b95c524c3e 100644 > > --- a/drivers/cxl/mem.c > > +++ b/drivers/cxl/mem.c > > @@ -922,11 +922,10 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev) > > return cxlm; > > } > > > > -static int cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi) > > +static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi) > > { > > struct pci_dev *pdev = cxlm->pdev; > > struct device *dev = &pdev->dev; > > - void __iomem *regs; > > u64 offset; > > u8 bar; > > int rc; > > @@ -938,20 +937,18 @@ static int cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi) > > 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 -ENXIO; > > + return (void __iomem *)ERR_PTR(-ENXIO); > > } > > > > rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev)); > > if (rc) { > > dev_err(dev, "failed to map registers\n"); > > - return rc; > > + return (void __iomem *)ERR_PTR(rc); > > The casting is fairly horrible, perhaps just pass in > a void __iomem ** and pass base back through that? > TIL: IOMEM_ERR_PTR. Would that suffice? > > } > > - regs = pcim_iomap_table(pdev)[bar]; > > - > > - cxlm->base = regs + offset; > > > > dev_dbg(dev, "Mapped CXL Memory Device resource\n"); > > - return 0; > > + > > + return pcim_iomap_table(pdev)[bar] + offset; > > } > > > > static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) > > @@ -993,7 +990,8 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) > > struct pci_dev *pdev = cxlm->pdev; > > struct device *dev = &pdev->dev; > > u32 regloc_size, regblocks; > > - int rc, regloc, i; > > + void __iomem *base; > > + int regloc, i; > > > > regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_OFFSET); > > if (!regloc) { > > @@ -1019,9 +1017,9 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) > > reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo); > > > > if (reg_type == CXL_REGLOC_RBI_MEMDEV) { > > - rc = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi); > > - if (rc) > > - return rc; > > + base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi); > > + if (IS_ERR(base)) > > + return PTR_ERR(base); > > break; > > } > > } > > @@ -1031,7 +1029,7 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) > > return -ENXIO; > > } > > > > - cxl_setup_device_regs(dev, cxlm->base, ®s->device_regs); > > + cxl_setup_device_regs(dev, base, ®s->device_regs); > > > > if (!regs->status || !regs->mbox || !regs->memdev) { > > dev_err(dev, "registers not found: %s%s%s\n", > > diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h > > index 8bad7166adba..bfcfef461b16 100644 > > --- a/drivers/cxl/mem.h > > +++ b/drivers/cxl/mem.h > > @@ -49,7 +49,6 @@ struct cxl_memdev { > > /** > > * struct cxl_mem - A CXL memory device > > * @pdev: The PCI device associated with this CXL device. > > - * @base: IO mappings to the device's MMIO > > * @cxlmd: Logical memory device chardev / interface > > * @regs: Parsed register blocks > > * @payload_size: Size of space for payload > > @@ -62,7 +61,6 @@ struct cxl_memdev { > > */ > > struct cxl_mem { > > struct pci_dev *pdev; > > - void __iomem *base; > > struct cxl_memdev *cxlmd; > > > > struct cxl_regs regs; >
On Tue, 13 Apr 2021 09:17:26 -0700 Ben Widawsky <ben.widawsky@intel.com> wrote: > On 21-04-08 18:26:35, Jonathan Cameron wrote: > > On Wed, 7 Apr 2021 15:26:22 -0700 > > Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > @cxlm.base only existed to support holding the base found in the > > > register block mapping code, and pass it along to the register setup > > > code. Now that the register setup function has all logic around managing > > > the registers, from DVSEC to iomapping up to populating our CXL specific > > > information, it is easy to turn the @base values into local variables > > > and remove them from our device driver state. > > > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > > > Patch is basically fine, but I do wonder if you could avoid the > > nasty casting in and out of __iomem in the error paths. > > > > It's a common enough idiom though so I'm not htat fussed. > > > > Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > --- > > > drivers/cxl/mem.c | 24 +++++++++++------------- > > > drivers/cxl/mem.h | 2 -- > > > 2 files changed, 11 insertions(+), 15 deletions(-) > > > > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > > index 04b4f7445083..60b95c524c3e 100644 > > > --- a/drivers/cxl/mem.c > > > +++ b/drivers/cxl/mem.c > > > @@ -922,11 +922,10 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev) > > > return cxlm; > > > } > > > > > > -static int cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi) > > > +static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi) > > > { > > > struct pci_dev *pdev = cxlm->pdev; > > > struct device *dev = &pdev->dev; > > > - void __iomem *regs; > > > u64 offset; > > > u8 bar; > > > int rc; > > > @@ -938,20 +937,18 @@ static int cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi) > > > 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 -ENXIO; > > > + return (void __iomem *)ERR_PTR(-ENXIO); > > > } > > > > > > rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev)); > > > if (rc) { > > > dev_err(dev, "failed to map registers\n"); > > > - return rc; > > > + return (void __iomem *)ERR_PTR(rc); > > > > The casting is fairly horrible, perhaps just pass in > > a void __iomem ** and pass base back through that? > > > > TIL: IOMEM_ERR_PTR. Would that suffice? Definitely. Didn't know about that! Jonathan
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c index 04b4f7445083..60b95c524c3e 100644 --- a/drivers/cxl/mem.c +++ b/drivers/cxl/mem.c @@ -922,11 +922,10 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev) return cxlm; } -static int cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi) +static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi) { struct pci_dev *pdev = cxlm->pdev; struct device *dev = &pdev->dev; - void __iomem *regs; u64 offset; u8 bar; int rc; @@ -938,20 +937,18 @@ static int cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi) 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 -ENXIO; + return (void __iomem *)ERR_PTR(-ENXIO); } rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev)); if (rc) { dev_err(dev, "failed to map registers\n"); - return rc; + return (void __iomem *)ERR_PTR(rc); } - regs = pcim_iomap_table(pdev)[bar]; - - cxlm->base = regs + offset; dev_dbg(dev, "Mapped CXL Memory Device resource\n"); - return 0; + + return pcim_iomap_table(pdev)[bar] + offset; } static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec) @@ -993,7 +990,8 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) struct pci_dev *pdev = cxlm->pdev; struct device *dev = &pdev->dev; u32 regloc_size, regblocks; - int rc, regloc, i; + void __iomem *base; + int regloc, i; regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_OFFSET); if (!regloc) { @@ -1019,9 +1017,9 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo); if (reg_type == CXL_REGLOC_RBI_MEMDEV) { - rc = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi); - if (rc) - return rc; + base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi); + if (IS_ERR(base)) + return PTR_ERR(base); break; } } @@ -1031,7 +1029,7 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm) return -ENXIO; } - cxl_setup_device_regs(dev, cxlm->base, ®s->device_regs); + cxl_setup_device_regs(dev, base, ®s->device_regs); if (!regs->status || !regs->mbox || !regs->memdev) { dev_err(dev, "registers not found: %s%s%s\n", diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h index 8bad7166adba..bfcfef461b16 100644 --- a/drivers/cxl/mem.h +++ b/drivers/cxl/mem.h @@ -49,7 +49,6 @@ struct cxl_memdev { /** * struct cxl_mem - A CXL memory device * @pdev: The PCI device associated with this CXL device. - * @base: IO mappings to the device's MMIO * @cxlmd: Logical memory device chardev / interface * @regs: Parsed register blocks * @payload_size: Size of space for payload @@ -62,7 +61,6 @@ struct cxl_memdev { */ struct cxl_mem { struct pci_dev *pdev; - void __iomem *base; struct cxl_memdev *cxlmd; struct cxl_regs regs;
@cxlm.base only existed to support holding the base found in the register block mapping code, and pass it along to the register setup code. Now that the register setup function has all logic around managing the registers, from DVSEC to iomapping up to populating our CXL specific information, it is easy to turn the @base values into local variables and remove them from our device driver state. Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> --- drivers/cxl/mem.c | 24 +++++++++++------------- drivers/cxl/mem.h | 2 -- 2 files changed, 11 insertions(+), 15 deletions(-)