On Sun, Mar 18, 2012 at 11:42 PM, Yinghai Lu <[email protected]> wrote:
> Set up iobusn_resource tree, and register bus number range to it.
> Later when need to find bus range, will try to allocate from the tree
>
> Need to test on arches other than x86. esp for ia64 and powerpc that support
> ?more than on peer root buses.
>
> The patches need to apply to linux v3.3 + pci-next and
> ? ? ? ?[PATCH -v3] PCI: pci_host_bridge related cleanup
>
> could be found at:
> ? ? ? ?git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-busn-alloc
I started merging this series, but I didn't get very far. I stopped
at the "resources: Add probe_resource()" patch because I don't think
that interface makes sense yet. My work-in-progress is at
git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git on the
topic/yinghai-busn-alloc branch.
I *could* merge that branch, but I don't think they fix anything (that
would come in the later patches), and there are still some issues in
my mind.
- I think we really want a [bus 00-ff] resource for each *domain*,
with each host bridge in the domain requesting part of that range. I
think these patches only track bus number usage within each host
bridge, and I'm not sure there's any place we would detect conflicts
between host bridges.
- I think I made a mistake in the pci_create_root_bus() and
pci_scan_root_bus() interfaces. They currently take a bus number and
a list of resources, and we expect the arch to include the bus number
aperture in the list. The bus number argument should have been
replaced with a bus number aperture resource. The bus number aperture
is essential, while the MMIO/IO aperture list may be empty. And it's
stupid to pass both the starting bus number and a bus resource that
contains the starting bus number. Fixing this would make some of
these patches quite a bit simpler.
- In some of the arches (sparc, powerpc), you added a bus number
resource right next to existing first_busno, last_busno fields. So
now that data lives two places, which will bite us later.
Bjorn
> Yinghai Lu (30):
> ?x86, PCI: Add print all root info for not using _CRS path
> ?x86, PCI: Allocate temp range array in amd_bus pci_root_info probing
> ?x86, PCI: Merge pcibios_scan_root and pci_scan_bus_on_node
> ?PCI: Add busn_res into struct pci_bus.
> ?PCI: Add busn_res operation functions
> ?PCI: Release busn_res when removing bus
> ?PCI: Insert busn_res in pci_create_root_bus()
> ?PCI: Checking busn_res in pci_scan_root_bus()
> ?PCI: Add default busn_resource
> ?PCI: Add default busn_res for pci_scan_bus()
> ?x86, PCI: Add busn_res into resources list for acpi path
> ?x86, PCI: Put busn resource in pci_root_info for not using _CRS path
> ?PCI, ia64: Register busn_res for root buses
> ?PCI, sparc: Register busn_res for root buses
> ?PCI, powerpc: Register busn_res for root buses
> ?PCI, parisc: Register busn_res for root buses
> ?resources: Add probe_resource()
> ?resources: Replace registered resource in tree.
> ?PCI: Add pci_bus_extend/shrink_top()
> ?PCI: Probe safe range that we can use for unassigned bridge.
> ?PCI: Add pci_bus_replace_busn_res()
> ?PCI: Allocate bus range instead of use max blindly
> ?PCI: Strict checking of valid range for bridge
> ?PCI: Kill pci_fixup_parent_subordinate_busnr()
> ?PCI: Seperate child bus scanning to two passes overall
> ?pcmcia: Remove workaround for fixing pci parent bus subordinate
> ?PCI: Double checking setting for bus register and bus struct.
> ?PCI, pciehp: Remove not needed bus number range checking
> ?PCI: More strict checking of valid range for bridge
> ?PCI: Don't shrink too much for hotplug bridge
>
> ?arch/ia64/pci/pci.c ? ? ? ? ? ? ? ? ? | ? ?2 +
> ?arch/powerpc/include/asm/pci-bridge.h | ? ?1 +
> ?arch/powerpc/kernel/pci-common.c ? ? ?| ? 10 +-
> ?arch/sparc/kernel/pci.c ? ? ? ? ? ? ? | ? ?4 +
> ?arch/sparc/kernel/pci_impl.h ? ? ? ? ?| ? ?1 +
> ?arch/x86/pci/acpi.c ? ? ? ? ? ? ? ? ? | ? 11 +-
> ?arch/x86/pci/amd_bus.c ? ? ? ? ? ? ? ?| ? 14 +-
> ?arch/x86/pci/bus_numa.c ? ? ? ? ? ? ? | ? 30 ++-
> ?arch/x86/pci/bus_numa.h ? ? ? ? ? ? ? | ? ?4 +-
> ?arch/x86/pci/common.c ? ? ? ? ? ? ? ? | ? 27 +--
> ?drivers/parisc/dino.c ? ? ? ? ? ? ? ? | ? ?5 +
> ?drivers/parisc/lba_pci.c ? ? ? ? ? ? ?| ? ?3 +
> ?drivers/pci/hotplug/pciehp_pci.c ? ? ?| ? 12 +-
> ?drivers/pci/probe.c ? ? ? ? ? ? ? ? ? | ?439 +++++++++++++++++++++++++++------
> ?drivers/pci/remove.c ? ? ? ? ? ? ? ? ?| ? ?1 +
> ?drivers/pcmcia/yenta_socket.c ? ? ? ? | ? 75 ------
> ?include/linux/ioport.h ? ? ? ? ? ? ? ?| ? ?8 +
> ?include/linux/pci.h ? ? ? ? ? ? ? ? ? | ? ?6 +
> ?kernel/resource.c ? ? ? ? ? ? ? ? ? ? | ?175 +++++++++++++-
> ?19 files changed, 613 insertions(+), 215 deletions(-)
>
> --
> 1.7.7
>
On Wed, May 2, 2012 at 2:22 PM, Bjorn Helgaas <[email protected]> wrote:
> On Sun, Mar 18, 2012 at 11:42 PM, Yinghai Lu <[email protected]> wrote:
>> Set up iobusn_resource tree, and register bus number range to it.
>> Later when need to find bus range, will try to allocate from the tree
>>
>> Need to test on arches other than x86. esp for ia64 and powerpc that support
>> ?more than on peer root buses.
>>
>> The patches need to apply to linux v3.3 + pci-next and
>> ? ? ? ?[PATCH -v3] PCI: pci_host_bridge related cleanup
>>
>> could be found at:
>> ? ? ? ?git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-busn-alloc
>
> I started merging this series, but I didn't get very far. ?I stopped
> at the "resources: Add probe_resource()" patch because I don't think
> that interface makes sense yet. ?My work-in-progress is at
> git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git on the
> topic/yinghai-busn-alloc branch.
>
> I *could* merge that branch, but I don't think they fix anything (that
> would come in the later patches), and there are still some issues in
> my mind.
>
> ?- I think we really want a [bus 00-ff] resource for each *domain*,
> with each host bridge in the domain requesting part of that range. ?I
> think these patches only track bus number usage within each host
> bridge, and I'm not sure there's any place we would detect conflicts
> between host bridges.
I updated the patch set a bit. now it have domain busn_res.
It could detect the conflicts. later could add code to resolve the conflicts.
int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
{
struct resource *res = &b->busn_res;
struct resource *parent_res;
int ret;
res->start = bus;
res->end = bus_max;
res->flags = IORESOURCE_BUS;
if (!pci_is_root_bus(b))
parent_res = &b->parent->busn_res;
else {
parent_res = get_pci_domain_busn_res(pci_domain_nr(b));
res->flags |= IORESOURCE_PCI_FIXED;
}
ret = insert_resource(parent_res, res);
dev_printk(KERN_DEBUG, &b->dev,
"busn_res: %pR %s inserted under %s %pR\n",
res, ret ? "can not be" : "is",
pci_is_root_bus(b) ? "domain":"", parent_res);
return ret;
}
>
> ?- I think I made a mistake in the pci_create_root_bus() and
> pci_scan_root_bus() interfaces. ?They currently take a bus number and
> a list of resources, and we expect the arch to include the bus number
> aperture in the list. ?The bus number argument should have been
> replaced with a bus number aperture resource. ?The bus number aperture
> is essential, while the MMIO/IO aperture list may be empty. ?And it's
> stupid to pass both the starting bus number and a bus resource that
> contains the starting bus number. ?Fixing this would make some of
> these patches quite a bit simpler.
Yes, could try to address that later to make it more clean.
> ?- In some of the arches (sparc, powerpc), you added a bus number
> resource right next to existing first_busno, last_busno fields. ?So
> now that data lives two places, which will bite us later.
then later we should kill first_busno and last_busno in those arches.
Yinghai
On Thu, May 3, 2012 at 3:08 AM, Yinghai Lu <[email protected]> wrote:
> On Wed, May 2, 2012 at 2:22 PM, Bjorn Helgaas <[email protected]> wrote:
>> On Sun, Mar 18, 2012 at 11:42 PM, Yinghai Lu <[email protected]> wrote:
>>> Set up iobusn_resource tree, and register bus number range to it.
>>> Later when need to find bus range, will try to allocate from the tree
>>>
>>> Need to test on arches other than x86. esp for ia64 and powerpc that support
>>> ?more than on peer root buses.
>> ?- I think we really want a [bus 00-ff] resource for each *domain*,
>> with each host bridge in the domain requesting part of that range. ?I
>> think these patches only track bus number usage within each host
>> bridge, and I'm not sure there's any place we would detect conflicts
>> between host bridges.
>
> I updated the patch set a bit. now it have domain busn_res.
I like the domain bus range stuff ("Add busn_res for pci domain").
I'd like it better if we centralized more of the PCI domain support in
the core, e.g., supply the domain directly to pci_scan_root_bus() and
get rid of the arch-specific pci_domain_nr() implementations. But
that can be done later.
> It could detect the conflicts. later could add code to resolve the conflicts.
This is in "Add busn_res operation functions". I think this makes
sense, too. If you look at the branch I started merging
(topic/yinghai-busn-alloc), I tweaked it to print the conflicting
resource when the insertion fails. That information is almost always
useful when debugging, so it'd be nice if you picked that up, too.
>> ?- In some of the arches (sparc, powerpc), you added a bus number
>> resource right next to existing first_busno, last_busno fields. ?So
>> now that data lives two places, which will bite us later.
>
> then later we should kill first_busno and last_busno in those arches.
I want to be sensitive to the arch maintainers by doing our homework,
minimizing the number of patches we ask them to review, and grouping
related changes all at one time. I propose the following, using
powerpc as an example:
- Add "struct resource busn_res" to struct pci_bus.
- powerpc: Replace pci_controller.first_busno/last_busno by "struct
resource busno" everywhere. This will touch many places, but should
be completely mechanical and obvious.
- powerpc: Replace all pci_bus.secondary/subordinate references with
busn_res. This also be only obvious changes.
- powerpc: Add the pci_add_resource() and pci_bus_update_busn_res_end() calls.
- powerpc: Add pci_bus_insert_busn_res() call in
of_scan_pci_bridge(). (This is currently buried in the "Remove
secondary/subordinate in pci_bus" patch, but it doesn't fit there.)
- Replace all non-arch pci_bus.secondary/subordinate references with
busn_res. Obvious changes only.
- Remove secondary/subordinate from struct pci_bus.
- Add pci_bus_insert_busn_res() calls. Again, this is logically
separate from "Remove secondary/subordinate in pci_bus".
I would split up the "Remove secondary/subordinate in pci_bus" patch,
which currently touches 30 files across 11 arches, so an arch
maintainer doesn't have to read the whole patch.
Ben, Dave, feel free to chime in if I'm going astray.
I did a fair amount of work updating changelogs and tweaking the code
in my topic/yinghai-busn-alloc branch. I know I'm currently a
bottleneck here, and it's worse because I'm a bit obsessive. It would
save me time if you picked up those tweaks so I don't have to re-do
them.
You have over 100 patches outstanding, and I'm having a hard time
making forward progress. I think we need to focus on just replacing
secondary/subordinate with busn_res, doing the corresponding
arch-specific replacements, and adding the insert_resource() stuff to
build the resource trees for bus numbers. I'm not going to merge the
probe_resource() stuff, so let's defer that and the resource
assignment stuff for later. Also defer the unrelated
pci_hp_add_bridge() stuff that crept into your for-pci-busn-alloc
branch. If you defer all that stuff and split out the arch stuff,
that should leave us with around 30 patches that deal *only* with
adding busn_res and building the resource trees. I don't think this
will fix any bugs or add any new functionality, but at least it's a
manageable step towards getting the infrastructure in place, so it's
reasonable to consider merging it.
These patches need to appear on the mailing list. In general, people
aren't going to pull your git tree and review them as I've been doing.
But *only* post the stuff mentioned above (busn_res and related
things). I'm not ready to consider the rest yet, so they would only
confuse things.
Bjorn
On Thu, May 3, 2012 at 4:47 PM, Bjorn Helgaas <[email protected]> wrote:
>
>> It could detect the conflicts. later could add code to resolve the conflicts.
>
> This is in "Add busn_res operation functions". ?I think this makes
> sense, too. ?If you look at the branch I started merging
> (topic/yinghai-busn-alloc), I tweaked it to print the conflicting
> resource when the insertion fails. ?That information is almost always
> useful when debugging, so it'd be nice if you picked that up, too.
ok, will check that branch and put that change in.
>
>>> ?- In some of the arches (sparc, powerpc), you added a bus number
>>> resource right next to existing first_busno, last_busno fields. ?So
>>> now that data lives two places, which will bite us later.
>>
>> then later we should kill first_busno and last_busno in those arches.
>
> I want to be sensitive to the arch maintainers by doing our homework,
> minimizing the number of patches we ask them to review, and grouping
> related changes all at one time. ?I propose the following, using
> powerpc as an example:
>
> ?- Add "struct resource busn_res" to struct pci_bus.
> ?- powerpc: Replace pci_controller.first_busno/last_busno by "struct
> resource busno" everywhere. ?This will touch many places, but should
> be completely mechanical and obvious.
> ?- powerpc: Replace all pci_bus.secondary/subordinate references with
> busn_res. ?This also be only obvious changes.
> ?- powerpc: Add the pci_add_resource() and pci_bus_update_busn_res_end() calls.
> ?- powerpc: Add pci_bus_insert_busn_res() call in
> of_scan_pci_bridge(). ?(This is currently buried in the "Remove
> secondary/subordinate in pci_bus" patch, but it doesn't fit there.)
> ?- Replace all non-arch pci_bus.secondary/subordinate references with
> busn_res. ?Obvious changes only.
> ?- Remove secondary/subordinate from struct pci_bus.
> ?- Add pci_bus_insert_busn_res() calls. ?Again, this is logically
> separate from "Remove secondary/subordinate in pci_bus".
>
> I would split up the "Remove secondary/subordinate in pci_bus" patch,
> which currently touches 30 files across 11 arches, so an arch
> maintainer doesn't have to read the whole patch.
could split that a little. not sure if it is worth it.
1. core update subordinate and busn_res.end at same time.
2. update subordinate and busn_res.end at same time
2. remove using secondary/subordinate in arch
3. remove using secondary/suborindate in core
>
> Ben, Dave, feel free to chime in if I'm going astray.
>
> I did a fair amount of work updating changelogs and tweaking the code
> in my topic/yinghai-busn-alloc branch. ?I know I'm currently a
> bottleneck here, and it's worse because I'm a bit obsessive. ?It would
> save me time if you picked up those tweaks so I don't have to re-do
> them.
ok, will pick them up.
>
> You have over 100 patches outstanding, and I'm having a hard time
> making forward progress. ?I think we need to focus on just replacing
> secondary/subordinate with busn_res, doing the corresponding
> arch-specific replacements, and adding the insert_resource() stuff to
> build the resource trees for bus numbers. ?I'm not going to merge the
> probe_resource() stuff, so let's defer that and the resource
> assignment stuff for later. ?Also defer the unrelated
> pci_hp_add_bridge() stuff that crept into your for-pci-busn-alloc
> branch. ?If you defer all that stuff and split out the arch stuff,
> that should leave us with around 30 patches that deal *only* with
> adding busn_res and building the resource trees. ?I don't think this
> will fix any bugs or add any new functionality, but at least it's a
> manageable step towards getting the infrastructure in place, so it's
> reasonable to consider merging it.
let me check if i change to that order.
>
> These patches need to appear on the mailing list. ?In general, people
> aren't going to pull your git tree and review them as I've been doing.
> ?But *only* post the stuff mentioned above (busn_res and related
> things). ?I'm not ready to consider the rest yet, so they would only
> confuse things.
ok.
Yinghai