On Wed, Apr 16, 2014 at 3:11 PM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote:
>
> I spent quite a bit of time trying to understand this code yesterday, but
> I failed. Comments might help, but more importantly, it needs to be
> simplified so the code makes sense even without comments.
will add some comments.
>> + b_res = &dev->resource[PCI_BRIDGE_RESOURCES];
>> +
>> + /*
>> + * 1. if there is io port assign fail, will release bridge
>> + * io port.
>> + * 2. if there is non pref mmio assign fail, release bridge
>> + * nonpref mmio.
>> + * 3. if there is 64bit pref mmio assign fail, and bridge pref
>> + * is 64bit, release bridge pref mmio.
>> + * 4. if there is pref mmio assign fail, and bridge pref is
>> + * 32bit mmio, release bridge pref mmio
>> + * 5. if there is pref mmio assign fail, and bridge pref is not
>> + * assigned, release bridge nonpref mmio.
>> + */
>> + if (type & IORESOURCE_IO)
>> + idx = 0;
>> + else if (!(type & IORESOURCE_PREFETCH))
>> + idx = 1;
>> + else if ((type & IORESOURCE_MEM_64) &&
>> + (b_res[2].flags & IORESOURCE_MEM_64))
>> + idx = 2;
>> + else if (!(b_res[2].flags & IORESOURCE_MEM_64) &&
>> + (b_res[2].flags & IORESOURCE_PREFETCH))
>> + idx = 2;
>> + else
>> + idx = 1;
>
> This is pretty hard to figure out. Is there any way we can just pass in
> the bridge window identifier directly, e.g., 0 == I/O window, 1 ==
> non-prefetchable MEM window, 2 == prefetchable window? It seems like way
> up in pci_assign_unassigned_root_bus_resources(), we ought to know which
> window failed, and we could just pass that in somehow instead of having to
> derive it here.
that is for find out which up-level could be scratched...
and do not know exact which one should be fit as twisted mmio-pref under mmio.
>>
>> - if (ret < 0 && (res->flags & IORESOURCE_PREFETCH)) {
>> + if (ret < 0 &&
>> + (res->flags & (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) ==
>> + (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) {
>> + /*
>> + * That failed.
>> + *
>> + * Try below 4g pref
>> + */
>> + ret = pci_bus_alloc_resource(bus, res, size, align, min,
>> + IORESOURCE_PREFETCH,
>> + pcibios_align_resource, dev);
>> + }
>
> pci_bus_alloc_from_region() already has a mechanism for restricting
> allocation to certain bus addresses. I don't like this second mechanism of
> also using IORESOURCE_MEM_64. There's too much implicit state -- I think
> there's a dependency here on how we manage the IORESOURCE_MEM_64 bit in the
> bridge window resources. That makes this code hard to understand.
>
will drop that.
Yinghai
--
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
