x86,pci,acpi: Handle invalid _CRS
in [Kernel]
|
Prev: [PATCHv7 2.6.34-rc5 3/5] mx5: Enable board specific functions for enabling USB host on Babbage
Next: ksm: check for ERR_PTR from follow_page()
From: Bjorn Helgaas on 21 Apr 2010 11:30 On Tuesday 20 April 2010 11:35:59 pm Yinghai wrote: > > Don't clear root bus resource too early, until We can make sure _CRS works Please observe English conventions like capitalizing the first word of a sentence (and not things like "We" in the middle) and using a period at the end. > also restore it, if all _CRS get rejected because of conflicts > > Signed-off-by: Yinghai Lu <yinghai(a)kernel.org> > > --- > arch/x86/pci/acpi.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > Index: linux-2.6/arch/x86/pci/acpi.c > =================================================================== > --- linux-2.6.orig/arch/x86/pci/acpi.c > +++ linux-2.6/arch/x86/pci/acpi.c > @@ -196,9 +196,6 @@ get_current_resources(struct acpi_device > struct pci_root_info info; > size_t size; > > - if (pci_use_crs) > - pci_bus_remove_resources(bus); > - > info.bridge = device; > info.bus = bus; > info.res_num = 0; > @@ -217,10 +214,20 @@ get_current_resources(struct acpi_device > goto name_alloc_fail; > sprintf(info.name, "PCI Bus %04x:%02x", domain, busnum); > > + /* Only clear that when _CRS works for sure*/ If you put a comment here, please at least add a space before the closing "*/". Otherwise it looks sloppy. > + if (pci_use_crs) > + pci_bus_remove_resources(bus); > + > info.res_num = 0; > acpi_walk_resources(device->handle, METHOD_NAME__CRS, setup_resource, > &info); > > + if (pci_use_crs && !info.res_num) { > + /* Restore default one */ > + bus->resource[0] = &ioport_resource; > + bus->resource[1] = &iomem_resource; This is ugly because it just repeats this code from pci_create_bus(), and there's no indication either here or there that they are connected. Admittedly, I think it's also sort of ugly that pci_bus_remove_resources() exists at all -- I'd rather have some sort of hook so we could set the bus resources correctly the first time. Maybe you could at least add a pci_bus_set_default_resources() that could be called both here and from pci_create_bus(). Why are you doing this patch? Did you see a machine where the host bridge was left with no resources because of _CRS issues? If so, this patch feels like a band-aid. I'd rather investigate the issue directly, because that would probably be a Linux problem we could fix. Also, if there *is* a reported problem, you should include a link to the bugzilla or email thread. Bjorn > + } > + > return; > > name_alloc_fail: > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Yinghai Lu on 21 Apr 2010 12:50 On 04/21/2010 08:21 AM, Bjorn Helgaas wrote: > > >> + if (pci_use_crs) >> + pci_bus_remove_resources(bus); >> + >> info.res_num = 0; >> acpi_walk_resources(device->handle, METHOD_NAME__CRS, setup_resource, >> &info); >> >> + if (pci_use_crs && !info.res_num) { >> + /* Restore default one */ >> + bus->resource[0] = &ioport_resource; >> + bus->resource[1] = &iomem_resource; >> > This is ugly because it just repeats this code from pci_create_bus(), > and there's no indication either here or there that they are connected. > > Admittedly, I think it's also sort of ugly that pci_bus_remove_resources() > exists at all -- I'd rather have some sort of hook so we could set the > bus resources correctly the first time. > sure. that would be better. Can you have patch for that. > Maybe you could at least add a pci_bus_set_default_resources() that > could be called both here and from pci_create_bus(). > good idea. > Why are you doing this patch? Did you see a machine where the host > bridge was left with no resources because of _CRS issues? If so, > this patch feels like a band-aid. I'd rather investigate the issue > directly, because that would probably be a Linux problem we could fix. > > Also, if there *is* a reported problem, you should include a link to > the bugzilla or email thread. > No, just code review. YH -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Bjorn Helgaas on 21 Apr 2010 13:00 On Wednesday 21 April 2010 10:45:46 am Yinghai Lu wrote: > On 04/21/2010 08:21 AM, Bjorn Helgaas wrote: > > Why are you doing this patch? Did you see a machine where the host > > bridge was left with no resources because of _CRS issues? If so, > > this patch feels like a band-aid. I'd rather investigate the issue > > directly, because that would probably be a Linux problem we could fix. > > > > Also, if there *is* a reported problem, you should include a link to > > the bugzilla or email thread. > > > No, just code review. In that case, I don't think this patch is a good idea. Assume we have a Linux problem with _CRS parsing, and as a result, we don't find any host bridge resources. This patch will cause us to silently revert to the default resources, so we lose the opportunity to debug and fix the Linux problem. Even worse, if there's a *BIOS* problem with _CRS, this patch will hide it and make it harder to diagnose. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: H. Peter Anvin on 21 Apr 2010 18:40 On 04/21/2010 09:59 AM, Bjorn Helgaas wrote: > On Wednesday 21 April 2010 10:45:46 am Yinghai Lu wrote: >> On 04/21/2010 08:21 AM, Bjorn Helgaas wrote: > >>> Why are you doing this patch? Did you see a machine where the host >>> bridge was left with no resources because of _CRS issues? If so, >>> this patch feels like a band-aid. I'd rather investigate the issue >>> directly, because that would probably be a Linux problem we could fix. >>> >>> Also, if there *is* a reported problem, you should include a link to >>> the bugzilla or email thread. >>> >> No, just code review. > > In that case, I don't think this patch is a good idea. Assume we have > a Linux problem with _CRS parsing, and as a result, we don't find any > host bridge resources. This patch will cause us to silently revert to > the default resources, so we lose the opportunity to debug and fix the > Linux problem. > > Even worse, if there's a *BIOS* problem with _CRS, this patch will hide > it and make it harder to diagnose. > > Bjorn Hi Bjorn, Do you have opinions on patches 1-2 of the series? I'm getting concerned about how the size of the patchset has grown, and we're past -rc5 already... but it is a regression so we can't just defer it to .35. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: H. Peter Anvin on 21 Apr 2010 20:10
On 04/21/2010 04:43 PM, Yinghai wrote: > > then use -v3 please > > -v4: also don't trim [0xa0000, 0x100000] for mrst. > That makes a lot of sense for 2.6.34. I agree with moving the trimming into subsystems, but I think it's .35 material at this point. [Cc: Jacob Pan] >> >> The use of a string match in: >> >> + if (check_child && !conflict->child && strstr(conflict->name, >> "PCI Bus")) >> ^^^^^^^^^ >> >> ... screams "wrong! ugly! bad!" in my opinion. I utterly fail to see >> how that could be acceptable under any circumstances. I thought that >> had been flagged earlier in the conversation, but it is apparently still >> there. > > the string checking is to make sure pci device that is hooked into bus0 directly, but pci bar is falling into > 0xa0000 - 0x100000. So can not put "reserved" holder under them. > It makes me extremely concerned, because such hacks tend to be extremely vulnerable. Strings are designed primarily for human consumption, and "find string inside another string" is *very* much so. As such, I really would like to understand that there isn't any more sensible way, such as a flag, that can be used to accomplish the objective. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |