Re: [tip:x86/apic] x86, PCI, ACPI: Kill private function resource_to_addr() in arch/x86/pci/acpi.c

[Thomas Gleixner]

On Tue, 9 Dec 2014, Yinghai Lu wrote:

Can you please

1) Cut out the completely irrelevant information from your replies?
   It's just annoying to scroll through hundreds of quoted lines to
   find the guts of the mail.

2) Send patches inline. It's a pain to review and reply and I can't
   use my normal tooling.
 
> This one cause one system with Nehalem and one with Westmere failing.
> 
> [   32.353347] acpi PNP0A08:00: host bridge window expanded to [mem
> 0x00000000-0xffffffff]; [mem 0x000a0000-0x000bffff] ignored
> [   32.362897] acpi PNP0A08:00: host bridge window expanded to [mem
> 0x00000000-0xffffffff]; [mem 0x000d0000-0x000dffff] ignored
> [   32.382862] acpi PNP0A08:00: host bridge window expanded to [mem
> 0x00000000-0xffffffff]; [mem 0x00000000-0xffffffff] ignored
> [   32.402889] acpi PNP0A08:00: host bridge window expanded to [mem
> 0x00000000-0xffffffff]; [??? 0x00000000-0xffffffff flags 0x0] ignored
> [   32.423000] acpi PNP0A08:00: host bridge window expanded to [mem
> 0x00000000-0xffffffff]; [mem 0x00000000-0xffffffff] ignored
> [   32.602921] PCI host bridge to bus 0000:00
> [   32.603158] pci_bus 0000:00: root bus resource [bus 00-3f]
> [   32.622782] pci_bus 0000:00: root bus resource [io  0x0000-0x5fff]
> [   32.642569] pci_bus 0000:00: root bus resource [mem 0x00000000-0xffffffff]
> [   32.642893] pci_bus 0000:00: root bus resource [mem
> 0xfc000000000-0xfc07fffffff pref]
> 
> Looks like the commit have several problems.
> 
> Attached patch should address them.

> -       struct resource r = {
> -               .flags = 0
> -       };
> +       struct resource r;
>  
> +       memset(&r, 0, sizeof(r));

What's the point of this change? Both initialize r to 0. memset()
generates better code, but that's irrelevant for the problem at hand.

And the "fix" is also missing that the address range check happens for
IORESOURCE_IO as well. Which is silly because
acpi_dev_resource_address_space() has that already for the IO
case. But sure, that does not help, because it does not return false,
it sets the IORESOURCE_DISABLED flag and returns true.

Now the code in setup_resource() clears that flag along with all other
flags which does not make any sense, at least not without a comment.

But clearing and therefor ignoring IORESOURCE_DISABLED does not make
any sense at all and is outright wrong.

So there is another interesting flag: IORESOURCE_WINDOW. That's
cleared as well and of course the rest of that setup code does not
handle it either. If IORESOURCE_WINDOW is not set, then this is
address space which is consumed by the bridge itself. So its just
wrong to treat it as window and try coalescing it with the real window
spaces.

Also why is this x86 bridge specific?

     if (addr.resource_type == ACPI_MEMORY_RANGE &&
     	 addr.info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
	    r.flags |= IORESOURCE_PREFETCH;

and not happening in the acpi code? Just because struct resource does
not have a field for it? Sigh.

This needs more than a hacked together fixup, really. It was wrong
before Jiangs change already.

> Please fix it before it get into linus tree.

You can be sure that I'm going to fix the whole mess there proper and
not by applying a cobbled together bandaid.

Thanks,

	tglx






--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/