diff mbox series

[2/8] cxl/pci: Cleanup cxl_map_device_regs()

Message ID 164740403286.3912056.2514975283929305856.stgit@dwillia2-desk3.amr.corp.intel.com
State New, archived
Headers show
Series cxl/pci: Add fundamental error handling | expand

Commit Message

Dan Williams March 16, 2022, 4:13 a.m. UTC 
Use a loop to reduce the duplicated code in cxl_map_device_regs(). This
is in preparation for deleting cxl_map_regs().

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/regs.c |   51 ++++++++++++++++++-----------------------------
 1 file changed, 20 insertions(+), 31 deletions(-)

Comments

Jonathan Cameron March 17, 2022, 10:07 a.m. UTC | #1 
On Tue, 15 Mar 2022 21:13:52 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Use a loop to reduce the duplicated code in cxl_map_device_regs(). This
> is in preparation for deleting cxl_map_regs().
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Trivial style comments inline.  Otherwise LGTM

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/regs.c |   51 ++++++++++++++++++-----------------------------
>  1 file changed, 20 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index bd6ae14b679e..bd766e461f7d 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -211,42 +211,31 @@ int cxl_map_device_regs(struct pci_dev *pdev,
>  			struct cxl_device_regs *regs,
>  			struct cxl_register_map *map)
>  {
> +	resource_size_t phys_addr =
> +		pci_resource_start(pdev, map->barno) + map->block_offset;

I'm not totally convinced by this refactoring as it's ugly either
way...  Still your code, and I don't care that strongly ;)

>  	struct device *dev = &pdev->dev;
> -	resource_size_t phys_addr;
> -
> -	phys_addr = pci_resource_start(pdev, map->barno);
> -	phys_addr += map->block_offset;
> -
> -	if (map->device_map.status.valid) {
> -		resource_size_t addr;
> +	struct mapinfo {
> +		struct cxl_reg_map *rmap;
> +		void __iomem **addr;
> +	} mapinfo[] = {
> +		{ .rmap = &map->device_map.status, &regs->status, },

Combining c99 style .rmap for first parameter and then not doing it
for the second is a bit odd looking.  Was there a strong reason for
doing this?  I'd just drop the ".rmap =" as it's not as though
we need to look far to see what it's setting.

> +		{ .rmap = &map->device_map.mbox, &regs->mbox, },
> +		{ .rmap = &map->device_map.memdev, &regs->memdev, },
> +	};
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(mapinfo); i++) {
> +		struct mapinfo *mi = &mapinfo[i];
>  		resource_size_t length;
> -
> -		addr = phys_addr + map->device_map.status.offset;
> -		length = map->device_map.status.size;
> -		regs->status = devm_cxl_iomap_block(dev, addr, length);
> -		if (!regs->status)
> -			return -ENOMEM;
> -	}
> -
> -	if (map->device_map.mbox.valid) {
>  		resource_size_t addr;
> -		resource_size_t length;
>  
> -		addr = phys_addr + map->device_map.mbox.offset;
> -		length = map->device_map.mbox.size;
> -		regs->mbox = devm_cxl_iomap_block(dev, addr, length);
> -		if (!regs->mbox)
> -			return -ENOMEM;
> -	}
> -
> -	if (map->device_map.memdev.valid) {
> -		resource_size_t addr;
> -		resource_size_t length;
> +		if (!mi->rmap->valid)
> +			continue;
>  
> -		addr = phys_addr + map->device_map.memdev.offset;
> -		length = map->device_map.memdev.size;
> -		regs->memdev = devm_cxl_iomap_block(dev, addr, length);
> -		if (!regs->memdev)
> +		addr = phys_addr + mi->rmap->offset;
> +		length = mi->rmap->size;
> +		*(mi->addr) = devm_cxl_iomap_block(dev, addr, length);
> +		if (!*(mi->addr))
>  			return -ENOMEM;
>  	}
>  
>
Dan Williams March 18, 2022, 5:13 p.m. UTC | #2 
On Thu, Mar 17, 2022 at 3:08 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 15 Mar 2022 21:13:52 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > Use a loop to reduce the duplicated code in cxl_map_device_regs(). This
> > is in preparation for deleting cxl_map_regs().
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Trivial style comments inline.  Otherwise LGTM
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> > ---
> >  drivers/cxl/core/regs.c |   51 ++++++++++++++++++-----------------------------
> >  1 file changed, 20 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> > index bd6ae14b679e..bd766e461f7d 100644
> > --- a/drivers/cxl/core/regs.c
> > +++ b/drivers/cxl/core/regs.c
> > @@ -211,42 +211,31 @@ int cxl_map_device_regs(struct pci_dev *pdev,
> >                       struct cxl_device_regs *regs,
> >                       struct cxl_register_map *map)
> >  {
> > +     resource_size_t phys_addr =
> > +             pci_resource_start(pdev, map->barno) + map->block_offset;
>
> I'm not totally convinced by this refactoring as it's ugly either
> way...  Still your code, and I don't care that strongly ;)

Fair enough, but isn't there intrinsic beauty in a diff that deletes
more code than it adds?

The cleaner aspect to me is that the RAS Capability Structure support
can be added with a one line change rather than a new if block in
cxl_map_component_regs().

>
> >       struct device *dev = &pdev->dev;
> > -     resource_size_t phys_addr;
> > -
> > -     phys_addr = pci_resource_start(pdev, map->barno);
> > -     phys_addr += map->block_offset;
> > -
> > -     if (map->device_map.status.valid) {
> > -             resource_size_t addr;
> > +     struct mapinfo {
> > +             struct cxl_reg_map *rmap;
> > +             void __iomem **addr;
> > +     } mapinfo[] = {
> > +             { .rmap = &map->device_map.status, &regs->status, },
>
> Combining c99 style .rmap for first parameter and then not doing it
> for the second is a bit odd looking.  Was there a strong reason for
> doing this?  I'd just drop the ".rmap =" as it's not as though
> we need to look far to see what it's setting.

Good catch, yeah, not sure why I typed it that way, will fix.
diff mbox series

Patch

diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index bd6ae14b679e..bd766e461f7d 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -211,42 +211,31 @@  int cxl_map_device_regs(struct pci_dev *pdev,
 			struct cxl_device_regs *regs,
 			struct cxl_register_map *map)
 {
+	resource_size_t phys_addr =
+		pci_resource_start(pdev, map->barno) + map->block_offset;
 	struct device *dev = &pdev->dev;
-	resource_size_t phys_addr;
-
-	phys_addr = pci_resource_start(pdev, map->barno);
-	phys_addr += map->block_offset;
-
-	if (map->device_map.status.valid) {
-		resource_size_t addr;
+	struct mapinfo {
+		struct cxl_reg_map *rmap;
+		void __iomem **addr;
+	} mapinfo[] = {
+		{ .rmap = &map->device_map.status, &regs->status, },
+		{ .rmap = &map->device_map.mbox, &regs->mbox, },
+		{ .rmap = &map->device_map.memdev, &regs->memdev, },
+	};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(mapinfo); i++) {
+		struct mapinfo *mi = &mapinfo[i];
 		resource_size_t length;
-
-		addr = phys_addr + map->device_map.status.offset;
-		length = map->device_map.status.size;
-		regs->status = devm_cxl_iomap_block(dev, addr, length);
-		if (!regs->status)
-			return -ENOMEM;
-	}
-
-	if (map->device_map.mbox.valid) {
 		resource_size_t addr;
-		resource_size_t length;
 
-		addr = phys_addr + map->device_map.mbox.offset;
-		length = map->device_map.mbox.size;
-		regs->mbox = devm_cxl_iomap_block(dev, addr, length);
-		if (!regs->mbox)
-			return -ENOMEM;
-	}
-
-	if (map->device_map.memdev.valid) {
-		resource_size_t addr;
-		resource_size_t length;
+		if (!mi->rmap->valid)
+			continue;
 
-		addr = phys_addr + map->device_map.memdev.offset;
-		length = map->device_map.memdev.size;
-		regs->memdev = devm_cxl_iomap_block(dev, addr, length);
-		if (!regs->memdev)
+		addr = phys_addr + mi->rmap->offset;
+		length = mi->rmap->size;
+		*(mi->addr) = devm_cxl_iomap_block(dev, addr, length);
+		if (!*(mi->addr))
 			return -ENOMEM;
 	}