On Mon, Jul 22, 2013 at 7:18 PM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote:
> On Mon, Jul 22, 2013 at 6:08 PM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote:
>> On Wed, Jul 3, 2013 at 8:04 AM, Mika Westerberg
>> <mika.westerberg@xxxxxxxxxxxxxxx> wrote:
>>> In hotplug case (especially with Thunderbolt enabled systems) we might need
>>> to call pcibios_resource_survey_bus() several times for a bus. The function
>>> ends up calling pci_claim_resource() for each bridge resource that then
>>> fails claiming that the resource exists already (which it does). Once this
>>> happens the resource is invalidated thus preventing devices behind the
>>> bridge to allocate their resources.
>>>
>>> To fix this we do what has been done in pcibios_allocate_dev_resources()
>>> and check 'parent' of the given resource. If it is non-NULL it means that
>>> the resource has been allocated already and we can skip it. We do the same
>>> for ROM resources as well.
>>>
>>> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
>>
>> This looks reasonable to me.
>
> Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>
>> However, the use of "r->parent" to determine whether the resource is
>> already allocated is nothing specific to x86. So I think we might be
>> missing an opportunity to make this more consistent across
>> architectures. I looked at other callers of pci_claim_resource() for
>> bridge and ROM resources, and it looks like we might be able to make
>> similar changes in:
>>
>> frv pcibios_allocate_bus_resources()
>> ia64 pcibios_fixup_resources() (via pcibios_fixup_bridge_resources())
>> mn10300 pcibios_allocate_bus_resources()
>> mn10300 pcibios_assign_resources() (ROM)
>> mn10300 pcibios_fixup_device_resources()
>> parisc lba_fixup_bus()
>>
>> I really hate the idea of fixing something for x86 but not for other
>> arches, so maybe somebody could take a deeper look at this and see if
>> it would make sense to change the other arches, too.
I misspoke here. The change below helps fix an issue on x86. It's
only an issue on x86 because only x86 has acpiphp and
pcibios_resource_survey_bus(). Other arches *do* call
pci_claim_resource(), but making similar changes on other arches does
not fix similar issues there, so it's not really a matter of "fixing
only x86."
My motivation is to move toward unification of how we claim resources.
There's nothing inherently arch-specific about calling
pci_claim_resource(), but arches use a variety of tests involving
"r->flags", "r->parent", and "r->start" to decide whether to do so.
Ideally we would call pci_claim_resource() from the core, not from
arch code, and we would use a set of tests that work for all arches
and situations.
We can always do the minimum of changing only what's absolutely
required, but the result is divergence and increased maintenance work
in the long term. I'm trying to counter that a little bit by
encouraging people to consider refactorings that fix the current issue
while reducing maintenance effort.
Bjorn
>>> ---
>>> arch/x86/pci/i386.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
>>> index 94919e3..db6b1ab 100644
>>> --- a/arch/x86/pci/i386.c
>>> +++ b/arch/x86/pci/i386.c
>>> @@ -210,6 +210,8 @@ static void pcibios_allocate_bridge_resources(struct pci_dev *dev)
>>> r = &dev->resource[idx];
>>> if (!r->flags)
>>> continue;
>>> + if (r->parent) /* Already allocated */
>>> + continue;
>>> if (!r->start || pci_claim_resource(dev, idx) < 0) {
>>> /*
>>> * Something is wrong with the region.
>>> @@ -318,6 +320,8 @@ static void pcibios_allocate_dev_rom_resource(struct pci_dev *dev)
>>> r = &dev->resource[PCI_ROM_RESOURCE];
>>> if (!r->flags || !r->start)
>>> return;
>>> + if (r->parent) /* Already allocated */
>>> + return;
>>>
>>> if (pci_claim_resource(dev, PCI_ROM_RESOURCE) < 0) {
>>> r->end -= r->start;
>>> --
>>> 1.8.3.2
>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
