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

[Bjorn Helgaas]

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@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().

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