please check this one
[PATCH] x86/pci: fix boundary checking for root res
don't touch info->res_num if we are out of space
Signed-off-by: Yinghai Lu <[email protected]>
---
arch/x86/pci/acpi.c | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 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
@@ -68,6 +68,10 @@ setup_resource(struct acpi_resource *acp
unsigned long flags;
struct resource *root;
int max_root_bus_resources = PCI_BUS_NUM_RESOURCES;
+ u64 start, end;
+
+ if (bus_has_transparent_bridge(info->bus))
+ max_root_bus_resources -= 3;
status = resource_to_addr(acpi_res, &addr);
if (!ACPI_SUCCESS(status))
@@ -84,25 +88,24 @@ setup_resource(struct acpi_resource *acp
} else
return AE_OK;
- res = &info->res[info->res_num];
- res->name = info->name;
- res->flags = flags;
- res->start = addr.minimum + addr.translation_offset;
- res->end = res->start + addr.address_length - 1;
- res->child = NULL;
-
- if (bus_has_transparent_bridge(info->bus))
- max_root_bus_resources -= 3;
+ start = addr.minimum + addr.translation_offset;
+ end = start + addr.address_length - 1;
if (info->res_num >= max_root_bus_resources) {
printk(KERN_WARNING "PCI: Failed to allocate 0x%lx-0x%lx "
"from %s for %s due to _CRS returning more than "
- "%d resource descriptors\n", (unsigned long) res->start,
- (unsigned long) res->end, root->name, info->name,
+ "%d resource descriptors\n", (unsigned long) start,
+ (unsigned long) end, root->name, info->name,
max_root_bus_resources);
- info->res_num++;
return AE_OK;
}
+ res = &info->res[info->res_num];
+ res->name = info->name;
+ res->flags = flags;
+ res->start = start;
+ res->end = end;
+ res->child = NULL;
+
if (insert_resource(root, res)) {
printk(KERN_ERR "PCI: Failed to allocate 0x%lx-0x%lx "
"from %s for %s\n", (unsigned long) res->start,
so we could remove adjust_transparent_bridge_resources..
and give x86_pci_root_bus_res_quirks chance when _CRS is not used or
not there.
v2: add print out if pci conf reading for res is used for root
Signed-off-by: Yinghai Lu <[email protected]>
---
arch/x86/pci/acpi.c | 32 +++++++++-----------------------
arch/x86/pci/amd_bus.c | 8 ++++++--
2 files changed, 15 insertions(+), 25 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
@@ -118,23 +118,6 @@ setup_resource(struct acpi_resource *acp
}
static void
-adjust_transparent_bridge_resources(struct pci_bus *bus)
-{
- struct pci_dev *dev;
-
- list_for_each_entry(dev, &bus->devices, bus_list) {
- int i;
- u16 class = dev->class >> 8;
-
- if (class == PCI_CLASS_BRIDGE_PCI && dev->transparent) {
- for(i = 3; i < PCI_BUS_NUM_RESOURCES; i++)
- dev->subordinate->resource[i] =
- dev->bus->resource[i - 3];
- }
- }
-}
-
-static void
get_current_resources(struct acpi_device *device, int busnum,
int domain, struct pci_bus *bus)
{
@@ -161,8 +144,6 @@ get_current_resources(struct acpi_device
info.res_num = 0;
acpi_walk_resources(device->handle, METHOD_NAME__CRS, setup_resource,
&info);
- if (info.res_num)
- adjust_transparent_bridge_resources(bus);
return;
@@ -225,8 +206,15 @@ struct pci_bus * __devinit pci_acpi_scan
*/
memcpy(bus->sysdata, sd, sizeof(*sd));
kfree(sd);
- } else
- bus = pci_scan_bus_parented(NULL, busnum, &pci_root_ops, sd);
+ } else {
+ bus = pci_create_bus(NULL, busnum, &pci_root_ops, sd);
+ if (bus) {
+ if (!(pci_probe & PCI_NO_ROOT_CRS))
+ get_current_resources(device, busnum, domain,
+ bus);
+ bus->subordinate = pci_scan_child_bus(bus);
+ }
+ }
if (!bus)
kfree(sd);
@@ -241,8 +229,6 @@ struct pci_bus * __devinit pci_acpi_scan
#endif
}
- if (bus && !(pci_probe & PCI_NO_ROOT_CRS))
- get_current_resources(device, busnum, domain, bus);
return bus;
}
Index: linux-2.6/arch/x86/pci/amd_bus.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/amd_bus.c
+++ linux-2.6/arch/x86/pci/amd_bus.c
@@ -100,8 +100,9 @@ void x86_pci_root_bus_res_quirks(struct
int j;
struct pci_root_info *info;
- /* don't go for it if _CRS is used */
- if (!(pci_probe & PCI_NO_ROOT_CRS))
+ /* don't go for it if _CRS is used already */
+ if (b->resource[0] != &ioport_resource ||
+ b->resource[1] != &iomem_resource)
return;
/* if only one root bus, don't need to anything */
@@ -116,6 +117,9 @@ void x86_pci_root_bus_res_quirks(struct
if (i == pci_root_num)
return;
+ printk(KERN_DEBUG "PCI: peer root bus %02x res updated from pci conf\n",
+ b->number);
+
info = &pci_root_info[i];
for (j = 0; j < info->res_num; j++) {
struct resource *res;
On Wed, 24 Jun 2009 14:48:25 -0700
Yinghai Lu <[email protected]> wrote:
>
> so we could remove adjust_transparent_bridge_resources..
>
> and give x86_pci_root_bus_res_quirks chance when _CRS is not used or
> not there.
>
> v2: add print out if pci conf reading for res is used for root
>
> Signed-off-by: Yinghai Lu <[email protected]>
Do you think this is generally safe? I was hoping we could do
something more complete, like making the code handle arbitrary numbers
of resources in _CRS. Hacking that together now.
--
Jesse Barnes, Intel Open Source Technology Center
for AMD system, when only one PCI root, just set PCI_NO_ROOT_CRS for it
Signed-off-by: Yinghai Lu <[email protected]>
---
arch/x86/pci/amd_bus.c | 4 ++++
1 file changed, 4 insertions(+)
Index: linux-2.6/arch/x86/pci/amd_bus.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/amd_bus.c
+++ linux-2.6/arch/x86/pci/amd_bus.c
@@ -561,6 +561,10 @@ static int __init early_fill_mp_bus_info
}
}
+ /* don't use _CRS if we only have one root */
+ if (pci_root_num <= 1)
+ pci_probe |= PCI_NO_ROOT_CRS;
+
return 0;
}
Should we not just revert 9e9f46c44e487af0a82eb61b624553e2f7118f5b?
The thing says:
"At this point, it seems to solve more problems than it causes, so let's
try using it by default. It's an easy revert if it ends up causing
trouble."
and it clearly does _not_ solve more problems than it causes, and the
whole message in that commit implies we should revert it.
I'm happy to apply various patches to fix it up, but regardless, I thinkwe
should revert that commit as bogus. We can try making it the default again
next round, when maybe it will be true that it doesn't cause issues.
What did it even ever help with?
Linus
On Wed, 24 Jun 2009, Yinghai Lu wrote:
>
> for AMD system, when only one PCI root, just set PCI_NO_ROOT_CRS for it
>
> Signed-off-by: Yinghai Lu <[email protected]>
>
> ---
> arch/x86/pci/amd_bus.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> Index: linux-2.6/arch/x86/pci/amd_bus.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/pci/amd_bus.c
> +++ linux-2.6/arch/x86/pci/amd_bus.c
> @@ -561,6 +561,10 @@ static int __init early_fill_mp_bus_info
> }
> }
>
> + /* don't use _CRS if we only have one root */
> + if (pci_root_num <= 1)
> + pci_probe |= PCI_NO_ROOT_CRS;
> +
> return 0;
> }
>
>
On Wed, 24 Jun 2009, Linus Torvalds wrote:
>
> I'm happy to apply various patches to fix it up, but regardless, I thinkwe
> should revert that commit as bogus. We can try making it the default again
> next round, when maybe it will be true that it doesn't cause issues.
Btw, I really think our _CRS handling sucks.
There's two things that you can do with _CRS:
- use the _existence_ of it as an indicator of a root bus
- try to use it to populate the resource tree.
And quite frankly, I think #2 is broken. There's no way in hell that ACPI
tables are ever going to be better than just asking the hardware. We've
gone through this before. Trusting ACPI over the hardware is just
FUNDAMENTALLY WRONG.
So I'm just going to do that revert. I'm not sure if it ever makes sense
to make that insane _CRS code the default. It seems like a fundamentally
flawed idea.
Linus
On Wed, 24 Jun 2009 16:21:09 -0700 (PDT)
Linus Torvalds <[email protected]> wrote:
>
>
> On Wed, 24 Jun 2009, Linus Torvalds wrote:
> >
> > I'm happy to apply various patches to fix it up, but regardless, I
> > thinkwe should revert that commit as bogus. We can try making it
> > the default again next round, when maybe it will be true that it
> > doesn't cause issues.
>
> Btw, I really think our _CRS handling sucks.
>
> There's two things that you can do with _CRS:
>
> - use the _existence_ of it as an indicator of a root bus
>
> - try to use it to populate the resource tree.
>
> And quite frankly, I think #2 is broken. There's no way in hell that
> ACPI tables are ever going to be better than just asking the
> hardware. We've gone through this before. Trusting ACPI over the
> hardware is just FUNDAMENTALLY WRONG.
>
> So I'm just going to do that revert. I'm not sure if it ever makes
> sense to make that insane _CRS code the default. It seems like a
> fundamentally flawed idea.
Yeah, I think it's reasonable to revert, especially given how we do
_CRS handling currently. I'm hoping at some point we can use the _CRS
data to at least augment the configuration we get from hardware, since
on some machines it seems to be necessary.
--
Jesse Barnes, Intel Open Source Technology Center
On Wed, 24 Jun 2009, Jesse Barnes wrote:
>
> Yeah, I think it's reasonable to revert, especially given how we do
> _CRS handling currently. I'm hoping at some point we can use the _CRS
> data to at least augment the configuration we get from hardware, since
> on some machines it seems to be necessary.
Agreed. I do think we should take _CRS into account - possibly just as a
minimal hint to determine which root buses to try to scan (maybe we do
this already, I really didn't check). Or maybe we could use it to extend
on our scan information.
But when it seems to have things like "this bus can forward VGA cycles"
kind of "resources" (which seems to be the main reason Larry Finger has so
many of them), then that's just worthless knowledge that we're much better
off just determining on our own.
Anyway, I may feel pretty strongly about things like this, but I'm also
open to being convinced otherwise for 2.6.32. I wanted to do -rc1 today
(it's been more than two weeks), and while I don't expect -rc1 to be
flawless, I also hate to release it with _known_ bugs.
So partly due to timing, I'd rather revert it, and we can revisit it for
the next release - whatever the actual end result then will be.
[ There's a difference between "we're supposed to find and fix bugs in the
-rc series", and "I release known-buggy -rc1's since we're supposed to
fix it later". For similar reasons, I hate pulling known-buggy stuff
during the merge window - it's ok if it shows itself to be buggy
_later_, but if people send me stuff that they know is buggy as they
send it to me, then that's a problem. ]
Linus
On Wed, Jun 24, 2009 at 04:09:36PM -0700, Linus Torvalds wrote:
>
> Should we not just revert 9e9f46c44e487af0a82eb61b624553e2f7118f5b?
>
> The thing says:
>
> "At this point, it seems to solve more problems than it causes, so let's
> try using it by default. It's an easy revert if it ends up causing
> trouble."
>
> and it clearly does _not_ solve more problems than it causes, and the
> whole message in that commit implies we should revert it.
>
> I'm happy to apply various patches to fix it up, but regardless, I thinkwe
> should revert that commit as bogus. We can try making it the default again
> next round, when maybe it will be true that it doesn't cause issues.
>
> What did it even ever help with?
In our case it is needed on some of our systems for PCI hotplug to
avoid MCKs due to a device under one root bus getting a resource
during hotplug that can only be used by devices under a different
root bus. If the user does not intend to use PCI hotplug, the
function isn't needed (i.e. 'pci=use_crs' can be omitted) because
resources are properly directed to the installed cards via BIOS
pre-assignment.
Gary
--
Gary Hade
System x Enablement
IBM Linux Technology Center
503-578-4503 IBM T/L: 775-4503
[email protected]
http://www.ibm.com/linux/ltc
On Wed, 24 Jun 2009 16:54:08 -0700 (PDT)
Linus Torvalds <[email protected]> wrote:
> On Wed, 24 Jun 2009, Jesse Barnes wrote:
> >
> > Yeah, I think it's reasonable to revert, especially given how we do
> > _CRS handling currently. I'm hoping at some point we can use the
> > _CRS data to at least augment the configuration we get from
> > hardware, since on some machines it seems to be necessary.
>
> Agreed. I do think we should take _CRS into account - possibly just
> as a minimal hint to determine which root buses to try to scan (maybe
> we do this already, I really didn't check). Or maybe we could use it
> to extend on our scan information.
>
> But when it seems to have things like "this bus can forward VGA
> cycles" kind of "resources" (which seems to be the main reason Larry
> Finger has so many of them), then that's just worthless knowledge
> that we're much better off just determining on our own.
Yeah, some of those bits don't seem useful; but OTOH if a given bridge
decodes a certain range in a non-configurable way how else will we get
that info w/o having huge tables of per-chip info? Those are the sorts
of things I worry about with our current resource handling. Maybe not
directly _CRS related, but still...
> Anyway, I may feel pretty strongly about things like this, but I'm
> also open to being convinced otherwise for 2.6.32. I wanted to do
> -rc1 today (it's been more than two weeks), and while I don't expect
> -rc1 to be flawless, I also hate to release it with _known_ bugs.
Sure, we'll keep at it and see if we can get something better in place
for .32.
> So partly due to timing, I'd rather revert it, and we can revisit it
> for the next release - whatever the actual end result then will be.
>
> [ There's a difference between "we're supposed to find and fix bugs
> in the -rc series", and "I release known-buggy -rc1's since we're
> supposed to fix it later". For similar reasons, I hate pulling
> known-buggy stuff during the merge window - it's ok if it shows
> itself to be buggy _later_, but if people send me stuff that they
> know is buggy as they send it to me, then that's a problem. ]
Yeah, 100% agreed. I didn't hear any reports until after people
started using your tree, so I think this case was handled correctly:
push something that *seems* ok upstream, but with eyes wide open for
the possibility we'd need to revert.
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
On Jun 24, 2009, at 3:37 PM, Jesse Barnes <[email protected]>
wrote:
> On Wed, 24 Jun 2009 14:48:25 -0700
> Yinghai Lu <[email protected]> wrote:
>
>>
>> so we could remove adjust_transparent_bridge_resources..
>>
>> and give x86_pci_root_bus_res_quirks chance when _CRS is not used or
>> not there.
>>
>> v2: add print out if pci conf reading for res is used for root
>>
>> Signed-off-by: Yinghai Lu <[email protected]>
>
> Do you think this is generally safe? I was hoping we could do
> something more complete, like making the code handle arbitrary numbers
> of resources in _CRS. Hacking that together now.
Should be safe
YH
don't touch info->res_num if we are out of space
Signed-off-by: Yinghai Lu <[email protected]>
---
arch/x86/pci/acpi.c | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 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
@@ -68,6 +68,10 @@ setup_resource(struct acpi_resource *acp
unsigned long flags;
struct resource *root;
int max_root_bus_resources = PCI_BUS_NUM_RESOURCES;
+ u64 start, end;
+
+ if (bus_has_transparent_bridge(info->bus))
+ max_root_bus_resources -= 3;
status = resource_to_addr(acpi_res, &addr);
if (!ACPI_SUCCESS(status))
@@ -84,25 +88,24 @@ setup_resource(struct acpi_resource *acp
} else
return AE_OK;
- res = &info->res[info->res_num];
- res->name = info->name;
- res->flags = flags;
- res->start = addr.minimum + addr.translation_offset;
- res->end = res->start + addr.address_length - 1;
- res->child = NULL;
-
- if (bus_has_transparent_bridge(info->bus))
- max_root_bus_resources -= 3;
+ start = addr.minimum + addr.translation_offset;
+ end = start + addr.address_length - 1;
if (info->res_num >= max_root_bus_resources) {
printk(KERN_WARNING "PCI: Failed to allocate 0x%lx-0x%lx "
"from %s for %s due to _CRS returning more than "
- "%d resource descriptors\n", (unsigned long) res->start,
- (unsigned long) res->end, root->name, info->name,
+ "%d resource descriptors\n", (unsigned long) start,
+ (unsigned long) end, root->name, info->name,
max_root_bus_resources);
- info->res_num++;
return AE_OK;
}
+ res = &info->res[info->res_num];
+ res->name = info->name;
+ res->flags = flags;
+ res->start = start;
+ res->end = end;
+ res->child = NULL;
+
if (insert_resource(root, res)) {
printk(KERN_ERR "PCI: Failed to allocate 0x%lx-0x%lx "
"from %s for %s\n", (unsigned long) res->start,
so we could remove adjust_transparent_bridge_resources..
and give x86_pci_root_bus_res_quirks chance when _CRS is not used or
not there.
v2: add print out if pci conf reading for res is used for root
v3: update after revert PCI_NO_ROOT_CRS
Signed-off-by: Yinghai Lu <[email protected]>
---
arch/x86/pci/acpi.c | 32 +++++++++-----------------------
arch/x86/pci/amd_bus.c | 8 ++++++--
2 files changed, 15 insertions(+), 25 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
@@ -118,23 +118,6 @@ setup_resource(struct acpi_resource *acp
}
static void
-adjust_transparent_bridge_resources(struct pci_bus *bus)
-{
- struct pci_dev *dev;
-
- list_for_each_entry(dev, &bus->devices, bus_list) {
- int i;
- u16 class = dev->class >> 8;
-
- if (class == PCI_CLASS_BRIDGE_PCI && dev->transparent) {
- for(i = 3; i < PCI_BUS_NUM_RESOURCES; i++)
- dev->subordinate->resource[i] =
- dev->bus->resource[i - 3];
- }
- }
-}
-
-static void
get_current_resources(struct acpi_device *device, int busnum,
int domain, struct pci_bus *bus)
{
@@ -161,8 +144,6 @@ get_current_resources(struct acpi_device
info.res_num = 0;
acpi_walk_resources(device->handle, METHOD_NAME__CRS, setup_resource,
&info);
- if (info.res_num)
- adjust_transparent_bridge_resources(bus);
return;
@@ -225,8 +206,15 @@ struct pci_bus * __devinit pci_acpi_scan
*/
memcpy(bus->sysdata, sd, sizeof(*sd));
kfree(sd);
- } else
- bus = pci_scan_bus_parented(NULL, busnum, &pci_root_ops, sd);
+ } else {
+ bus = pci_create_bus(NULL, busnum, &pci_root_ops, sd);
+ if (bus) {
+ if (pci_probe & PCI_USE_CRS)
+ get_current_resources(device, busnum, domain,
+ bus);
+ bus->subordinate = pci_scan_child_bus(bus);
+ }
+ }
if (!bus)
kfree(sd);
@@ -241,8 +229,6 @@ struct pci_bus * __devinit pci_acpi_scan
#endif
}
- if (bus && (pci_probe & PCI_USE__CRS))
- get_current_resources(device, busnum, domain, bus);
return bus;
}
Index: linux-2.6/arch/x86/pci/amd_bus.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/amd_bus.c
+++ linux-2.6/arch/x86/pci/amd_bus.c
@@ -100,8 +100,9 @@ void x86_pci_root_bus_res_quirks(struct
int j;
struct pci_root_info *info;
- /* don't go for it if _CRS is used */
- if (pci_probe & PCI_USE__CRS)
+ /* don't go for it if _CRS is used already */
+ if (b->resource[0] != &ioport_resource ||
+ b->resource[1] != &iomem_resource)
return;
/* if only one root bus, don't need to anything */
@@ -116,6 +117,9 @@ void x86_pci_root_bus_res_quirks(struct
if (i == pci_root_num)
return;
+ printk(KERN_DEBUG "PCI: peer root bus %02x res updated from pci conf\n",
+ b->number);
+
info = &pci_root_info[i];
for (j = 0; j < info->res_num; j++) {
struct resource *res;
so we could remove adjust_transparent_bridge_resources..
and give x86_pci_root_bus_res_quirks chance when _CRS is not used or
not there.
v2: add print out if pci conf reading for res is used for root
v3: update after revert PCI_NO_ROOT_CRS
v4: fix typo PCI_USE__CRS
Signed-off-by: Yinghai Lu <[email protected]>
---
arch/x86/pci/acpi.c | 32 +++++++++-----------------------
arch/x86/pci/amd_bus.c | 8 ++++++--
2 files changed, 15 insertions(+), 25 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
@@ -118,23 +118,6 @@ setup_resource(struct acpi_resource *acp
}
static void
-adjust_transparent_bridge_resources(struct pci_bus *bus)
-{
- struct pci_dev *dev;
-
- list_for_each_entry(dev, &bus->devices, bus_list) {
- int i;
- u16 class = dev->class >> 8;
-
- if (class == PCI_CLASS_BRIDGE_PCI && dev->transparent) {
- for(i = 3; i < PCI_BUS_NUM_RESOURCES; i++)
- dev->subordinate->resource[i] =
- dev->bus->resource[i - 3];
- }
- }
-}
-
-static void
get_current_resources(struct acpi_device *device, int busnum,
int domain, struct pci_bus *bus)
{
@@ -161,8 +144,6 @@ get_current_resources(struct acpi_device
info.res_num = 0;
acpi_walk_resources(device->handle, METHOD_NAME__CRS, setup_resource,
&info);
- if (info.res_num)
- adjust_transparent_bridge_resources(bus);
return;
@@ -225,8 +206,15 @@ struct pci_bus * __devinit pci_acpi_scan
*/
memcpy(bus->sysdata, sd, sizeof(*sd));
kfree(sd);
- } else
- bus = pci_scan_bus_parented(NULL, busnum, &pci_root_ops, sd);
+ } else {
+ bus = pci_create_bus(NULL, busnum, &pci_root_ops, sd);
+ if (bus) {
+ if (pci_probe & PCI_USE__CRS)
+ get_current_resources(device, busnum, domain,
+ bus);
+ bus->subordinate = pci_scan_child_bus(bus);
+ }
+ }
if (!bus)
kfree(sd);
@@ -241,8 +229,6 @@ struct pci_bus * __devinit pci_acpi_scan
#endif
}
- if (bus && (pci_probe & PCI_USE__CRS))
- get_current_resources(device, busnum, domain, bus);
return bus;
}
Index: linux-2.6/arch/x86/pci/amd_bus.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/amd_bus.c
+++ linux-2.6/arch/x86/pci/amd_bus.c
@@ -100,8 +100,9 @@ void x86_pci_root_bus_res_quirks(struct
int j;
struct pci_root_info *info;
- /* don't go for it if _CRS is used */
- if (pci_probe & PCI_USE__CRS)
+ /* don't go for it if _CRS is used already */
+ if (b->resource[0] != &ioport_resource ||
+ b->resource[1] != &iomem_resource)
return;
/* if only one root bus, don't need to anything */
@@ -116,6 +117,9 @@ void x86_pci_root_bus_res_quirks(struct
if (i == pci_root_num)
return;
+ printk(KERN_DEBUG "PCI: peer root bus %02x res updated from pci conf\n",
+ b->number);
+
info = &pci_root_info[i];
for (j = 0; j < info->res_num; j++) {
struct resource *res;
* Jesse Barnes <[email protected]> wrote:
> > [ There's a difference between "we're supposed to find and fix bugs
> > in the -rc series", and "I release known-buggy -rc1's since we're
> > supposed to fix it later". For similar reasons, I hate pulling
> > known-buggy stuff during the merge window - it's ok if it shows
> > itself to be buggy _later_, but if people send me stuff that they
> > know is buggy as they send it to me, then that's a problem. ]
>
> Yeah, 100% agreed. I didn't hear any reports until after people
> started using your tree, so I think this case was handled
> correctly: push something that *seems* ok upstream, but with eyes
> wide open for the possibility we'd need to revert.
There's only one small gripe i have with the handling of it: the
timing. "9e9f46c: PCI: use ACPI _CRS data by default" was written
and committed on June 11th, two days _after_ the merge window
opened.
That's way too late for maybe-broken changes to x86 lowlevel details
(especially if it touches hw-environmental interaction - which is
very hard to test with meaningful coverage), and it's also pretty
much the worst moment to solicit testing from people who are busy
getting their stuff to Linus and who are busy testing out any of the
unexpected interactions and bugs.
So this was, to a certain degree, a predictable outcome. Trees in
the Linux "critical path" of testing (core kernel, x86, core
networking, very common drivers, PCI, driver core, VFS, etc.) should
generally try to cool down 1-2 weeks before the merge window -
because breakage there can do a lot of knock-on cascading damage.
Two weeks is not a lot of time and the effects of showstopper bugs
get magnified disproportionately.
Ingo
On Thu, 2009-06-25 at 09:03 +0200, Ingo Molnar wrote:
> * Jesse Barnes <[email protected]> wrote:
>
> > > [ There's a difference between "we're supposed to find and fix bugs
> > > in the -rc series", and "I release known-buggy -rc1's since we're
> > > supposed to fix it later". For similar reasons, I hate pulling
> > > known-buggy stuff during the merge window - it's ok if it shows
> > > itself to be buggy _later_, but if people send me stuff that they
> > > know is buggy as they send it to me, then that's a problem. ]
> >
> > Yeah, 100% agreed. I didn't hear any reports until after people
> > started using your tree, so I think this case was handled
> > correctly: push something that *seems* ok upstream, but with eyes
> > wide open for the possibility we'd need to revert.
>
> There's only one small gripe i have with the handling of it: the
> timing. "9e9f46c: PCI: use ACPI _CRS data by default" was written
> and committed on June 11th, two days _after_ the merge window
> opened.
>
> That's way too late for maybe-broken changes to x86 lowlevel details
> (especially if it touches hw-environmental interaction - which is
> very hard to test with meaningful coverage), and it's also pretty
> much the worst moment to solicit testing from people who are busy
> getting their stuff to Linus and who are busy testing out any of the
> unexpected interactions and bugs.
>
> So this was, to a certain degree, a predictable outcome. Trees in
> the Linux "critical path" of testing (core kernel, x86, core
> networking, very common drivers, PCI, driver core, VFS, etc.) should
> generally try to cool down 1-2 weeks before the merge window -
> because breakage there can do a lot of knock-on cascading damage.
> Two weeks is not a lot of time and the effects of showstopper bugs
> get magnified disproportionately.
>
Yes, I was also thinking about this when I checked the commit date. And
totally agree with Ingo's suggestions.
Thanks,
--
JSR
On Thu, 25 Jun 2009 09:03:47 +0200
Ingo Molnar <[email protected]> wrote:
>
> * Jesse Barnes <[email protected]> wrote:
>
> > > [ There's a difference between "we're supposed to find and fix
> > > bugs in the -rc series", and "I release known-buggy -rc1's since
> > > we're supposed to fix it later". For similar reasons, I hate
> > > pulling known-buggy stuff during the merge window - it's ok if it
> > > shows itself to be buggy _later_, but if people send me stuff
> > > that they know is buggy as they send it to me, then that's a
> > > problem. ]
> >
> > Yeah, 100% agreed. I didn't hear any reports until after people
> > started using your tree, so I think this case was handled
> > correctly: push something that *seems* ok upstream, but with eyes
> > wide open for the possibility we'd need to revert.
>
> There's only one small gripe i have with the handling of it: the
> timing. "9e9f46c: PCI: use ACPI _CRS data by default" was written
> and committed on June 11th, two days _after_ the merge window
> opened.
>
> That's way too late for maybe-broken changes to x86 lowlevel details
> (especially if it touches hw-environmental interaction - which is
> very hard to test with meaningful coverage), and it's also pretty
> much the worst moment to solicit testing from people who are busy
> getting their stuff to Linus and who are busy testing out any of the
> unexpected interactions and bugs.
True, the patch had been around long before that and I definitely
should have committed it sooner.
--
Jesse Barnes, Intel Open Source Technology Center
On Wed, 24 Jun 2009 19:01:19 -0700
Yinghai Lu <[email protected]> wrote:
>
> don't touch info->res_num if we are out of space
>
> Signed-off-by: Yinghai Lu <[email protected]>
Gary, any comments on these?
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
On Mon, Jun 29, 2009 at 06:16:03PM -0700, Jesse Barnes wrote:
> On Wed, 24 Jun 2009 19:01:19 -0700
> Yinghai Lu <[email protected]> wrote:
>
> >
> > don't touch info->res_num if we are out of space
> >
> > Signed-off-by: Yinghai Lu <[email protected]>
>
> Gary, any comments on these?
Jesse,
"[PATCH 1/3] x86/pci: fix boundary checking when using root CRS"
which convinced that my code was incorrect and
"[PATCH 2/3] x86/pci: get root CRS before scan childs -v3"
look good to me from both code review and functional [1] standpoints.
Acked-by: <[email protected]>
Tested-by: <[email protected]>
[1] Successful boot of patched 2.6.31-rc1 with pci=use_crs
followed by successful hot-add and hot-remove of PCI-X
and PCIe cards on an IBM x3850.
Thanks Yinghai.
Gary
--
Gary Hade
System x Enablement
IBM Linux Technology Center
503-578-4503 IBM T/L: 775-4503
[email protected]
http://www.ibm.com/linux/ltc
On Tue, 30 Jun 2009 11:04:49 -0700
Gary Hade <[email protected]> wrote:
> On Mon, Jun 29, 2009 at 06:16:03PM -0700, Jesse Barnes wrote:
> > On Wed, 24 Jun 2009 19:01:19 -0700
> > Yinghai Lu <[email protected]> wrote:
> >
> > >
> > > don't touch info->res_num if we are out of space
> > >
> > > Signed-off-by: Yinghai Lu <[email protected]>
> >
> > Gary, any comments on these?
>
> Jesse,
> "[PATCH 1/3] x86/pci: fix boundary checking when using root CRS"
> which convinced that my code was incorrect and
> "[PATCH 2/3] x86/pci: get root CRS before scan childs -v3"
> look good to me from both code review and functional [1] standpoints.
> Acked-by: <[email protected]>
> Tested-by: <[email protected]>
>
> [1] Successful boot of patched 2.6.31-rc1 with pci=use_crs
> followed by successful hot-add and hot-remove of PCI-X
> and PCIe cards on an IBM x3850.
>
> Thanks Yinghai.
Thanks guys, I've applied these to my for-linus branch for a pull later
this week.
--
Jesse Barnes, Intel Open Source Technology Center