Re: [patch 2/2] PNP: don't check disabled PCI BARs for conflicts in quirk_system_pci_resources()

Rene Herman
2008-09-29T18:27:16Z

On 29-09-08 18:34, Linus Torvalds wrote:

> On Mon, 29 Sep 2008, Bjorn Helgaas wrote:
>>
>> + if (!pci_resource_enabled(pdev, i))
>> + continue;
>
> I really don't think this is the right approach.
>
> Maybe the PCI device has been turned off, but the *resource* may still be
> valid.
>
> Wouldn't it be much better to just check whether the resource is inserted
> in the resource tree or not?
>
> Quite frankly, it looks like your change will basically cause us to look
> over *every* system PnP resource, and for each of them, it will look at
> *every* PCI device, and for each PCI device it will look at *every* BAR,
> and for each BAR it finds it will read the PCI status register, over and
> over and over again.
>
> Now, I doubt you'll be able to wear out the PCI bus, but doesn't this just
> make you go "umm, that's not pretty, and it doesn't make much sense".
>
> If we've detected the PCI resource as being valid by the PCI layer, why
> not just use that information? And afaik, the easy way to check that is
> just whether it's inserted into the resource tree, which in turn is most
> trivially done by just checking whether the resource has a parent.
>
> IOW, why isn't it just doing
>
> struct resource *res = dev->resource[bar];
>
> if (!res->parent)
> continue;
>
> or something? Or what was wrong with just checking the res->start for
> being zero? Wherever PnP is relevant, resources that start at zero are
> disabled, no?

I believe the possible issue is that resources that do _not_ (seem to)
start at zero might also be disabled.

Bjorn commented that pci_resource_start() returns a CPU address for I/O
which might not be the actual I/O address on some platforms. I haven't a
clue if that's actually possible "wherever PnP is relevent" as you put
it but that seems to otherwise make sense.

If it does though, it might for all I know also be possible to check
against some ARCH_SPECIFIC_INVALID_IO_ADDRESS instead of plain unadorned
0 (or just recheck the actual BAR again if not stored anywhere).

But that's the issue as I understood it: we might miss them on some
platforms if checking against 0...

Rene.