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

[Yinghai Lu]

On Tue, Oct 20, 2015 at 3:16 PM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
> On Tue, Oct 20, 2015 at 2:57 PM, Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/