Fix boot-time hang on G31/G33 PC

This patch, loosely based on a patch from Robert Hancock, which was in turn based on a patch from Jesse Barnes, fixes a boot-time hang on my shiny new PC. The 'conflict' mentioned in the patch in my case happens to be between mmconfig and the graphics card, but it could easily be between any pair of devices if they are left enabled by the BIOS and mappen in the wrong place.

Signed-off-by Matthew Wilcox <[email protected]>

drivers/pci/probe.c +18 -0

index 34b8dae..51ef450 100644
@@ -180,11 +180,26 @@ static inline int is_64bit_memory(u32 mask)
 	return 0;
 }
 
+/*
+ * Sizing PCI BARs requires us to disable decoding, otherwise we may run
+ * into conflicts with other devices while trying to size the BAR.  Normally
+ * this isn't a problem, but it happens on some machines normally, and can
+ * happen on others during PCI device hotplug.  Don't disable BARs for host
+ * bridges, though.  Some of them do silly things like disable accesses to
+ * RAM from the CPU
+ */
 static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
 {
 	unsigned int pos, reg, next;
 	u32 l, sz;
 	struct resource *res;
+	u16 orig_cmd;
+
+	if ((dev->class >> 8) != PCI_CLASS_BRIDGE_HOST) {
+		pci_read_config_word(dev, PCI_COMMAND, &orig_cmd);
+		pci_write_config_word(dev, PCI_COMMAND,
+			orig_cmd & ~(PCI_COMMAND_MEMORY | PCI_COMMAND_IO));
+	}
 
 	for(pos=0; pos<howmany; pos = next) {
 		u64 l64;

@@ -283,6 +298,9 @@ static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
 			}
 		}
 	}
+
+	if ((dev->class >> 8) != PCI_CLASS_BRIDGE_HOST)
+		pci_write_config_word(dev, PCI_COMMAND, orig_cmd);
 }
 
 void __devinit pci_read_bridge_bases(struct pci_bus *child)

We've already got a patch for this in Greg's PCI tree, hopefully it should go in for 2.6.24.

Are you getting MMCONFIG enabled on your system with 2.6.23?

Quoted 2 lines · Robert Hancock

We've already got a patch for this in Greg's PCI tree, hopefully it should go in for 2.6.24.

I haven't seen it. I guess it wasn't sent to the PCI mailing list.

Your patch had two or three problems with it; assuming we're talking about the same patch.

Quoted 6 lines · Matthew Wilcox

Quoted 1 line · Robert Hancock

We've already got a patch for this in Greg's PCI tree, hopefully it should go in for 2.6.24.

I haven't seen it. I guess it wasn't sent to the PCI mailing list.

Your patch had two or three problems with it; assuming we're talking about the same patch.

I found
http://git.kernel.org/?p=linux/kernel/git/gregkh/patches.git;a=blob;f=pci/pci-disable-decode-of-io-memory-during-bar-sizing.patch;h=958ef4e837733206a80f058aee236847eec5fbd8;hb=HEAD

which has two problems:

- With a 64-bit BAR, it checks to see if the upper 32 bits represent IO or memory. You can't do that to the upper 32 bits.
- It does a lot of additional writes to the cmd register; my patch does two writes per device; yours does two per BAR

It's also a lot more complex than my patch, IMO.

Greg, please drop Robert's patch and put mine in instead.

Quoted 1 line

Greg, please drop Robert's patch and put mine in instead.

Based on looking at your patch it seems OK. The first problem you mentioned with what Jesse and I had there is definitely a valid one.

You can add my:

Acked-by Robert Hancock <[email protected]>

This patch, loosely based on a patch from Robert Hancock, which was in turn based on a patch from Jesse Barnes, fixes a boot-time hang on my shiny new PC. The 'conflict' mentioned in the patch in my case happens to be between mmconfig and the graphics card, but it could easily be between any pair of devices if they are left enabled by the BIOS and mappen in the wrong place.

Signed-off-by Matthew Wilcox <[email protected]>

drivers/pci/probe.c +18 -0

index 34b8dae..51ef450 100644
@@ -180,11 +180,26 @@ static inline int is_64bit_memory(u32 mask)
 	return 0;
 }
 
+/*
+ * Sizing PCI BARs requires us to disable decoding, otherwise we may run
+ * into conflicts with other devices while trying to size the BAR.  Normally
+ * this isn't a problem, but it happens on some machines normally, and can
+ * happen on others during PCI device hotplug.  Don't disable BARs for host
+ * bridges, though.  Some of them do silly things like disable accesses to
+ * RAM from the CPU
+ */
 static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
 {
 	unsigned int pos, reg, next;
 	u32 l, sz;
 	struct resource *res;
+	u16 orig_cmd;
+
+	if ((dev->class >> 8) != PCI_CLASS_BRIDGE_HOST) {
+		pci_read_config_word(dev, PCI_COMMAND, &orig_cmd);
+		pci_write_config_word(dev, PCI_COMMAND,
+			orig_cmd & ~(PCI_COMMAND_MEMORY | PCI_COMMAND_IO));
+	}
 
 	for(pos=0; pos<howmany; pos = next) {
 		u64 l64;

@@ -283,6 +298,9 @@ static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
 			}
 		}
 	}
+
+	if ((dev->class >> 8) != PCI_CLASS_BRIDGE_HOST)
+		pci_write_config_word(dev, PCI_COMMAND, orig_cmd);
 }
 
 void __devinit pci_read_bridge_bases(struct pci_bus *child)

Signature · 4 lines

-- 
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Quoted 7 lines · Matthew Wilcox

This patch, loosely based on a patch from Robert Hancock, which was in turn based on a patch from Jesse Barnes, fixes a boot-time hang on my shiny new PC. The 'conflict' mentioned in the patch in my case happens to be between mmconfig and the graphics card, but it could easily be between any pair of devices if they are left enabled by the BIOS and mappen in the wrong place.

This issue has come up before:

    http://www.uwsg.indiana.edu/hypermail/linux/kernel/0212.2/0232.html

This patch, loosely based on a patch from Robert Hancock, which was in turn based on a patch from Jesse Barnes, fixes a boot-time hang on my shiny new PC. The 'conflict' mentioned in the patch in my case happens to be between mmconfig and the graphics card, but it could easily be between any pair of devices if they are left enabled by the BIOS and mappen in the wrong place.

Signed-off-by Matthew Wilcox <[email protected]>

Acked-by Robert Hancock <[email protected]>

Acked-by Jesse Barnes <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/pci/probe.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

drivers/pci/probe.c +18 -0

@@ -180,11 +180,26 @@ static inline int is_64bit_memory(u32 ma
 	return 0;
 }
 
+/*
+ * Sizing PCI BARs requires us to disable decoding, otherwise we may run
+ * into conflicts with other devices while trying to size the BAR.  Normally
+ * this isn't a problem, but it happens on some machines normally, and can
+ * happen on others during PCI device hotplug.  Don't disable BARs for host
+ * bridges, though.  Some of them do silly things like disable accesses to
+ * RAM from the CPU
+ */
 static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
 {
 	unsigned int pos, reg, next;
 	u32 l, sz;
 	struct resource *res;
+	u16 orig_cmd;
+
+	if ((dev->class >> 8) != PCI_CLASS_BRIDGE_HOST) {
+		pci_read_config_word(dev, PCI_COMMAND, &orig_cmd);
+		pci_write_config_word(dev, PCI_COMMAND,
+			orig_cmd & ~(PCI_COMMAND_MEMORY | PCI_COMMAND_IO));
+	}
 
 	for(pos=0; pos<howmany; pos = next) {
 		u64 l64;

@@ -283,6 +298,9 @@ static void pci_read_bases(struct pci_de
 			}
 		}
 	}
+
+	if ((dev->class >> 8) != PCI_CLASS_BRIDGE_HOST)
+		pci_write_config_word(dev, PCI_COMMAND, orig_cmd);
 }
 
 void pci_read_bridge_bases(struct pci_bus *child)

Due to the issues surrounding this patch, I'm dropping it from my repo.

Quoted 2 lines · Greg KH

Due to the issues surrounding this patch, I'm dropping it from my repo.

What issues? Is it causing problems for people?

Quoted 5 lines · Jesse Barnes

Quoted 2 lines · Greg KH

Due to the issues surrounding this patch, I'm dropping it from my repo.

What issues? Is it causing problems for people?

I thought this was the patch that Ivan objected to.

Quoted 4 lines · Greg KH

Quoted 3 lines · Jesse Barnes

Quoted 2 lines · Greg KH

Due to the issues surrounding this patch, I'm dropping it from my repo.

Quoted 2 lines

Quoted 1 line

What issues? Is it causing problems for people?

I thought this was the patch that Ivan objected to.

Yeah, Ivan objected to this, but incorrectly I think.

Ivan, your concern is about disabling things like interrupt controllers and power management chips during probe right? You're right that doing that could cause problems if we get and interrupt or PMU event at just the wrong time, but that could just as easily happen if decode was still enabled but the BAR had a bogus address programmed (as it would during probing).

Ultimately, I don't care much one way or another as long as we can get the desktop platforms fixed somehow. I think disabling decode is the most correct way of doing this, but I'm open to other solutions (this is the only patch I've seen though that's been tested to solve the problem).

Ivan, your concern is about disabling things like interrupt controllers and power management chips during probe right? You're right that doing that could cause problems if we get and interrupt or PMU event at just the wrong time, but that could just as easily happen if decode was still enabled but the BAR had a bogus address programmed (as it would during probing).

Ultimately, I don't care much one way or another as long as we can get the desktop platforms fixed somehow. I think disabling decode is the most correct way of doing this, but I'm open to other solutions (this is the only patch I've seen though that's been tested to solve the problem).

Quoted 6 lines · Jesse Barnes

Ivan, your concern is about disabling things like interrupt controllers and power management chips during probe right? You're right that doing that could cause problems if we get and interrupt or PMU event at just the wrong time, but that could just as easily happen if decode was still enabled but the BAR had a bogus address programmed (as it would during probing).

Yes, nobody is arguing that moving the BAR around is unsafe, but generally it's the less of two evils.

The major problem here is that with IO and MEM bits cleared in the command register you disable *all* address decoders on the device, not just ranges that have respective BARs. At least this behaviour is required by PCI spec. Examples:
- legacy VGA IO and memory (no corresponding BARs);
- base/limit registers of P2P bridge;
- PMU and SMBus registers (sort of normal BARs, but hidden elsewhere in the config space);
- IDE legacy mode registers;
- IO-APIC registers (typically sort of read-only BAR).

For all of these address ranges our current BAR probe is effectively a no-op, but disable/re-enable clearly isn't.

Quoted 5 lines

Ultimately, I don't care much one way or another as long as we can get the desktop platforms fixed somehow. I think disabling decode is the most correct way of doing this, but I'm open to other solutions (this is the only patch I've seen though that's been tested to solve the problem).

There are two other solutions: one is to disable decode selectively, only on devices or systems where it's necessary and known to be safe. I've posted a patch which introduces "disable_while_probe" pci_dev field for that purpose.
Another one is to delay mmconfig probe until after the PCI probe is done, as Matthew suggested, and Robert confirmed that it's feasible.

Quoted 31 lines · Ivan Kokshaysky

Quoted 6 lines · Jesse Barnes

Ivan, your concern is about disabling things like interrupt controllers and power management chips during probe right? You're right that doing that could cause problems if we get and interrupt or PMU event at just the wrong time, but that could just as easily happen if decode was still enabled but the BAR had a bogus address programmed (as it would during probing).

Yes, nobody is arguing that moving the BAR around is unsafe, but generally it's the less of two evils.

The major problem here is that with IO and MEM bits cleared in the command register you disable *all* address decoders on the device, not just ranges that have respective BARs. At least this behaviour is required by PCI spec. Examples:
- legacy VGA IO and memory (no corresponding BARs);
- base/limit registers of P2P bridge;
- PMU and SMBus registers (sort of normal BARs, but hidden elsewhere in the config space);
- IDE legacy mode registers;
- IO-APIC registers (typically sort of read-only BAR).

For all of these address ranges our current BAR probe is effectively a no-op, but disable/re-enable clearly isn't.

Quoted 5 lines

Ultimately, I don't care much one way or another as long as we can get the desktop platforms fixed somehow. I think disabling decode is the most correct way of doing this, but I'm open to other solutions (this is the only patch I've seen though that's been tested to solve the problem).

Quoted 6 lines

There are two other solutions: one is to disable decode selectively, only on devices or systems where it's necessary and known to be safe. I've posted a patch which introduces "disable_while_probe" pci_dev field for that purpose.
Another one is to delay mmconfig probe until after the PCI probe is done, as Matthew suggested, and Robert confirmed that it's feasible.

for everyone who's using this quirk or has the same boot issue: I just confirmed that the new dg33tl bios update v0287 (released 9/20) fixes the boot issue for my systems. I encourage everyone to update their BIOS image and see if this works.

Quoted 39 lines · Auke

Quoted 37 lines · Ivan Kokshaysky

Quoted 6 lines · Jesse Barnes

Ivan, your concern is about disabling things like interrupt controllers and power management chips during probe right? You're right that doing that could cause problems if we get and interrupt or PMU event at just the wrong time, but that could just as easily happen if decode was still enabled but the BAR had a bogus address programmed (as it would during probing).

Yes, nobody is arguing that moving the BAR around is unsafe, but generally it's the less of two evils.

The major problem here is that with IO and MEM bits cleared in the command register you disable *all* address decoders on the device, not just ranges that have respective BARs. At least this behaviour is required by PCI spec. Examples:
- legacy VGA IO and memory (no corresponding BARs);
- base/limit registers of P2P bridge;
- PMU and SMBus registers (sort of normal BARs, but hidden elsewhere in the config space);
- IDE legacy mode registers;
- IO-APIC registers (typically sort of read-only BAR).

For all of these address ranges our current BAR probe is effectively a no-op, but disable/re-enable clearly isn't.

Quoted 5 lines

Ultimately, I don't care much one way or another as long as we can get the desktop platforms fixed somehow. I think disabling decode is the most correct way of doing this, but I'm open to other solutions (this is the only patch I've seen though that's been tested to solve the problem).

There are two other solutions: one is to disable decode selectively, only on devices or systems where it's necessary and known to be safe. I've posted a patch which introduces "disable_while_probe" pci_dev field for that purpose.
Another one is to delay mmconfig probe until after the PCI probe is done, as Matthew suggested, and Robert confirmed that it's feasible.

Quoted 4 lines

for everyone who's using this quirk or has the same boot issue: I just confirmed that the new dg33tl bios update v0287 (released 9/20) fixes the boot issue for my systems. I encourage everyone to update their BIOS image and see if this works.

Thanks for letting us know. So, another reason to drop this patch :)

thanks,

greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 37 lines · On the 28 September 2007 03:13 Greg KH, wrote:

Quoted 36 lines · Auke

Quoted 35 lines · Ivan Kokshaysky

Quoted 6 lines · Jesse Barnes

Ivan, your concern is about disabling things like interrupt controllers and power management chips during probe right? You're right that doing that could cause problems if we get and interrupt or PMU event at just the wrong time, but that could just as easily happen if decode was still enabled but the BAR had a bogus address programmed (as it would during probing).

Yes, nobody is arguing that moving the BAR around is unsafe, but generally it's the less of two evils.

The major problem here is that with IO and MEM bits cleared in the command register you disable *all* address decoders on the device, not just ranges that have respective BARs. At least this behaviour is required by PCI spec. Examples:
- legacy VGA IO and memory (no corresponding BARs);
- base/limit registers of P2P bridge;
- PMU and SMBus registers (sort of normal BARs, but hidden elsewhere in the config space);
- IDE legacy mode registers;
- IO-APIC registers (typically sort of read-only BAR).

For all of these address ranges our current BAR probe is effectively a no-op, but disable/re-enable clearly isn't.

Quoted 5 lines

Ultimately, I don't care much one way or another as long as we can get the desktop platforms fixed somehow. I think disabling decode is the most correct way of doing this, but I'm open to other solutions (this is the only patch I've seen though that's been tested to solve the problem).

There are two other solutions: one is to disable decode selectively, only on devices or systems where it's necessary and known to be safe. I've posted a patch which introduces "disable_while_probe" pci_dev field for that purpose.
Another one is to delay mmconfig probe until after the PCI probe is done, as Matthew suggested, and Robert confirmed that it's feasible.

Quoted 4 lines

Quoted 4 lines

for everyone who's using this quirk or has the same boot issue: I just confirmed that the new dg33tl bios update v0287 (released 9/20) fixes the boot issue for my systems. I encourage everyone to update their BIOS image and see if this works.

No, it still doesn't work even with a latest BIOS verstion. I have a computer with Intel DQ35MP motherboard. I upgraded BIOS to rev. 0696, released Oct 1. But kernel (2.6.22.5) still hangs up. Kernel with this fix patch boots and works fine.