On Tue, Nov 19, 2013 at 12:31 PM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
> On Mon, Nov 18, 2013 at 10:36 PM, Guo Chao <yan@xxxxxxxxxxxxxxxxxx> wrote:
>> In resource assignment code, limits are exerted to restrict allocated
>> resource range. However these limits are PCI address but compared to
>> CPU address in the end. Translated them before comparing.
>>
>> We can't just use pcibios_bus_to_resource because the limits may not
>> included in the host bridge window. Introduce a help function to
>> do this translation, if address missed, return an approximate one.
>>
>> Signed-off-by: Guo Chao <yan@xxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/pci/bus.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 63 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
>> index fc1b740..532c0a4 100644
>> --- a/drivers/pci/bus.c
>> +++ b/drivers/pci/bus.c
>> @@ -99,6 +99,64 @@ void pci_bus_remove_resources(struct pci_bus *bus)
>> }
>>
>> /**
>> + * pci_bus_to_resource
>> + *
>> + * Much like pcibios_bus_to_resource() except it takes a single address
>> + * and returns an approximate one if target address is not included
>> + * in the bridge window. The approximate address is smaller than required
>> + * one is 'bound' is 1, larger than required one if 'bound' is 0.
>> + */
>> +static resource_size_t pci_bus_to_resource(struct pci_bus *bus, int flags,
>> + int mask, resource_size_t addr, int bound)
>> +{
>> + struct pci_host_bridge *bridge;
>> + struct pci_host_bridge_window *window, *match = NULL;
>> + resource_size_t max = 0, min = -1;
>> + resource_size_t offset = -1, start, end;
>> +
>> + while (bus->parent)
>> + bus = bus->parent;
>> +
>> + bridge = to_pci_host_bridge(bus->bridge);
>> +
>> + list_for_each_entry(window, &bridge->windows, list) {
>> + if ((flags ^ window->res->flags) & mask)
>> + continue;
>> +
>> + start = window->res->start - window->offset;
>> + end = window->res->end - window->offset;
>> +
>> + if (addr >= start && addr <= end) {
>> + offset = window->offset;
>> + break;
>> + }
>> +
>> + if (bound && addr > end && end > max) {
>> + max = end;
>> + match = window;
>> + } else if (!bound && addr < start && start < min) {
>> + min = start;
>> + match = window;
>> + }
>> + }
>> +
>> + if (offset == -1) {
>> + /*
>> + * Not even found the matched type. This may happen,
>> + * for example, if try to translate IO address in a HB
>> + * without IO window. Just return the original address,
>> + * it will fail later anyway.
>> + */
>> + if (match == NULL)
>> + return addr;
>> +
>> + return (bound ? max : min) + match->offset;
>> + }
>> +
>> + return addr + offset;
>> +}
>
> that is confusing.
I agree, this is way too confusing.
I certainly agree that the current code is wrong -- PCIBIOS_MAX_MEM_32
is a limit on the *bus* address, but we're using it to limit CPU
addresses. And it's not arch-dependent at all; it's strictly a
function of PCI, so I think you should fix that, too. Your [2/3]
patch is a start, but I don't think there's any reason to allow an
arch to override it, and it's only used in one place, so it doesn't
need to be in include/linux/pci.h where it's effectively exported to
the world.
It's completely bogus to try to compute "max" outside the loop because
there's no requirement that the translation offset be the same for all
the resources. But *inside* the loop, you have a valid struct
resource, and you can easily compute the corresponding bus addresses
and determine whether they fit in 32 bits.
Bjorn
--
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
