firmware leaves device in D3hot at boot

I'm looking at a problem caused by a machine's BIOS leaving an adapter in D3hot when booting. As part of the boot process, the driver (in this case 3c59x) calls pci_enable_device which sets the power state to D0. The device in question gets its PCI configuration reset, including the BARs. As a result, the driver is unable to init the device.

Section 5.4.1 of the "PCI BUS POWER MANAGEMENT INTERFACE SPECIFICATION, REV. 1.2" indicates that a device transitioning from D3hot to D0 _may_ perform an internal reset, thereby going to "D0 Uninitialized" rather than "D0 Initialized". This behaviour requires that the PCI configuration of the device be restored after the D0 transition.

In most cases (as in calling ->resume), this is No Big Deal. Many (or most?) drivers simply call pci_restore_state after the transition to D0. However, when a driver loads it typically calls pci_enable_device very early in the process. (I guess they presume that the device will be in D0.) AFAICT none of them call pci_save_state before doing so (or pci_restore_state afterward).

Drivers could be modified to do this, but that potentially requires a lot of very similar changes to many/most/all PCI drivers. It could be argued that this is appropriate since the necessity of this step depends on the hardware in use. On the other hand, many drivers will likely support some devices that exhibit this behaviour and some that do not.

This issue regarding D3hot->D0 state transitions seems like a piece of minutiae that we should not force individual drivers to address. There is a bit in the PM status/control register that determines whether or not the D3hot->D0 transition will cause such a reset. The pci_set_power_state function is already looking at the PM status/control register while most drivers have no other need to do so. Therefore, I propose handling this situation inside the pci_set_power_state routine. Patch below for discussion only...please do NOT apply at this time...

Anyway, let me know what you think...

John

P.S. I'm using dev->current_state == PCI_D3cold to indicate the first use of this device after a (re)boot, and to restrict this behaviour to that situation. Part of me thinks that it may be worthwhile to preserve the PCI configuration on any D3hot->D0 transition in case the driver is being reloaded and doesn't know to restore the config.

P.P.S. Of course, the above concern may argue in favor of putting responsibility for this on the drivers in the first place...? Is there any way to differentiate between whether or not pci_set_power_state is being called from a ->probe routine rather than a ->resume routine?

@@ -241,6 +241,7 @@ pci_set_power_state(struct pci_dev *dev,
 {
 	int pm;
 	u16 pmcsr, pmc;
+	int pci_state_saved = 0;
 
 	/* bound the state we're entering */
 	if (state > PCI_D3hot)

@@ -278,6 +279,22 @@ pci_set_power_state(struct pci_dev *dev,
 			return -EIO;
 	}
 
+	pci_read_config_word(dev, pm + PCI_PM_CTRL, &pmcsr);
+
+	/* dev->current_state == PCI_D3cold actually indicates (re)boot
+		-- some firmware will leave devices in D3hot on boot
+		-- some devices will loose config (incl BARs) in D3hot->D0
+		-- for those devices, save config and restore after ->D0
+	   Could make drivers do this, but better to leave them ignorant
+	   of PCI PM trivia...
+	*/
+	if ((state == PCI_D0 && dev->current_state == PCI_D3cold) &&
+	    ((pmcsr & PCI_PM_CTRL_STATE_MASK) == PCI_D3hot) &&
+	    !(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET)) {
+		pci_save_state(dev);
+		pci_state_saved = 1;
+	}
+
 	/* If we're in D3, force entire word to 0.
 	 * This doesn't affect PME_Status, disables PME_En, and
 	 * sets PowerState to 0.

@@ -285,7 +302,6 @@ pci_set_power_state(struct pci_dev *dev,
 	if (dev->current_state >= PCI_D3hot)
 		pmcsr = 0;
 	else {
-		pci_read_config_word(dev, pm + PCI_PM_CTRL, &pmcsr);
 		pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
 		pmcsr |= state;
 	}

@@ -301,6 +317,9 @@ pci_set_power_state(struct pci_dev *dev,
 		udelay(200);
 	dev->current_state = state;
 
+	if (pci_state_saved)
+		pci_restore_state(dev);
+
 	return 0;
 }
 

@@ -225,6 +225,7 @@
 #define  PCI_PM_CAP_PME_D3cold  0x8000  /* PME# from D3 (cold) */
 #define PCI_PM_CTRL		4	/* PM control and status register */
 #define  PCI_PM_CTRL_STATE_MASK	0x0003	/* Current power state (D0 to D3) */
+#define  PCI_PM_CTRL_NO_SOFT_RESET	0x0004	/* No reset for D3hot->D0 */
 #define  PCI_PM_CTRL_PME_ENABLE	0x0100	/* PME pin enable */
 #define  PCI_PM_CTRL_DATA_SEL_MASK	0x1e00	/* Data select (??) */
 #define  PCI_PM_CTRL_DATA_SCALE_MASK	0x6000	/* Data scale (??) */

John W. Linville
[email protected]

After some thought, I'm inclined to think that the patch below is the right one. It unconditionally saves and restores the PCI config registers when pci_enable_device is called. Although this is often (usually?) unnecessary, it seems like a safe thing to do and it should not be a performance-sensitive path. The code to check for whether or not this is necessary would be a little harder to read IMHO, so I think this is warranted.

The comment block at the head of pci_enable_device says "Initialize device before it's used by a driver" which implies that saving and restoring the PCI config should be safe if the function is used as intended. Out of ~318 files referencing pci_enable_device, ~47 of them (~15%) of them are actually calling pci_enable_device from multiple places. Most of these that I've looked at so far are calling it from ->resume as well as ->probe. A few drivers seem to call pci_enable_device from one of several branches within the same if statement inside the ->probe function, but I figure thoese examples are equivalent to just calling it from a single place.

Is calling pci_enable_device from ->resume really proper? If not, what should those devices be doing? I'll be happy to take a glance at all the drivers calling pci_enable_device multiple times if there is general agreement that the approach below would be acceptable.

What do you all think?

John

P.S. Below is the one-liner I used to determine who is accessing pci_enable_device. It probably misses a few strays...

# Generate list of calls to pci_enable_device

find . -type f -name '*.c' -exec \
	grep -H pci_enable_device[[:space:]]*\(.*\).*[\;\)] {} \; | \
	cut -f1 -d: | sort | uniq -c

@@ -399,10 +399,23 @@ pci_enable_device(struct pci_dev *dev)
 {
 	int err;
 
+	/* Need to save the PCI config space because some firmware
+	   will leave devices in D3hot on boot and some devices
+	   will loose config (incl BARs) in D3hot->D0 PM state
+	   transition.	Could make drivers do this, but better to
+	   leave them ignorant of PCI PM trivia...
+	*/
+	if (err = pci_save_state(dev))
+		return err;
+
 	if ((err = pci_enable_device_bars(dev, (1 << PCI_NUM_RESOURCES) - 1)))
 		return err;
 	pci_fixup_device(pci_fixup_enable, dev);
 	dev->is_enabled = 1;
+
+	if (err = pci_restore_state(dev))
+		return err;
+
 	return 0;
 }
 

John W. Linville
[email protected]

But how does this solve your problem with the state change?

All pci drivers must call pci_enable_device() before they start to use it.

thanks,

greg k-h

Please ignore the previous patches. I've had some good feedback from Russell King and Adam Belay, and I have reformulated the patch accordingly. I will be posting that shortly.

Thanks,

John

John W. Linville
[email protected]

Some PCI devices lose all configuration (including BARs) when transitioning from D3hot->D0. This leaves such a device in an inaccessible state. The patch below causes the BARs to be restored when enabling a device, so that the driver will be able to access it.

---
Some firmware leaves devices in D3hot after a (re)boot. Most drivers call pci_enable_device very early, so devices left in D3hot that lose configuration during the D3hot->D0 transition will be inaccessible to their drivers.

Drivers could be modified to account for this, but it would be difficult to know which drivers need modification. This is especially true since often many devices are covered by the same driver. It likely would be necessary to replicate code across dozens of drivers.

Since the fix should be safe for any device, it seems appropriate to make it part of the PCI infrastructure.

 drivers/pci/pci.c |   49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 48 insertions(+), 1 deletion(-)

@@ -378,9 +378,56 @@ pci_restore_state(struct pci_dev *dev)
 int
 pci_enable_device_bars(struct pci_dev *dev, int bars)
 {
-	int err;
+	int i, numres, err;
 
 	pci_set_power_state(dev, PCI_D0);
+
+	/* Some devices lose PCI config header data during D3hot->D0
+	   transition.	Since some firmware leaves devices in D3hot
+	   state at boot, this information needs to be restored.  We
+	   could force drivers to do this, but better to leave them
+	   ignorant of PCI PM trivia...
+	*/
+	switch (dev->hdr_type) {
+	case PCI_HEADER_TYPE_NORMAL:
+		numres = 6;
+		break;
+	case PCI_HEADER_TYPE_BRIDGE:
+		numres = 2;
+		break;
+	case PCI_HEADER_TYPE_CARDBUS:
+		numres = 1;
+		break;
+	default:
+		/* Should never get here, but just in case... */
+		numres = 0;
+		break;
+	}
+	for (i = 0; i < numres; i ++) {
+		struct pci_bus_region region;
+		u32 val;
+		int reg;
+
+		if (!dev->resource[i].flags)
+			continue;
+
+		pcibios_resource_to_bus(dev, &region, &dev->resource[i]);
+
+		val = region.start
+		    | (dev->resource[i].flags & PCI_REGION_FLAG_MASK);
+
+		reg = PCI_BASE_ADDRESS_0 + (i * 4);
+
+		pci_write_config_dword(dev, reg, val);
+
+		if ((val & (PCI_BASE_ADDRESS_SPACE
+		          | PCI_BASE_ADDRESS_MEM_TYPE_MASK))
+		 == (PCI_BASE_ADDRESS_SPACE_MEMORY
+		   | PCI_BASE_ADDRESS_MEM_TYPE_64)) {
+			pci_write_config_dword(dev, reg + 4, 0);
+		}
+	}
+
 	if ((err = pcibios_enable_device(dev, bars)) < 0)
 		return err;
 	return 0;

John W. Linville
[email protected]

On Thu, Jun 30, 2005 at 10:26:37PM -0400, John W. Linville wrote:
...

...

		val = region.start >> 4;
		pci_write_config_dword(dev, reg + 4, val);

That's contary to the assumptions made by setup-res.c. BAR0 is dev->resource[0]. If that resource is 64-bit, dev->resource[1] is unused and the next resource is dev->resource[2].

Are you sure you mean >> 4 ? Don't you mean >> 32 ? Note again that setup-res.c writes zero unconditionally here.

The PCI subsystem is incomplete for 64-bit BAR support. What it does do though is ensure that 64-bit BARs will work correctly in a 32-bit system. Therefore, I think that folk who want 64-bit BAR support to work need to do some code reviews on the PCI subsystem to resolve the remaining issues.

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

64-bit BARs work fine on 64-bit machines. I'm ambivalent whether we ought to support 64-bit BARs on 32-bit machines.

"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

This only occurs because the problematical functions (eg, pci_update_resource) probably aren't called on 64-bit machines - if they were, they'd zero the upper 32-bits. Maybe 64-bit machines are happy with that anyway?

Rather than reimplementing the internals of pci_update_resource() it may be worth splitting the common stuff out so it gets fixed for both pci_update_resource() and pci_enable_device().

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Why problematical? It's just the way how linux has always dealt with 64-bit BARs - put everything below 4G in the bus address space, on *any* architecture. I'd be quite surprised if some firmware doesn't do the same thing - so far I haven't heard any complaints.
Eventually we'll have to support MMIO above 4G, but I suspect this won't be necessary at least in a next couple of years. Anyway, there are no fundamental changes required for the generic PCI layer to handle that, just some tweaks here and there - almost all non-trivial stuff will be arch-specific.

Just use pci_update_resource().

John, I'd also suggest following changes to the patch:
- move the code to pci_set_power_state(), where it belongs to;
- explicitly check for D3hot->D0 transition *and* for the No_Soft_Reset bit, to avoid unnecessary config space accesses;
- add a quote from PCI spec (as a comment) explaining why is it needed.

Ivan.

If this is so, Grant's concern about programming the top half of 64-bit resources with zero isn't appropriate. However, he did raise this as an issue...

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Problem: pci_update_resource doesn't exist for sparc64.

I have reformulated the patch to account for these comments, but I am not currently using pci_update_resource for the reason stated above. I'll go ahead and post the new patch for comment. If we can resolve the pci_update_resource issue, I'll post another (either alternative or additional) patch to cover that. Patch to follow...

Thanks!

John

John W. Linville
[email protected]

Some PCI devices lose all configuration (including BARs) when transitioning from D3hot->D0. This leaves such a device in an inaccessible state. The patch below causes the BARs to be restored when enabling such a device, so that its driver will be able to access it.

---
Some firmware leaves devices in D3hot after a (re)boot. Most drivers call pci_enable_device very early, so devices left in D3hot that lose configuration during the D3hot->D0 transition will be inaccessible to their drivers.

Drivers could be modified to account for this, but it would be difficult to know which drivers need modification. This is especially true since often many devices are covered by the same driver. It likely would be necessary to replicate code across dozens of drivers.

The patch below should trigger only when transitioning from D3hot->D0 (or at boot), and only for devices that have the "no soft reset" bit cleared in the PM control register. I believe it is safe to include as part of the PCI infrastructure.

 drivers/pci/pci.c   |   74 +++++++++++++++++++++++++++++++++++++++++++++++++---
 include/linux/pci.h |    1 
 2 files changed, 71 insertions(+), 4 deletions(-)

@@ -222,6 +222,59 @@ pci_find_parent_resource(const struct pc
 }
 
 /**
+ * pci_restore_bars - restore a devices BAR values (e.g. after wake-up)
+ * @dev: PCI device to have its BARs restored
+ *
+ * Restore the BAR values for a given device, so as to make it
+ * accessible by its driver.
+ */
+static void
+pci_restore_bars(struct pci_dev *dev)
+{
+	int i, numres;
+
+	switch (dev->hdr_type) {
+	case PCI_HEADER_TYPE_NORMAL:
+		numres = 6;
+		break;
+	case PCI_HEADER_TYPE_BRIDGE:
+		numres = 2;
+		break;
+	case PCI_HEADER_TYPE_CARDBUS:
+		numres = 1;
+		break;
+	default:
+		/* Should never get here, but just in case... */
+		return;
+	}
+
+	for (i = 0; i < numres; i ++) {
+		struct pci_bus_region region;
+		u32 val;
+		int reg;
+
+		if (!dev->resource[i].flags)
+			continue;
+
+		pcibios_resource_to_bus(dev, &region, &dev->resource[i]);
+
+		val = region.start
+		    | (dev->resource[i].flags & PCI_REGION_FLAG_MASK);
+
+		reg = PCI_BASE_ADDRESS_0 + (i * 4);
+
+		pci_write_config_dword(dev, reg, val);
+
+		if ((val & (PCI_BASE_ADDRESS_SPACE
+		          | PCI_BASE_ADDRESS_MEM_TYPE_MASK))
+		 == (PCI_BASE_ADDRESS_SPACE_MEMORY
+		   | PCI_BASE_ADDRESS_MEM_TYPE_64)) {
+			pci_write_config_dword(dev, reg + 4, 0);
+		}
+	}
+}
+
+/**
  * pci_set_power_state - Set the power state of a PCI device
  * @dev: PCI device to be suspended
  * @state: PCI power state (D0, D1, D2, D3hot, D3cold) we're entering

@@ -239,7 +292,7 @@ pci_find_parent_resource(const struct pc
 int
 pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 {
-	int pm;
+	int pm, need_restore = 0;
 	u16 pmcsr, pmc;
 
 	/* bound the state we're entering */

@@ -278,14 +331,17 @@ pci_set_power_state(struct pci_dev *dev,
 			return -EIO;
 	}
 
+	pci_read_config_word(dev, pm + PCI_PM_CTRL, &pmcsr);
+
 	/* If we're in D3, force entire word to 0.
 	 * This doesn't affect PME_Status, disables PME_En, and
 	 * sets PowerState to 0.
 	 */
-	if (dev->current_state >= PCI_D3hot)
+	if (dev->current_state >= PCI_D3hot) {
+		if (!(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET))
+			need_restore = 1;
 		pmcsr = 0;
-	else {
-		pci_read_config_word(dev, pm + PCI_PM_CTRL, &pmcsr);
+	} else {
 		pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
 		pmcsr |= state;
 	}

@@ -301,6 +357,16 @@ pci_set_power_state(struct pci_dev *dev,
 		udelay(200);
 	dev->current_state = state;
 
+	/* According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT
+	 * INTERFACE SPECIFICATION, REV. 1.2", a device transitioning
+	 * from D3hot to D0 _may_ perform an internal reset, thereby
+	 * going to "D0 Uninitialized" rather than "D0 Initialized".
+	 * In that case, we need to restore at least the BARs so that
+	 * the device will be accessible to its driver.
+	 */
+	if (need_restore)
+		pci_restore_bars(dev);
+
 	return 0;
 }
 

@@ -225,6 +225,7 @@
 #define  PCI_PM_CAP_PME_D3cold  0x8000  /* PME# from D3 (cold) */
 #define PCI_PM_CTRL		4	/* PM control and status register */
 #define  PCI_PM_CTRL_STATE_MASK	0x0003	/* Current power state (D0 to D3) */
+#define  PCI_PM_CTRL_NO_SOFT_RESET	0x0004	/* No reset for D3hot->D0 */
 #define  PCI_PM_CTRL_PME_ENABLE	0x0100	/* PME pin enable */
 #define  PCI_PM_CTRL_DATA_SEL_MASK	0x1e00	/* Data select (??) */
 #define  PCI_PM_CTRL_DATA_SCALE_MASK	0x6000	/* Data scale (??) */

John W. Linville
[email protected]

Hmm, I wonder if I missed something in previous email, but exactly why isn't this the responsibility of the driver for that device? It's only one of several similar issues, and not necessarily the dominant one.

We had to address this D3hot->D0uninitialized issue for various USB HCDs, in conjunction with similar problems wherein BIOS or swsusp may also have stuck their nasty little fingers in the middle of the power state transitions. (And similarly, the variability of system sleep states putting a USB controller into D3hot or D3cold... not always with system wakeup capabilities.)

There, it was relatively straightforward to NOT involve the PCI layer; and given the complications with BIOS, "legacy PCI" hardware (without PCI PM support), and swsusp (plus different types of hardware support even for hardware that does support PCI PM) more or less essential not to do so. Though to be sure, it did involve PCI-specific usbcore glue code in hcd-pci.c; the PCI PM framework seemed to maybe expect less variation in system behavior than seems routine with USB controllers. But all that's just to condition things so the HCDs more or less see a limited and sane set of states in their resume() methods.

- Dave

p.s. Until I sort out some mailer issues, it seems like my email
is getting filtered from many lists; remember that for followups.

It certainly could be handled that way. Of course, that could mean replicating essentially identical code across dozens (or more) drivers. Plus, many of those drivers might only need such changes for one variation of a device or for devices under a handful of BIOSen. Those drivers probably won't get fixed anytime soon unless some kernel hacker happens to stumble into such a situation. In the meantime, those drivers fail to work when they "should" be working.

In my mind, this is a documented behaviour that should not be unexpected. It is not some random quirk of some oddball device. A few simple steps taken when this situation is recognized can allow drivers to remain unaware of this detail of PCI PM. That seems worthwhile to me.

John

John W. Linville
[email protected]

From: "John W. Linville" <[email protected]>
Date: Thu, 7 Jul 2005 20:57:04 -0400

Yes, the drivers/pci/setup-res.c code isn't compiled in on sparc64 because it assumes a totally different model of PCI bus probing than we use on sparc64.

On sparc64, we check out if the boot firmware has assigned the PCI resources for the device, and if so we just leave things alone. We also make sure that the firmware device tree properties mostly match what the PCI config space registers say and we spit out a warning message if not.

Why not just implement sparc64 version of pci_update_resource elsewhere (perhaps a dummy one, if you don't need PCI PM), rather than force the rest of the world to duplicate the code?

Ivan.

From: Ivan Kokshaysky <[email protected]>
Date: Fri, 8 Jul 2005 09:51:04 +0400

That's fine, what would be the most minimal implementation?

#define pci_update_resource(dev, res, n) BUG()

No, I'm serious - I don't believe that generic implementation of pcibios_resource_to_bus() in the proposed patch does the right thing on sparc64 anyway.

Ivan.

From: Ivan Kokshaysky <[email protected]>
Date: Fri, 8 Jul 2005 11:03:58 +0400

We really don't do power management on sparc64 at all currently, besides cpufreq, so I guess this would be OK.

Do PCI devices ever come out of reset in a PM state, and thus would execute John's new code as a side effect?

PCI spec requires that all devices must enter D0 state from power on reset, so this code shouldn't be executed unless you have some really buggy PM firmware (which is unlikely :-).

Ivan.

Some PCI devices lose all configuration (including BARs) when transitioning from D3hot->D0. This leaves such a device in an inaccessible state. The patch below causes the BARs to be restored when enabling such a device, so that its driver will be able to access it.

---
Some firmware leaves devices in D3hot after a (re)boot. Most drivers call pci_enable_device very early, so devices left in D3hot that lose configuration during the D3hot->D0 transition will be inaccessible to their drivers.

Drivers could be modified to account for this, but it would be difficult to know which drivers need modification. This is especially true since often many devices are covered by the same driver. It likely would be necessary to replicate code across dozens of drivers.

The patch below should trigger only when transitioning from D3hot->D0 (or at boot), and only for devices that have the "no soft reset" bit cleared in the PM control register. I believe it is safe to include as part of the PCI infrastructure.

The cleanest implementation of pci_restore_bars was to call pci_update_resource. Unfortunately, that does not currently exist for the sparc64 architecture. The patch below includes a stub implemenation of pci_update_resource for sparc64.

 arch/sparc64/kernel/pci.c |    6 +++++
 drivers/pci/pci.c         |   52 ++++++++++++++++++++++++++++++++++++++++++----
 drivers/pci/setup-res.c   |    2 -
 include/linux/pci.h       |    2 +
 4 files changed, 57 insertions(+), 5 deletions(-)

@@ -413,6 +413,12 @@ static int pci_assign_bus_resource(const
 	return -EBUSY;
 }
 
+void pci_update_resource(struct pci_dev *dev, struct resource *res, int resno)
+{
+	/* Not implemented for sparc64... */
+	BUG();
+}
+
 int pci_assign_resource(struct pci_dev *pdev, int resource)
 {
 	struct pcidev_cookie *pcp = pdev->sysdata;

@@ -26,7 +26,7 @@
 #include "pci.h"
 
 
-static void
+void
 pci_update_resource(struct pci_dev *dev, struct resource *res, int resno)
 {
 	struct pci_bus_region region;

@@ -225,6 +225,7 @@
 #define  PCI_PM_CAP_PME_D3cold  0x8000  /* PME# from D3 (cold) */
 #define PCI_PM_CTRL		4	/* PM control and status register */
 #define  PCI_PM_CTRL_STATE_MASK	0x0003	/* Current power state (D0 to D3) */
+#define  PCI_PM_CTRL_NO_SOFT_RESET	0x0004	/* No reset for D3hot->D0 */
 #define  PCI_PM_CTRL_PME_ENABLE	0x0100	/* PME pin enable */
 #define  PCI_PM_CTRL_DATA_SEL_MASK	0x1e00	/* Data select (??) */
 #define  PCI_PM_CTRL_DATA_SCALE_MASK	0x6000	/* Data scale (??) */

@@ -816,6 +817,7 @@ int pci_set_mwi(struct pci_dev *dev);
 void pci_clear_mwi(struct pci_dev *dev);
 int pci_set_dma_mask(struct pci_dev *dev, u64 mask);
 int pci_set_consistent_dma_mask(struct pci_dev *dev, u64 mask);
+void pci_update_resource(struct pci_dev *dev, struct resource *res, int resno);
 int pci_assign_resource(struct pci_dev *dev, int i);
 
 /* ROM control related routines */

John W. Linville
[email protected]

From: "John W. Linville" <[email protected]>
Date: Fri, 8 Jul 2005 14:34:56 -0400

I have no problems with the sparc64 part.

Ivan.

It might be useful to have this functionality exported to outside of the generic PCI code.

There are a number of PCI boards that have their reset logic wired up wrong and lose their config space info (BARs) when you reset them. The Radisys ENP2611 PCI board is a good example -- it has its reset logic wired in such a way that if you reset the (ARM-based) CPU on the board, it also causes the 21555 nontransparent PCI bridge on the board to be reset, which makes it lose all its primary config space info (BARs, etc.) The IXP1200 CPU-based PCI cards (now obsolete) used to suffer from the same issue.

This is currently worked around in the driver, which caches all BAR values when the module is first loaded, and detects when the card is reset and then writes back all BARs manually.

--L

Fine by me...patch to follow...

John

John W. Linville
[email protected]

Globalize and add EXPORT_SYMBOL for pci_restore_bars.

---
Some have expressed interest in making general use of the the pci_restore_bars function.

 drivers/pci/pci.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

@@ -228,7 +228,7 @@ pci_find_parent_resource(const struct pc
  * Restore the BAR values for a given device, so as to make it
  * accessible by its driver.
  */
-static void
+void
 pci_restore_bars(struct pci_dev *dev)
 {
 	int i, numres;

@@ -833,6 +833,7 @@ struct pci_dev *isa_bridge;
 EXPORT_SYMBOL(isa_bridge);
 #endif
 
+EXPORT_SYMBOL(pci_restore_bars);
 EXPORT_SYMBOL(pci_enable_device_bars);
 EXPORT_SYMBOL(pci_enable_device);
 EXPORT_SYMBOL(pci_disable_device);

John W. Linville
[email protected]

EXPORT_SYMBOL_GPL() perhaps as this is a new function?

thanks,

greg k-h

Sure...that will please Arjan as well... :-)

Patch to follow...

John

John W. Linville
[email protected]

Globalize and add EXPORT_SYMBOL_GPL for pci_restore_bars.

---
Some have expressed interest in making general use of the the pci_restore_bars function.

Revised to use EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL.

 drivers/pci/pci.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

@@ -228,7 +228,7 @@ pci_find_parent_resource(const struct pc
  * Restore the BAR values for a given device, so as to make it
  * accessible by its driver.
  */
-static void
+void
 pci_restore_bars(struct pci_dev *dev)
 {
 	int i, numres;

@@ -833,6 +833,7 @@ struct pci_dev *isa_bridge;
 EXPORT_SYMBOL(isa_bridge);
 #endif
 
+EXPORT_SYMBOL_GPL(pci_restore_bars);
 EXPORT_SYMBOL(pci_enable_device_bars);
 EXPORT_SYMBOL(pci_enable_device);
 EXPORT_SYMBOL(pci_disable_device);

John W. Linville
[email protected]

Also, I think there is a possibility of only enabling boot devices for ACPI S4. However, for the reboot case, we're not restoring anything. Instead new resource assignments are being made. Doesn't the PCI subsystem already handle this?

Agreed.

I'm not sure I understand you...the kernel doesn't actually make the assignments, relying on the BIOS to do so...am I wrong?

So, if the BIOS leaves a device in D3hot, it will loose it's BIOS-assigned resources when it transitions to D0 in pci_enable_device. The point of this patch is to restore those BAR assignments so that the device's registers will be accessible to the driver. The driver remains blissfully unaware of the D3hot->D0 issue.

Currently pci_restore_state is only useful if there is a preceding pci_save_state. While this commonly occurs in the ->resume routines, most drivers don't do any such thing (i.e. either save or restore) in the ->probe routines. This is likely because it only makes sense to do so if you know about the D3hot->D0 issue; in that case, we would be back to replicating code in any number of drivers. I think we agree that should be avoided.

If you are asking for additional patches (or more changes to this one) then you'll have to be more specific. I don't know what you want here.

Thanks,

John

John W. Linville
[email protected]

Hmmm...you must be missing this hunk from the patch posted on July 8?

@@ -239,7 +270,7 @@ pci_find_parent_resource(const struct pc
int
pci_set_power_state(struct pci_dev *dev, pci_power_t state) {
- int pm;

+	int pm, need_restore = 0;
	u16 pmcsr, pmc;

	/* bound the state we're entering */

I'll be happy to do so, and include the other comment tweaks that Grant requested. I should get to it tomorrow morning.

Thanks,

John

John W. Linville
[email protected]

Looks like there was enough change between 8 July and now that patch (the utility) got confused. When I applied my 8 July patch against a current tree, it put the last hunk in some totally different function. This probably accounts for the compile failure you saw... :-)

New patch (w/ comment tweaks and symbol export) to follow...

John

John W. Linville
[email protected]

Some PCI devices (e.g. 3c905B, 3c556B) lose all configuration (including BARs) when transitioning from D3hot->D0. This leaves such a device in an inaccessible state. The patch below causes the BARs to be restored when enabling such a device, so that its driver will be able to access it.

The patch also adds pci_restore_bars as a new global symbol, and adds a correpsonding EXPORT_SYMBOL_GPL for that.

---
Some firmware (e.g. Thinkpad T21) leaves devices in D3hot after a (re)boot. Most drivers call pci_enable_device very early, so devices left in D3hot that lose configuration during the D3hot->D0 transition will be inaccessible to their drivers.

Drivers could be modified to account for this, but it would be difficult to know which drivers need modification. This is especially true since often many devices are covered by the same driver. It likely would be necessary to replicate code across dozens of drivers.

The patch below should trigger only when transitioning from D3hot->D0 (or at boot), and only for devices that have the "no soft reset" bit cleared in the PM control register. I believe it is safe to include this patch as part of the PCI infrastructure.

The cleanest implementation of pci_restore_bars was to call pci_update_resource. Unfortunately, that does not currently exist for the sparc64 architecture. The patch below includes a null implemenation of pci_update_resource for sparc64.

Some have expressed interest in making general use of the the pci_restore_bars function, so that has been exported to GPL licensed modules.

 arch/sparc64/kernel/pci.c |    6 ++++
 drivers/pci/pci.c         |   59 ++++++++++++++++++++++++++++++++++++++++++----
 drivers/pci/setup-res.c   |    2 -
 include/linux/pci.h       |    3 ++
 4 files changed, 65 insertions(+), 5 deletions(-)

@@ -413,6 +413,12 @@ static int pci_assign_bus_resource(const
 	return -EBUSY;
 }
 
+void pci_update_resource(struct pci_dev *dev, struct resource *res, int resno)
+{
+	/* Not implemented for sparc64... */
+	BUG();
+}
+
 int pci_assign_resource(struct pci_dev *pdev, int resource)
 {
 	struct pcidev_cookie *pcp = pdev->sysdata;

@@ -26,7 +26,7 @@
 #include "pci.h"
 
 
-static void
+void
 pci_update_resource(struct pci_dev *dev, struct resource *res, int resno)
 {
 	struct pci_bus_region region;

@@ -225,6 +225,7 @@
 #define  PCI_PM_CAP_PME_D3cold  0x8000  /* PME# from D3 (cold) */
 #define PCI_PM_CTRL		4	/* PM control and status register */
 #define  PCI_PM_CTRL_STATE_MASK	0x0003	/* Current power state (D0 to D3) */
+#define  PCI_PM_CTRL_NO_SOFT_RESET	0x0004	/* No reset for D3hot->D0 */
 #define  PCI_PM_CTRL_PME_ENABLE	0x0100	/* PME pin enable */
 #define  PCI_PM_CTRL_DATA_SEL_MASK	0x1e00	/* Data select (??) */
 #define  PCI_PM_CTRL_DATA_SCALE_MASK	0x6000	/* Data scale (??) */

@@ -816,7 +817,9 @@ int pci_set_mwi(struct pci_dev *dev);
 void pci_clear_mwi(struct pci_dev *dev);
 int pci_set_dma_mask(struct pci_dev *dev, u64 mask);
 int pci_set_consistent_dma_mask(struct pci_dev *dev, u64 mask);
+void pci_update_resource(struct pci_dev *dev, struct resource *res, int resno);
 int pci_assign_resource(struct pci_dev *dev, int i);
+void pci_restore_bars(struct pci_dev *dev);
 
 /* ROM control related routines */
 void __iomem *pci_map_rom(struct pci_dev *pdev, size_t *size);

John W. Linville
[email protected]

Tested with the 3com 3c556B Hurricane mini-PCI card in the IBM A21P. Without this patch the 3c59x driver has not been able to read the MAC address of the card's EEPROM with ACPI enabled, now it works with and without ACPI support. This patch should settle at least some of the issues in
http://bugzilla.kernel.org/show_bug.cgi?id=1188.

Ralf

Thanks for testing. I'm still going to hold off sending this in for 2.6.13 and wait for 2.6.14, unless people really think it should go in now.

greg k-h

Is it just me or will this stuff help the kexec guys as well? They seem to have lots of problems because drivers put the device into D3 before the reload of the new kernel. I think this might help.

Jesse

Certain (SGI?) ia64 boxes object to having their PCI BARs restored unless absolutely necessary. This patch restricts calling pci_restore_bars from pci_set_power_state unless the current state is PCI_UNKNOWN, the actual (i.e. physical) state of the device is PCI_D3hot, and the device indicates that it will lose its configuration when transitioning to PCI_D0.

---
Many thanks to Keith Owens <[email protected]> for a) narrowing-down the problem; and, b) quickly testing the fix and reporting the results.

 drivers/pci/pci.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

@@ -309,17 +309,25 @@ pci_set_power_state(struct pci_dev *dev,
 
 	pci_read_config_word(dev, pm + PCI_PM_CTRL, &pmcsr);
 
-	/* If we're in D3, force entire word to 0.
+	/* If we're (effectively) in D3, force entire word to 0.
 	 * This doesn't affect PME_Status, disables PME_En, and
 	 * sets PowerState to 0.
 	 */
-	if (dev->current_state >= PCI_D3hot) {
-		if (!(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET))
+	switch (dev->current_state) {
+	case PCI_UNKNOWN: /* Boot-up */
+		if ((pmcsr & PCI_PM_CTRL_STATE_MASK) == PCI_D3hot
+		 && !(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET))
 			need_restore = 1;
+		/* Fall-through: force to D0 */
+	case PCI_D3hot:
+	case PCI_D3cold:
+	case PCI_POWER_ERROR:
 		pmcsr = 0;
-	} else {
+		break;
+	default:
 		pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
 		pmcsr |= state;
+		break;
 	}
 
 	/* enter specified state */

From: Jeff Garzik <[email protected]>
Date: Wed, 14 Sep 2005 11:08:12 -0400

I wasn't going to say anything about this ia64 workaround, but yes I have to agree with Jeff, this change starts to lose the whole point of the original change.

Why in the world can a PCI device not handle it's BARs being rewritten, especially if we're just rewriting the same exact values it had when we probed it beforehand?

IA64 could handle the necessary cases in it's PCI config space access methods. Ugly, but keeps the core clean and limits the avoidance to the cases that really truly cannot handle the BAR rewrites.

Those cases are handled by the driver calling pci_restore_state after calling pci_set_power_state(..., PCI_D0).

The only time need_restore is actually needed is when the device is first accessed after boot (signified by PCI_UNKNOWN). When PCI drivers load, they typically call pci_enable_device before doing anything else. pci_enable_device calls pci_set_power_state(..., PCI_D0), which exposes the device to potentially become uninitialized if it had previously been left in PCI_D3hot. Any other time pci_set_power_state(..., PCI_D0) is called, drivers know to call (and can call) pci_restore_state afterwards.

If not calling pci_restore_bars from pci_set_power_state during normal transitions from PCI_D3hot was a problem, it would have been a problem long before the pci_restore_bars patch came along in 2.6.14-rc1. :-)

John

John W. Linville
[email protected]

Definitely. I wonder whether pci_resource_to_bus() works correctly on this platform.

Ivan.

I don't really have a list. The devices that brought this issue to my attention are a 3c905B and a 3c556B, both covered by the 3c59x driver.

According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT INTERFACE SPECIFICATION, REV. 1.2", a device transitioning from D3hot to D0 _may_ perform an internal reset, thereby going to "D0 Uninitialized" rather than "D0 Initialized". Since this behaviour is ratified by the spec, I think we need to accomodate it.

A bit in the PMCSR register indicates how a device will behave in this regard. We could have a test to only execute the BAR restoration for those devices that seem to need it. I left that out because it seemed to add needless complexity. In the meantime the patch has gotten bigger and more complex, so maybe that code doesn't make it any worse. Do you want me to add that?

The Thinkpad T21 does this. I don't know of any others specifically, but it seems like something laptop BIOSes would be likely to do.

As RMK pointed-out, this code is inspired by setup-res.c; specifically that in pci_update_resource. I'd prefer not to blaze any new trails regarding 64-bit BAR support ATM... :-)

So, is the current patch acceptable? Or shall I add the check for the "no soft reset" bit in the PMCSR register?

Thanks,

John

John W. Linville
[email protected]

John,
apologies for the late reply - been offline the past two weeks on holiday.

Just listing the two devices in a comment would be sufficient.

Including the above paragraph in a comment would be a good thing. I don't know if this spec is publicly available. But even if it is, typically only a handful of people will be familiar enough with it to know where to look in it.

Yes - sounds reasonable to me too.

I think I'd keep it simpler until someone proves we need it. I've read the rest of the thread and don't recall any such proof.

That's fine - just listing the Thinkpad T21 in a comment is helpful. If you happen to know the firmware version too, that would be even better.

After thinking about this more, I'm convinced it's broken if a 64-bit BAR is present on the PCI device. It doesn't matter if the MMIO value is greater than 4GB or not. The problem is pci_dev->resource[i] does NOT map 1:1 with PCI_BASE_ADDRESS_0+(i*4).

I don't think so. 64-bit BARs are just too common today.
One solution is to use a seperate variable to track the offset into PCI config space. ie use "i" to walk through pci_dev->resource[] and add "unsigned int pcibar_offset" to keep track of 32 vs 64-bit BARs.

I don't see why that's necessary.

thanks,
grant

Some PCI devices lose all configuration (including BARs) when transitioning from D3hot->D0. This leaves such a device in an inaccessible state. The patch below causes the BARs to be restored when enabling a device, so that the driver will be able to access it.

---
Some firmware leaves devices in D3hot after a (re)boot. Most drivers call pci_enable_device very early, so devices left in D3hot that lose configuration during the D3hot->D0 transition will be inaccessible to their drivers.

Drivers could be modified to account for this, but it would be difficult to know which drivers need modification. This is especially true since often many devices are covered by the same driver. It likely would be necessary to replicate code across dozens of drivers.

Since the fix should be safe for any device, it seems appropriate to make it part of the PCI infrastructure.

 drivers/pci/pci.c |   49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 48 insertions(+), 1 deletion(-)

@@ -378,9 +378,56 @@ pci_restore_state(struct pci_dev *dev)
 int
 pci_enable_device_bars(struct pci_dev *dev, int bars)
 {
-	int err;
+	int i, numres, err;
 
 	pci_set_power_state(dev, PCI_D0);
+
+	/* Some devices lose PCI config header data during D3hot->D0
+	   transition.	Since some firmware leaves devices in D3hot
+	   state at boot, this information needs to be restored.  We
+	   could force drivers to do this, but better to leave them
+	   ignorant of PCI PM trivia...
+	*/
+	switch (dev->hdr_type) {
+	case PCI_HEADER_TYPE_NORMAL:
+		numres = 6;
+		break;
+	case PCI_HEADER_TYPE_BRIDGE:
+		numres = 2;
+		break;
+	case PCI_HEADER_TYPE_CARDBUS:
+		numres = 1;
+		break;
+	default:
+		/* Should never get here, but just in case... */
+		numres = 0;
+		break;
+	}
+	for (i = 0; i < numres; i ++) {
+		struct pci_bus_region region;
+		u32 val;
+		int reg;
+
+		if (!dev->resource[i].flags)
+			continue;
+
+		pcibios_resource_to_bus(dev, &region, &dev->resource[i]);
+
+		val = region.start
+		    | (dev->resource[i].flags & PCI_REGION_FLAG_MASK);
+
+		reg = PCI_BASE_ADDRESS_0 + (i * 4);
+
+		pci_write_config_dword(dev, reg, val);
+
+		if ((val & (PCI_BASE_ADDRESS_SPACE
+		          | PCI_BASE_ADDRESS_MEM_TYPE_MASK))
+		 == (PCI_BASE_ADDRESS_SPACE_MEMORY
+		   | PCI_BASE_ADDRESS_MEM_TYPE_64)) {
+			pci_write_config_dword(dev, reg + 4, 0);
+		}
+	}
+
 	if ((err = pcibios_enable_device(dev, bars)) < 0)
 		return err;
 	return 0;

John W. Linville
[email protected]