Re: [PATCH 06/24] PCI: Add busn_res tracking in core

[Jesse Barnes <jbarnes@virtuousgeek.org>]

[-- Attachment #1: Type: text/plain, Size: 6274 bytes --]

On Wed, 8 Feb 2012 09:26:52 -0800
Yinghai Lu <yinghai@kernel.org> wrote:

> On Wed, Feb 8, 2012 at 8:08 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > On Sat, Feb 4, 2012 at 10:57 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> >> update pci_scan_root_bus, and pci_scan_bus to insert root bus busn into
> >> iobusn_resource tree.
> >>
> >> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >> ---
> >>  drivers/pci/probe.c  |   30 ++++++++++++++++++++++++++----
> >>  drivers/pci/remove.c |    1 +
> >>  include/linux/pci.h  |    4 ++++
> >>  3 files changed, 31 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> >> index fabf9d0..cdd6e83 100644
> >> --- a/drivers/pci/probe.c
> >> +++ b/drivers/pci/probe.c
> >> @@ -1667,8 +1667,9 @@ void pci_bus_release_busn_res(struct pci_bus *b)
> >>                        res, ret ? "can not be" : "is");
> >>  }
> >>
> >> -struct pci_bus * __devinit pci_scan_root_bus(struct device *parent, int bus,
> >> -               struct pci_ops *ops, void *sysdata, struct list_head *resources)
> >> +struct pci_bus * __devinit pci_scan_root_bus_max(struct device *parent, int bus,
> >> +               int bus_max, struct pci_ops *ops, void *sysdata,
> >> +               struct list_head *resources)
> >>  {
> >>        struct pci_bus *b;
> >>
> >> @@ -1676,10 +1677,26 @@ struct pci_bus * __devinit pci_scan_root_bus(struct device *parent, int bus,
> >>        if (!b)
> >>                return NULL;
> >>
> >> +       pci_bus_insert_busn_res(b, bus, bus_max);
> >>        b->subordinate = pci_scan_child_bus(b);
> >>        pci_bus_add_devices(b);
> >>        return b;
> >>  }
> >> +EXPORT_SYMBOL(pci_scan_root_bus_max);
> >> +
> >> +struct pci_bus * __devinit pci_scan_root_bus(struct device *parent, int bus,
> >> +               struct pci_ops *ops, void *sysdata,
> >> +               struct list_head *resources)
> >> +{
> >> +       struct pci_bus *b;
> >> +
> >> +       b = pci_scan_root_bus_max(parent, bus, 255, ops, sysdata, resources);
> >> +
> >> +       if (b)
> >> +               pci_bus_update_busn_res_end(b, b->subordinate);
> >> +
> >> +       return b;
> >> +}
> >>  EXPORT_SYMBOL(pci_scan_root_bus);
> >>
> >>  /* Deprecated; use pci_scan_root_bus() instead */
> >> @@ -1692,9 +1709,11 @@ struct pci_bus * __devinit pci_scan_bus_parented(struct device *parent,
> >>        pci_add_resource(&resources, &ioport_resource);
> >>        pci_add_resource(&resources, &iomem_resource);
> >>        b = pci_create_root_bus(parent, bus, ops, sysdata, &resources);
> >> -       if (b)
> >> +       if (b) {
> >> +               pci_bus_insert_busn_res(b, bus, 255);
> >>                b->subordinate = pci_scan_child_bus(b);
> >> -       else
> >> +               pci_bus_update_busn_res_end(b, b->subordinate);
> >> +       } else
> >>                pci_free_resource_list(&resources);
> >>        return b;
> >>  }
> >> @@ -1710,7 +1729,10 @@ struct pci_bus * __devinit pci_scan_bus(int bus, struct pci_ops *ops,
> >>        pci_add_resource(&resources, &iomem_resource);
> >>        b = pci_create_root_bus(NULL, bus, ops, sysdata, &resources);
> >>        if (b) {
> >> +               pci_bus_insert_busn_res(b, bus, 255);
> >>                b->subordinate = pci_scan_child_bus(b);
> >> +               pci_bus_update_busn_res_end(b, b->subordinate);
> >> +
> >>                pci_bus_add_devices(b);
> >>        } else {
> >>                pci_free_resource_list(&resources);
> >> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> >> index 82f8ae5..5b679d2 100644
> >> --- a/drivers/pci/remove.c
> >> +++ b/drivers/pci/remove.c
> >> @@ -68,6 +68,7 @@ void pci_remove_bus(struct pci_bus *pci_bus)
> >>
> >>        down_write(&pci_bus_sem);
> >>        list_del(&pci_bus->node);
> >> +       pci_bus_release_busn_res(pci_bus);
> >>        up_write(&pci_bus_sem);
> >>        if (!pci_bus->is_added)
> >>                return;
> >> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >> index 3da935c..d5b6786 100644
> >> --- a/include/linux/pci.h
> >> +++ b/include/linux/pci.h
> >> @@ -668,6 +668,10 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> >>  void pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
> >>  void pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
> >>  void pci_bus_release_busn_res(struct pci_bus *b);
> >> +struct pci_bus * __devinit pci_scan_root_bus_max(struct device *parent, int bus,
> >> +                                            int busmax, struct pci_ops *ops,
> >> +                                            void *sysdata,
> >> +                                            struct list_head *resources);
> >>  struct pci_bus * __devinit pci_scan_root_bus(struct device *parent, int bus,
> >>                                             struct pci_ops *ops, void *sysdata,
> >>                                             struct list_head *resources);
> >
> > Now we have both pci_bus_insert_busn_res() and
> > pci_scan_root_bus_max().  Why do we need both?  I think it's too much
> > of a burden on architectures to expect them to understand both.
> >
> > Maybe some sample pseudo-code using both interfaces would help me
> > understand what you expect a typical architecture to do.
> 
> We need to call pci_bus_insert_busn_res for root bus:
> 1. after that bus is allocated (...) : that busn_res is in the bus struct.
> 2. before pci_scan_child_bus

I agree with Bjorn; at the very least this has a really bad name.  How
are people supposed to know which function to call?  If it's just for
the primary scan at init, then it should be named as such.  But ideally
it would be inside the existing scan_root_bus...

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]