Re: [PATCH v7 03/60] sparc/PCI: Unify pci_register_region()

[Bjorn Helgaas]

On Thu, Oct 08, 2015 at 02:38:22PM -0700, Yinghai Lu wrote:
> We register regions for legacy and iommu and all have open code.
> 
> Unify them to pci_register_region() and call it accordingly.
> 
> Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
> ---
>  arch/sparc/kernel/pci_common.c | 83 +++++++++++++++++++-----------------------
>  1 file changed, 37 insertions(+), 46 deletions(-)
> 
> diff --git a/arch/sparc/kernel/pci_common.c b/arch/sparc/kernel/pci_common.c
> index 28e976a..c4b6989 100644
> --- a/arch/sparc/kernel/pci_common.c
> +++ b/arch/sparc/kernel/pci_common.c
> @@ -328,41 +328,49 @@ void pci_get_pbm_props(struct pci_pbm_info *pbm)
>  	}
>  }
>  
> -static void pci_register_legacy_regions(struct resource *io_res,
> -					struct resource *mem_res)
> +static void pci_register_region(struct pci_pbm_info *pbm, const char *name,
> +				resource_size_t rstart, resource_size_t size)
>  {
> -	struct resource *p;
> +	struct resource *res, *conflict;
> +	struct resource *mem_res = &pbm->mem_space;
> +	resource_size_t offset = pbm->mem_offset;
> +	resource_size_t mem_rstart, mem_rend;
> +	resource_size_t rend = rstart + size - 1UL;
>  
> -	/* VGA Video RAM. */
> -	p = kzalloc(sizeof(*p), GFP_KERNEL);
> -	if (!p)
> +	if (!mem_res->flags)
>  		return;
>  
> -	p->name = "Video RAM area";
> -	p->start = mem_res->start + 0xa0000UL;
> -	p->end = p->start + 0x1ffffUL;
> -	p->flags = IORESOURCE_BUSY;
> -	request_resource(mem_res, p);
> +	mem_rstart = mem_res->start - offset;
> +	mem_rend = mem_res->end - offset;

This is essentially doing pcibios_bus_to_resource(), except in an
unnecessarily arch-dependent way.  Can you rework it to use
pcibios_bus_to_resource()?  I think you'll have to do
pci_register_legacy_regions() in pci_scan_one_pbm() instead of in
pci_determine_mem_io_space(), so that you have a struct pci_bus to
use, but your next patch does that anyway.

>  
> -	p = kzalloc(sizeof(*p), GFP_KERNEL);
> -	if (!p)
> +	/* contain checking */
> +	if (!(mem_rstart <= rstart && mem_rend >= rend))
>  		return;
>  
> -	p->name = "System ROM";
> -	p->start = mem_res->start + 0xf0000UL;
> -	p->end = p->start + 0xffffUL;
> -	p->flags = IORESOURCE_BUSY;
> -	request_resource(mem_res, p);
> -
> -	p = kzalloc(sizeof(*p), GFP_KERNEL);
> -	if (!p)
> +	res = kzalloc(sizeof(*res), GFP_KERNEL);
> +	if (!res)
>  		return;
>  
> -	p->name = "Video ROM";
> -	p->start = mem_res->start + 0xc0000UL;
> -	p->end = p->start + 0x7fffUL;
> -	p->flags = IORESOURCE_BUSY;
> -	request_resource(mem_res, p);
> +	res->name = name;
> +	res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> +	res->start = rstart + offset;
> +	res->end = rend + offset;
> +	conflict = request_resource_conflict(mem_res, res);
> +	if (conflict) {
> +		printk(KERN_DEBUG "PCI: %s can't claim %s %pR: address conflict with %s %pR\n",
> +			pbm->name, res->name, res, conflict->name, conflict);
> +		kfree(res);
> +	}
> +}
> +
> +static void pci_register_legacy_regions(struct pci_pbm_info *pbm)
> +{
> +	/* VGA Video RAM. */
> +	pci_register_region(pbm, "Video RAM area", 0xa0000UL, 0x20000UL);
> +
> +	pci_register_region(pbm, "System ROM",     0xf0000UL, 0x10000UL);
> +
> +	pci_register_region(pbm, "Video ROM",      0xc0000UL,  0x8000UL);
>  }
>  
>  static void pci_register_iommu_region(struct pci_pbm_info *pbm)
> @@ -370,24 +378,8 @@ static void pci_register_iommu_region(struct pci_pbm_info *pbm)
>  	const u32 *vdma = of_get_property(pbm->op->dev.of_node, "virtual-dma",
>  					  NULL);
>  
> -	if (vdma) {
> -		struct resource *rp = kzalloc(sizeof(*rp), GFP_KERNEL);
> -
> -		if (!rp) {
> -			pr_info("%s: Cannot allocate IOMMU resource.\n",
> -				pbm->name);
> -			return;
> -		}
> -		rp->name = "IOMMU";
> -		rp->start = pbm->mem_space.start + (unsigned long) vdma[0];
> -		rp->end = rp->start + (unsigned long) vdma[1] - 1UL;
> -		rp->flags = IORESOURCE_BUSY;
> -		if (request_resource(&pbm->mem_space, rp)) {
> -			pr_info("%s: Unable to request IOMMU resource.\n",
> -				pbm->name);
> -			kfree(rp);
> -		}
> -	}
> +	if (vdma)
> +		pci_register_region(pbm, "IOMMU", vdma[0], vdma[1]);
>  }
>  
>  void pci_determine_mem_io_space(struct pci_pbm_info *pbm)
> @@ -506,8 +498,7 @@ void pci_determine_mem_io_space(struct pci_pbm_info *pbm)
>  	if (pbm->mem64_space.flags)
>  		request_resource(&iomem_resource, &pbm->mem64_space);
>  
> -	pci_register_legacy_regions(&pbm->io_space,
> -				    &pbm->mem_space);
> +	pci_register_legacy_regions(pbm);
>  	pci_register_iommu_region(pbm);
>  }
>  
> -- 
> 1.8.4.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/