Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1252119 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v7 03/60] sparc/PCI: Unify pci_register_region() Bjorn Helgaas <helgaas@kernel.org> - 2015-10-21 00:00 +0200
    Re: [PATCH v7 03/60] sparc/PCI: Unify pci_register_region() Yinghai Lu <yinghai@kernel.org> - 2015-10-21 00:20 +0200
      Re: [PATCH v7 03/60] sparc/PCI: Unify pci_register_region() Yinghai Lu <yinghai@kernel.org> - 2015-10-21 00:40 +0200
        Re: [PATCH v7 03/60] sparc/PCI: Unify pci_register_region() Bjorn Helgaas <helgaas@kernel.org> - 2015-10-21 04:40 +0200
          Re: [PATCH v7 03/60] sparc/PCI: Unify pci_register_region() Yinghai Lu <yinghai@kernel.org> - 2015-10-21 20:20 +0200
            Re: [PATCH v7 03/60] sparc/PCI: Unify pci_register_region() Bjorn Helgaas <helgaas@kernel.org> - 2015-10-21 21:00 +0200
              Re: [PATCH v7 03/60] sparc/PCI: Unify pci_register_region() Yinghai Lu <yinghai@kernel.org> - 2015-10-22 00:40 +0200
                Re: [PATCH v7 03/60] sparc/PCI: Unify pci_register_region() Yinghai Lu <yinghai@kernel.org> - 2015-10-22 01:00 +0200
                  Re: [PATCH v7 03/60] sparc/PCI: Unify pci_register_region() Yinghai Lu <yinghai@kernel.org> - 2015-10-22 03:20 +0200
                    Re: [PATCH v7 03/60] sparc/PCI: Unify pci_register_region() Yinghai Lu <yinghai@kernel.org> - 2015-10-22 22:00 +0200
            Re: [PATCH v7 03/60] sparc/PCI: Unify pci_register_region() David Miller <davem@davemloft.net> - 2015-10-22 03:20 +0200
              Re: [PATCH v7 03/60] sparc/PCI: Unify pci_register_region() Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2015-10-22 05:50 +0200
                Re: [PATCH v7 03/60] sparc/PCI: Unify pci_register_region() Bjorn Helgaas <helgaas@kernel.org> - 2015-10-23 07:40 +0200

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

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@kernel.org>
> ---
>  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@vger.kernel.org
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1252210

On Tue, Oct 20, 2015 at 2:57 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 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.
>>
>> +     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;

Difference is here.

pcibios_bus_to_resource() would return resource even region is not contained.

>> +     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);
>> +     }

also we will need to get parent resource to use request_resource_conflict.

otherwise we need to use insert_resource(), but that will prevent driver to use
the resource.

        Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1252273

On Tue, Oct 20, 2015 at 3:16 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Oct 20, 2015 at 2:57 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> 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.
>>>
>>> +     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;
>
> Difference is here.
>
> pcibios_bus_to_resource() would return resource even region is not contained.
>
>>> +     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);
>>> +     }
>
> also we will need to get parent resource to use request_resource_conflict.

so do you agree following change to pcibios_bus_to_resource() ?
then we can use __pcibios_bus_resource().

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index 5f4a2e0..6c73327 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -70,12 +70,15 @@ static bool region_contains(struct pci_bus_region *region1,
     return region1->start <= region2->start && region1->end >= region2->end;
 }

-void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
-                 struct pci_bus_region *region)
+int __pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
+                 struct pci_bus_region *region,
+                 struct resource **res_ret,
+                 resource_size *off_ret)
 {
     struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
     struct resource_entry *window;
     resource_size_t offset = 0;
+    int found = 0;

     resource_list_for_each_entry(window, &bridge->windows) {
         struct pci_bus_region bus_region;
@@ -88,11 +91,23 @@ void pcibios_bus_to_resource(struct pci_bus *bus,
struct resource *res,

         if (region_contains(&bus_region, region)) {
             offset = window->offset;
+            if (off_ret)
+                *off_ret = offset;
+            if (res_ret)
+                *res_ret = window->res;
+            found = 1;
             break;
         }
     }

     res->start = region->start + offset;
     res->end = region->end + offset;
+
+    return found;
+}
+void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
+                 struct pci_bus_region *region)
+{
+    __pcibios_bus_to_resource(bus, res, region, NULL, NULL);
 }
 EXPORT_SYMBOL(pcibios_bus_to_resource);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index b54fbf1..bec3eed 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -781,6 +781,10 @@ void pci_fixup_cardbus(struct pci_bus *);

 void pcibios_resource_to_bus(struct pci_bus *bus, struct
pci_bus_region *region,
                  struct resource *res);
+int __pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
+                  struct pci_bus_region *region,
+                  struct resource **res_ret,
+                  resource_size *off_ret);
 void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
                  struct pci_bus_region *region);
 void pcibios_scan_specific_bus(int busn);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1252431

On Tue, Oct 20, 2015 at 03:30:44PM -0700, Yinghai Lu wrote:
> On Tue, Oct 20, 2015 at 3:16 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Tue, Oct 20, 2015 at 2:57 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >> 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.
> >>>
> >>> +     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;
> >
> > Difference is here.
> >
> > pcibios_bus_to_resource() would return resource even region is not contained.
> >
> >>> +     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);
> >>> +     }
> >
> > also we will need to get parent resource to use request_resource_conflict.
> 
> so do you agree following change to pcibios_bus_to_resource() ?
> then we can use __pcibios_bus_resource().

I doubt I would agree with a change like this, but maybe, if you can
explain what's unique about the sparc architecture that would require
a change like this.

> diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
> index 5f4a2e0..6c73327 100644
> --- a/drivers/pci/host-bridge.c
> +++ b/drivers/pci/host-bridge.c
> @@ -70,12 +70,15 @@ static bool region_contains(struct pci_bus_region *region1,
>      return region1->start <= region2->start && region1->end >= region2->end;
>  }
> 
> -void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
> -                 struct pci_bus_region *region)
> +int __pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
> +                 struct pci_bus_region *region,
> +                 struct resource **res_ret,
> +                 resource_size *off_ret)
>  {
>      struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
>      struct resource_entry *window;
>      resource_size_t offset = 0;
> +    int found = 0;
> 
>      resource_list_for_each_entry(window, &bridge->windows) {
>          struct pci_bus_region bus_region;
> @@ -88,11 +91,23 @@ void pcibios_bus_to_resource(struct pci_bus *bus,
> struct resource *res,
> 
>          if (region_contains(&bus_region, region)) {
>              offset = window->offset;
> +            if (off_ret)
> +                *off_ret = offset;
> +            if (res_ret)
> +                *res_ret = window->res;
> +            found = 1;
>              break;
>          }
>      }
> 
>      res->start = region->start + offset;
>      res->end = region->end + offset;
> +
> +    return found;
> +}
> +void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
> +                 struct pci_bus_region *region)
> +{
> +    __pcibios_bus_to_resource(bus, res, region, NULL, NULL);
>  }
>  EXPORT_SYMBOL(pcibios_bus_to_resource);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index b54fbf1..bec3eed 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -781,6 +781,10 @@ void pci_fixup_cardbus(struct pci_bus *);
> 
>  void pcibios_resource_to_bus(struct pci_bus *bus, struct
> pci_bus_region *region,
>                   struct resource *res);
> +int __pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
> +                  struct pci_bus_region *region,
> +                  struct resource **res_ret,
> +                  resource_size *off_ret);
>  void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
>                   struct pci_bus_region *region);
>  void pcibios_scan_specific_bus(int busn);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1253106

On Tue, Oct 20, 2015 at 7:35 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Tue, Oct 20, 2015 at 03:30:44PM -0700, Yinghai Lu wrote:
> I doubt I would agree with a change like this, but maybe, if you can
> explain what's unique about the sparc architecture that would require
> a change like this.

Then how about this one ?

otherwise we need to compare res with pbm->mem_space or pbm->mem64_space
to get direct parent for request_resource_conflict() calling in
pci_register_legacy_regions().

also check if if return is NULL to decide if it is valid region.
---
 drivers/pci/host-bridge.c |   10 +++++++++-
 include/linux/pci.h       |    5 +++--
 2 files changed, 12 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/pci/host-bridge.c
===================================================================
--- linux-2.6.orig/drivers/pci/host-bridge.c
+++ linux-2.6/drivers/pci/host-bridge.c
@@ -70,12 +70,17 @@ static bool region_contains(struct pci_b
     return region1->start <= region2->start && region1->end >= region2->end;
 }

-void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
+/*
+ * return host bridge window resource
+ */
+struct resource *pcibios_bus_to_resource(struct pci_bus *bus,
+                 struct resource *res,
                  struct pci_bus_region *region)
 {
     struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
     struct resource_entry *window;
     resource_size_t offset = 0;
+    struct resource *res_ret = NULL;

     resource_list_for_each_entry(window, &bridge->windows) {
         struct pci_bus_region bus_region;
@@ -88,11 +93,14 @@ void pcibios_bus_to_resource(struct pci_

         if (region_contains(&bus_region, region)) {
             offset = window->offset;
+            res_ret = window->res;
             break;
         }
     }

     res->start = region->start + offset;
     res->end = region->end + offset;
+
+    return res_ret;
 }
 EXPORT_SYMBOL(pcibios_bus_to_resource);
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -781,8 +781,9 @@ void pci_fixup_cardbus(struct pci_bus *)

 void pcibios_resource_to_bus(struct pci_bus *bus, struct
pci_bus_region *region,
                  struct resource *res);
-void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
-                 struct pci_bus_region *region);
+struct resource *pcibios_bus_to_resource(struct pci_bus *bus,
+                     struct resource *res,
+                     struct pci_bus_region *region);
 void pcibios_scan_specific_bus(int busn);
 struct pci_bus *pci_find_bus(int domain, int busnr);
 void pci_bus_add_devices(const struct pci_bus *bus);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1253121

On Wed, Oct 21, 2015 at 11:16:53AM -0700, Yinghai Lu wrote:
> On Tue, Oct 20, 2015 at 7:35 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Tue, Oct 20, 2015 at 03:30:44PM -0700, Yinghai Lu wrote:
> > I doubt I would agree with a change like this, but maybe, if you can
> > explain what's unique about the sparc architecture that would require
> > a change like this.
> 
> Then how about this one ?
> 
> otherwise we need to compare res with pbm->mem_space or pbm->mem64_space
> to get direct parent for request_resource_conflict() calling in
> pci_register_legacy_regions().

Lots of other architectures have both mem32 and mem64 apertures.  You
haven't explained what's unique about sparc yet.  Does
pci_find_parent_resource() not work on sparc?  If not, is that because the
resource tree looks different on sparc than on other architectures?  If so,
is that difference something intentional that we want to keep, or should we
converge on a single resource tree format across all architectures?

> also check if if return is NULL to decide if it is valid region.
> ---
>  drivers/pci/host-bridge.c |   10 +++++++++-
>  include/linux/pci.h       |    5 +++--
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6/drivers/pci/host-bridge.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/host-bridge.c
> +++ linux-2.6/drivers/pci/host-bridge.c
> @@ -70,12 +70,17 @@ static bool region_contains(struct pci_b
>      return region1->start <= region2->start && region1->end >= region2->end;
>  }
> 
> -void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
> +/*
> + * return host bridge window resource
> + */
> +struct resource *pcibios_bus_to_resource(struct pci_bus *bus,
> +                 struct resource *res,
>                   struct pci_bus_region *region)
>  {
>      struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
>      struct resource_entry *window;
>      resource_size_t offset = 0;
> +    struct resource *res_ret = NULL;
> 
>      resource_list_for_each_entry(window, &bridge->windows) {
>          struct pci_bus_region bus_region;
> @@ -88,11 +93,14 @@ void pcibios_bus_to_resource(struct pci_
> 
>          if (region_contains(&bus_region, region)) {
>              offset = window->offset;
> +            res_ret = window->res;
>              break;
>          }
>      }
> 
>      res->start = region->start + offset;
>      res->end = region->end + offset;
> +
> +    return res_ret;
>  }
>  EXPORT_SYMBOL(pcibios_bus_to_resource);
> Index: linux-2.6/include/linux/pci.h
> ===================================================================
> --- linux-2.6.orig/include/linux/pci.h
> +++ linux-2.6/include/linux/pci.h
> @@ -781,8 +781,9 @@ void pci_fixup_cardbus(struct pci_bus *)
> 
>  void pcibios_resource_to_bus(struct pci_bus *bus, struct
> pci_bus_region *region,
>                   struct resource *res);
> -void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
> -                 struct pci_bus_region *region);
> +struct resource *pcibios_bus_to_resource(struct pci_bus *bus,
> +                     struct resource *res,
> +                     struct pci_bus_region *region);
>  void pcibios_scan_specific_bus(int busn);
>  struct pci_bus *pci_find_bus(int domain, int busnr);
>  void pci_bus_add_devices(const struct pci_bus *bus);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1253314

On Wed, Oct 21, 2015 at 11:50 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> Lots of other architectures have both mem32 and mem64 apertures.  You
> haven't explained what's unique about sparc yet.  Does
> pci_find_parent_resource() not work on sparc?  If not, is that because the
> resource tree looks different on sparc than on other architectures?  If so,
> is that difference something intentional that we want to keep, or should we
> converge on a single resource tree format across all architectures?

In the new pci_register_legacy_regions(), we only have root bus.
but current pci_find_parent_resource is taking pci dev.

So will need to change it to take bus instead, please check if you are ok
with following, then I would update other two.

---
 arch/microblaze/pci/pci-common.c |    4 ++--
 arch/powerpc/kernel/pci-common.c |    4 ++--
 drivers/pci/pci.c                |    5 ++---
 drivers/pci/setup-res.c          |    4 ++--
 drivers/pcmcia/rsrc_nonstatic.c  |    2 +-
 include/linux/pci.h              |    2 +-
 6 files changed, 10 insertions(+), 11 deletions(-)

Index: linux-2.6/arch/microblaze/pci/pci-common.c
===================================================================
--- linux-2.6.orig/arch/microblaze/pci/pci-common.c
+++ linux-2.6/arch/microblaze/pci/pci-common.c
@@ -1006,7 +1006,7 @@ static void pcibios_allocate_bus_resourc
              * and as such ensure proper re-allocation
              * later.
              */
-            pr = pci_find_parent_resource(bus->self, res);
+            pr = pci_find_parent_resource(bus->self->bus, res);
             if (pr == res) {
                 /* this happens when the generic PCI
                  * code (wrongly) decides that this
@@ -1064,7 +1064,7 @@ static inline void alloc_resource(struct
          (unsigned long long)r->end,
          (unsigned int)r->flags);

-    pr = pci_find_parent_resource(dev, r);
+    pr = pci_find_parent_resource(dev->bus, r);
     if (!pr || (pr->flags & IORESOURCE_UNSET) ||
         request_resource(pr, r) < 0) {
         pr_warn("PCI: Cannot allocate resource region %d ", idx);
Index: linux-2.6/arch/powerpc/kernel/pci-common.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/pci-common.c
+++ linux-2.6/arch/powerpc/kernel/pci-common.c
@@ -1180,7 +1180,7 @@ static void pcibios_allocate_bus_resourc
             pr = (res->flags & IORESOURCE_IO) ?
                 &ioport_resource : &iomem_resource;
         else {
-            pr = pci_find_parent_resource(bus->self, res);
+            pr = pci_find_parent_resource(bus->self->bus, res);
             if (pr == res) {
                 /* this happens when the generic PCI
                  * code (wrongly) decides that this
@@ -1237,7 +1237,7 @@ static inline void alloc_resource(struct
     pr_debug("PCI: Allocating %s: Resource %d: %pR\n",
          pci_name(dev), idx, r);

-    pr = pci_find_parent_resource(dev, r);
+    pr = pci_find_parent_resource(dev->bus, r);
     if (!pr || (pr->flags & IORESOURCE_UNSET) ||
         request_resource(pr, r) < 0) {
         printk(KERN_WARNING "PCI: Cannot allocate resource region %d"
Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -417,16 +417,15 @@ EXPORT_SYMBOL_GPL(pci_find_ht_capability

 /**
  * pci_find_parent_resource - return resource region of parent bus of
given region
- * @dev: PCI device structure contains resources to be searched
+ * @bus: PCI bus structure contains resources to be searched
  * @res: child resource record for which parent is sought
  *
  *  For given resource region of given device, return the resource
  *  region of parent bus the given region is contained in.
  */
-struct resource *pci_find_parent_resource(const struct pci_dev *dev,
+struct resource *pci_find_parent_resource(const struct pci_bus *bus,
                       struct resource *res)
 {
-    const struct pci_bus *bus = dev->bus;
     struct resource *r;
     int i;

Index: linux-2.6/drivers/pci/setup-res.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-res.c
+++ linux-2.6/drivers/pci/setup-res.c
@@ -121,7 +121,7 @@ int pci_claim_resource(struct pci_dev *d
         return -EINVAL;
     }

-    root = pci_find_parent_resource(dev, res);
+    root = pci_find_parent_resource(dev->bus, res);
     if (!root) {
         dev_info(&dev->dev, "can't claim BAR %d %pR: no compatible
bridge window\n",
              resource, res);
@@ -184,7 +184,7 @@ static int pci_revert_fw_address(struct
     res->end = res->start + size - 1;
     res->flags &= ~IORESOURCE_UNSET;

-    root = pci_find_parent_resource(dev, res);
+    root = pci_find_parent_resource(dev->bus, res);
     if (!root) {
         if (res->flags & IORESOURCE_IO)
             root = &ioport_resource;
Index: linux-2.6/drivers/pcmcia/rsrc_nonstatic.c
===================================================================
--- linux-2.6.orig/drivers/pcmcia/rsrc_nonstatic.c
+++ linux-2.6/drivers/pcmcia/rsrc_nonstatic.c
@@ -84,7 +84,7 @@ claim_region(struct pcmcia_socket *s, re
     if (res) {
 #ifdef CONFIG_PCI
         if (s && s->cb_dev)
-            parent = pci_find_parent_resource(s->cb_dev, res);
+            parent = pci_find_parent_resource(s->cb_dev->bus, res);
 #endif
         if (!parent || request_resource(parent, res)) {
             kfree(res);
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -818,7 +818,7 @@ void pci_device_add(struct pci_dev *dev,
 unsigned int pci_scan_child_bus(struct pci_bus *bus);
 void pci_bus_add_device(struct pci_dev *dev);
 void pci_read_bridge_bases(struct pci_bus *child);
-struct resource *pci_find_parent_resource(const struct pci_dev *dev,
+struct resource *pci_find_parent_resource(const struct pci_bus *bus,
                       struct resource *res);
 u8 pci_swizzle_interrupt_pin(const struct pci_dev *dev, u8 pin);
 int pci_get_interrupt_pin(struct pci_dev *dev, struct pci_dev **bridge);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1253330

On Wed, Oct 21, 2015 at 3:32 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Oct 21, 2015 at 11:50 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> Lots of other architectures have both mem32 and mem64 apertures.  You
>> haven't explained what's unique about sparc yet.  Does
>> pci_find_parent_resource() not work on sparc?  If not, is that because the
>> resource tree looks different on sparc than on other architectures?  If so,
>> is that difference something intentional that we want to keep, or should we
>> converge on a single resource tree format across all architectures?
>
> In the new pci_register_legacy_regions(), we only have root bus.
> but current pci_find_parent_resource is taking pci dev.
>
> So will need to change it to take bus instead, please check if you are ok
> with following, then I would update other two.

Never mind, I could just one dummy pci device to work around it.
so don't need to change that.

Thanks

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1253387

[Multipart message — attachments visible in raw view] — view raw

On Wed, Oct 21, 2015 at 3:59 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Oct 21, 2015 at 3:32 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Wed, Oct 21, 2015 at 11:50 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>> Lots of other architectures have both mem32 and mem64 apertures.  You
>>> haven't explained what's unique about sparc yet.  Does
>>> pci_find_parent_resource() not work on sparc?  If not, is that because the
>>> resource tree looks different on sparc than on other architectures?  If so,
>>> is that difference something intentional that we want to keep, or should we
>>> converge on a single resource tree format across all architectures?
>>
>> In the new pci_register_legacy_regions(), we only have root bus.
>> but current pci_find_parent_resource is taking pci dev.
>>
>> So will need to change it to take bus instead, please check if you are ok
>> with following, then I would update other two.
>
> Never mind, I could just one dummy pci device to work around it.
> so don't need to change that.

Khalid,

Please use attached two to replace patch2, patch3 and patch4.

Thanks

Yinghai

[toc] | [prev] | [next] | [standalone]

#1254144

On Wed, Oct 21, 2015 at 6:15 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Oct 21, 2015 at 3:59 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Wed, Oct 21, 2015 at 3:32 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> On Wed, Oct 21, 2015 at 11:50 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>> Lots of other architectures have both mem32 and mem64 apertures.  You
>>>> haven't explained what's unique about sparc yet.  Does
>>>> pci_find_parent_resource() not work on sparc?  If not, is that because the
>>>> resource tree looks different on sparc than on other architectures?  If so,
>>>> is that difference something intentional that we want to keep, or should we
>>>> converge on a single resource tree format across all architectures?
>>>
>>> In the new pci_register_legacy_regions(), we only have root bus.
>>> but current pci_find_parent_resource is taking pci dev.
>>>
>>> So will need to change it to take bus instead, please check if you are ok
>>> with following, then I would update other two.
>>
>> Never mind, I could just one dummy pci device to work around it.
>> so don't need to change that.
>
> Khalid,
>
> Please use attached two to replace patch2, patch3 and patch4.
>
looks like i mix the use root bus resource with parent bus resource ...
so that still have problem.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1253383

From: Yinghai Lu <yinghai@kernel.org>
Date: Wed, 21 Oct 2015 11:16:53 -0700

> otherwise we need to compare res with pbm->mem_space or pbm->mem64_space
> to get direct parent for request_resource_conflict() calling in
> pci_register_legacy_regions().

Right, this is the issue.

On sparc64, the cpu physical base address used for accessing 64-bit
and non-64-bit memory spaces is different.

And that's why the resource values will be different.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1253460

On Wed, 2015-10-21 at 18:27 -0700, David Miller wrote:
> From: Yinghai Lu <yinghai@kernel.org>
> Date: Wed, 21 Oct 2015 11:16:53 -0700
> 
> > otherwise we need to compare res with pbm->mem_space or pbm
> ->mem64_space
> > to get direct parent for request_resource_conflict() calling in
> > pci_register_legacy_regions().
> 
> Right, this is the issue.
> 
> On sparc64, the cpu physical base address used for accessing 64-bit
> and non-64-bit memory spaces is different.
> 
> And that's why the resource values will be different.

This is the same on power btw, we have separate PowerBus windows to the
PHB that get mapped respectively to 32-bit PCI MMIO and 64-bit PCI
MMIO. The former gets "remapped" to generates 0-based PCI cycles, while
the latter is 1:1. So the offset between CPU and PCI changes depending
on which window you hit.

Cheers,
Ben.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1254338

On Thu, Oct 22, 2015 at 02:47:42PM +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2015-10-21 at 18:27 -0700, David Miller wrote:
> > From: Yinghai Lu <yinghai@kernel.org>
> > Date: Wed, 21 Oct 2015 11:16:53 -0700
> > 
> > > otherwise we need to compare res with pbm->mem_space or pbm
> > ->mem64_space
> > > to get direct parent for request_resource_conflict() calling in
> > > pci_register_legacy_regions().
> > 
> > Right, this is the issue.
> > 
> > On sparc64, the cpu physical base address used for accessing 64-bit
> > and non-64-bit memory spaces is different.
> > 
> > And that's why the resource values will be different.
> 
> This is the same on power btw, we have separate PowerBus windows to the
> PHB that get mapped respectively to 32-bit PCI MMIO and 64-bit PCI
> MMIO. The former gets "remapped" to generates 0-based PCI cycles, while
> the latter is 1:1. So the offset between CPU and PCI changes depending
> on which window you hit.

Lots of architectures do this, and the PCI core supports it by keeping
a list of all the host bridge windows.  Each entry has a struct
resource that contains the CPU address, as well as the offset from the
CPU address to the PCI address.  The arch or host bridge driver uses
pci_add_resource_offset() to tell the core about the window and the
offset.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web