[RFC] firmware leaves device in D3hot at boot

[RFC] firmware leaves device in D3hot at boot

[John W. Linville]

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?

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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;
 }
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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
linville@tuxdriver.com

Re: [RFC] firmware leaves device in D3hot at boot

[John W. Linville]

On Thu, Jun 23, 2005 at 03:14:52PM -0400, John W. Linville wrote:

> This issue regarding D3hot->D0 state transitions seems like a piece
> of minutiae that we should not force individual drivers to address.

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

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.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
linville@tuxdriver.com

Re: [RFC] firmware leaves device in D3hot at boot

[Greg KH]

On Thu, Jun 23, 2005 at 10:28:07PM -0400, John W. Linville wrote:
> On Thu, Jun 23, 2005 at 03:14:52PM -0400, John W. Linville wrote:
> 
> > This issue regarding D3hot->D0 state transitions seems like a piece
> > of minutiae that we should not force individual drivers to address.
> 
> 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.

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

> 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.

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

thanks,

greg k-h

Re: [RFC] firmware leaves device in D3hot at boot

[John W. Linville]

On Thu, Jun 30, 2005 at 10:10:10AM -0700, Greg KH wrote:
> On Thu, Jun 23, 2005 at 10:28:07PM -0400, John W. Linville wrote:
> > On Thu, Jun 23, 2005 at 03:14:52PM -0400, John W. Linville wrote:
> > 
> > > This issue regarding D3hot->D0 state transitions seems like a piece
> > > of minutiae that we should not force individual drivers to address.
> > 
> > After some thought, I'm inclined to think that the patch below is
> > the right one.  It unconditionally saves and restores the PCI config
 
> But how does this solve your problem with the state change?

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
linville@tuxdriver.com

[patch 2.6.12] pci: restore BAR values in pci_enable_device

[John W. Linville]

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.

Acked-by: Adam Belay <ambx1@neo.rr.com>
Acked-by: Russell King <rmk@arm.linux.org.uk>
Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
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(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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
linville@tuxdriver.com

[patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars

[John W. Linville]

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.

Acked-by: Adam Belay <ambx1@neo.rr.com>
Acked-by: Russell King <rmk@arm.linux.org.uk>
Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
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(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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
linville@tuxdriver.com

Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars

[Grant Grundler]

On Thu, Jun 30, 2005 at 10:26:37PM -0400, John W. Linville wrote:
...
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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

Can you name some of those devices here?
I just want to know what sort of devices need to be tested 
if this code changes in the future.

> +	   transition.	Since some firmware leaves devices in D3hot
> +	   state at boot, this information needs to be restored.

Again, which firmware?
Examples are good since it makes it possible to track down
the offending devices for testing.

The following chunk looks like it will have issues with 64-bit BARs:
...
> +	for (i = 0; i < numres; i ++) {
> +		struct pci_bus_region region;
> +		u32 val;
> +		int reg;
...
> +		val = region.start
> +		    | (dev->resource[i].flags & PCI_REGION_FLAG_MASK);
> +
> +		reg = PCI_BASE_ADDRESS_0 + (i * 4);

ISTR dev->resource[i] doesn't necessarily correspond directly BAR[i].
If BAR0 is a 64-bit BAR, then dev->resource[1] will point at BAR2.

I'm not sure how to fix this since I'm not quite sure where
state is being saved off from.

> +		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);

64-bit BARs need the upper half of dev->resource[] written.
I expect something like:
		val = region.start >> 4;
		pci_write_config_dword(dev, reg + 4, val);

This should put zero in the upper 32-bit if it's only a 32-bit value.
I suspect "i" needs to be split into two indices: one for dev->resource[]
and another for offset into PCI config space (BARs).
But it really depends on how dev->resource[] maps to the BARs.

hth,
grant

> +		}
> +	}
> +
>  	if ((err = pcibios_enable_device(dev, bars)) < 0)
>  		return err;
>  	return 0;
> -- 
> John W. Linville
> linville@tuxdriver.com

Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars

[Russell King]

On Sat, Jul 02, 2005 at 01:29:54AM -0600, Grant Grundler wrote:
> On Thu, Jun 30, 2005 at 10:26:37PM -0400, John W. Linville wrote:
> ...
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -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
> 
> Can you name some of those devices here?
> I just want to know what sort of devices need to be tested 
> if this code changes in the future.
> 
> > +	   transition.	Since some firmware leaves devices in D3hot
> > +	   state at boot, this information needs to be restored.
> 
> Again, which firmware?
> Examples are good since it makes it possible to track down
> the offending devices for testing.
> 
> The following chunk looks like it will have issues with 64-bit BARs:
> ...
> > +	for (i = 0; i < numres; i ++) {
> > +		struct pci_bus_region region;
> > +		u32 val;
> > +		int reg;
> ...
> > +		val = region.start
> > +		    | (dev->resource[i].flags & PCI_REGION_FLAG_MASK);
> > +
> > +		reg = PCI_BASE_ADDRESS_0 + (i * 4);
> 
> ISTR dev->resource[i] doesn't necessarily correspond directly BAR[i].
> If BAR0 is a 64-bit BAR, then dev->resource[1] will point at BAR2.

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].

> > +		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);
> 
> 64-bit BARs need the upper half of dev->resource[] written.
> I expect something like:
> 		val = region.start >> 4;

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

Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars

[Matthew Wilcox]

On Sat, Jul 02, 2005 at 09:09:13AM +0100, Russell King wrote:
> 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.

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

Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars

[Russell King]

On Tue, Jul 05, 2005 at 09:05:55PM +0100, Matthew Wilcox wrote:
> On Sat, Jul 02, 2005 at 09:09:13AM +0100, Russell King wrote:
> > 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.
> 
> 64-bit BARs work fine on 64-bit machines.  I'm ambivalent whether we
> ought to support 64-bit BARs on 32-bit machines.

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

Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars

[Ivan Kokshaysky]

On Tue, Jul 05, 2005 at 10:46:20PM +0100, Russell King wrote:
> On Tue, Jul 05, 2005 at 09:05:55PM +0100, Matthew Wilcox wrote:
> > 64-bit BARs work fine on 64-bit machines.  I'm ambivalent whether we
> > ought to support 64-bit BARs on 32-bit machines.
> 
> 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?

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.

> 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().

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.

Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars

[Russell King]

On Wed, Jul 06, 2005 at 03:34:54AM +0400, Ivan Kokshaysky wrote:
> On Tue, Jul 05, 2005 at 10:46:20PM +0100, Russell King wrote:
> > On Tue, Jul 05, 2005 at 09:05:55PM +0100, Matthew Wilcox wrote:
> > > 64-bit BARs work fine on 64-bit machines.  I'm ambivalent whether we
> > > ought to support 64-bit BARs on 32-bit machines.
> > 
> > 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?
> 
> 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.

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

Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars

[John W. Linville]

On Wed, Jul 06, 2005 at 03:34:54AM +0400, Ivan Kokshaysky wrote:
> On Tue, Jul 05, 2005 at 10:46:20PM +0100, Russell King wrote:

> > 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().
> 
> Just use pci_update_resource().
 
Problem: pci_update_resource doesn't exist for sparc64.

> 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.

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
linville@tuxdriver.com

[patch 2.6.13-rc2] pci: restore BAR values in pci_set_power_state for D3hot->D0

[John W. Linville]

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.

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
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(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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;
 }
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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
linville@tuxdriver.com

Re: [linux-pm] [patch 2.6.13-rc2] pci: restore BAR values in pci_set_power_state for D3hot->D0

[david-b]

> 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.

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.

Re: [linux-pm] [patch 2.6.13-rc2] pci: restore BAR values in pci_set_power_state for D3hot->D0

[John W. Linville]

On Thu, Jul 07, 2005 at 08:43:02PM -0700, david-b@pacbell.net wrote:
> > Some PCI devices lose all configuration (including BARs) when
> > transitioning from D3hot->D0.  This leaves such a device in an

> 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 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
linville@tuxdriver.com

Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars

[David S. Miller]

From: "John W. Linville" <linville@tuxdriver.com>
Date: Thu, 7 Jul 2005 20:57:04 -0400

> Problem: pci_update_resource doesn't exist for sparc64.

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.

Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars

[Ivan Kokshaysky]

On Thu, Jul 07, 2005 at 08:11:03PM -0700, David S. Miller wrote:
> > Problem: pci_update_resource doesn't exist for sparc64.
> 
> 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.

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.

Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars

[David S. Miller]

From: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Date: Fri, 8 Jul 2005 09:51:04 +0400

> 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?

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

Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars

[Ivan Kokshaysky]

On Thu, Jul 07, 2005 at 11:35:30PM -0700, David S. Miller wrote:
> 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.

Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars

[David S. Miller]

From: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Date: Fri, 8 Jul 2005 11:03:58 +0400

> On Thu, Jul 07, 2005 at 11:35:30PM -0700, David S. Miller wrote:
> > 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.

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?

Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars

[Ivan Kokshaysky]

On Fri, Jul 08, 2005 at 12:33:33AM -0700, David S. Miller wrote:
> 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.

[patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0

[John W. Linville]

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.

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
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(-)

diff --git a/arch/sparc64/kernel/pci.c b/arch/sparc64/kernel/pci.c
--- a/arch/sparc64/kernel/pci.c
+++ b/arch/sparc64/kernel/pci.c
@@ -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;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -222,6 +222,37 @@ 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 ++)
+		pci_update_resource(dev, &dev->resource[i], i);
+}
+
+/**
  * 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 +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 */
@@ -278,14 +309,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 +335,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;
 }
 
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -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;
diff --git a/include/linux/pci.h b/include/linux/pci.h
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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
linville@tuxdriver.com

Re: [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0

[David S. Miller]

From: "John W. Linville" <linville@tuxdriver.com>
Date: Fri, 8 Jul 2005 14:34:56 -0400

> 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.

I have no problems with the sparc64 part.

Re: [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0

[Ivan Kokshaysky]

On Fri, Jul 08, 2005 at 02:34:56PM -0400, John W. Linville wrote:
> 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.
> 
> Signed-off-by: John W. Linville <linville@tuxdriver.com>

Acked-by: Ivan Kokshaysky <ink@jurassic.park.msu.ru>

Ivan.

Re: [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0

[Lennert Buytenhek]

On Fri, Jul 08, 2005 at 02:34:56PM -0400, John W. Linville wrote:

> 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.

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

Re: [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0

[John W. Linville]

On Mon, Jul 11, 2005 at 02:48:44PM +0200, Lennert Buytenhek wrote:
> On Fri, Jul 08, 2005 at 02:34:56PM -0400, John W. Linville wrote:
> 
> > 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.
> 
> It might be useful to have this functionality exported to outside of
> the generic PCI code.

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

John
-- 
John W. Linville
linville@tuxdriver.com

[patch 2.6.13-rc2] PCI: Add symbol exports for pci_restore_bars

[John W. Linville]

Globalize and add EXPORT_SYMBOL for pci_restore_bars.

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
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(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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
linville@tuxdriver.com

Re: [patch 2.6.13-rc2] PCI: Add symbol exports for pci_restore_bars

[Greg KH]

On Mon, Jul 11, 2005 at 09:19:03AM -0400, John W. Linville wrote:
> Globalize and add EXPORT_SYMBOL for pci_restore_bars.

EXPORT_SYMBOL_GPL() perhaps as this is a new function?

thanks,

greg k-h

Re: [patch 2.6.13-rc2] PCI: Add symbol exports for pci_restore_bars

[John W. Linville]

On Mon, Jul 11, 2005 at 10:18:30AM -0700, Greg KH wrote:
> On Mon, Jul 11, 2005 at 09:19:03AM -0400, John W. Linville wrote:
> > Globalize and add EXPORT_SYMBOL for pci_restore_bars.
> 
> EXPORT_SYMBOL_GPL() perhaps as this is a new function?

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

Patch to follow...

John
-- 
John W. Linville
linville@tuxdriver.com

[patch 2.6.13-rc2] PCI: Add GPL symbol export for pci_restore_bars

[John W. Linville]

Globalize and add EXPORT_SYMBOL_GPL for pci_restore_bars.

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
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(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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
linville@tuxdriver.com

Re: [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0

[Adam Belay]

On Fri, Jul 08, 2005 at 02:34:56PM -0400, John W. Linville wrote:
> 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.
> 
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> ---
> 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.

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?

> 
> 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.

Agreed.

> 
> 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.


>   * 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 +270,7 @@ pci_find_parent_resource(const struct pc
>  int
>  pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>  {

Couldn't this be in pci_restore_state() instead?  I was thinking it would
(in part) replace the ugly dword reads we have now.  They include many
registers we don't need to touch.  I wonder if we'll need pci_save_state()
at all or if we can derive all the information from the pci_dev.  I'll have
to look into it further.

Also we need a way to restore specific PCI capabilities.

Thanks,
Adam

Re: [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0

[John W. Linville]

On Mon, Jul 11, 2005 at 10:28:55PM -0400, Adam Belay wrote:
> On Fri, Jul 08, 2005 at 02:34:56PM -0400, John W. Linville wrote:

> > 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.
> 
> 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?

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.

> >   * 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 +270,7 @@ pci_find_parent_resource(const struct pc
> >  int
> >  pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> >  {
> 
> Couldn't this be in pci_restore_state() instead?  I was thinking it would
> (in part) replace the ugly dword reads we have now.  They include many
> registers we don't need to touch.  I wonder if we'll need pci_save_state()
> at all or if we can derive all the information from the pci_dev.  I'll have
> to look into it further.
 
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.

> Also we need a way to restore specific PCI capabilities.

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
linville@tuxdriver.com

Re: [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0

[Greg KH]

On Fri, Jul 08, 2005 at 02:34:56PM -0400, John W. Linville wrote:
> @@ -301,6 +335,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);
> +

This code doesn't even build, as need_restore isn't a global variable.

Care to redo this patch (and merge it with your other one) and resend
it?

thanks,

greg k-h

Re: [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0

[John W. Linville]

On Tue, Jul 26, 2005 at 04:49:34PM -0700, Greg KH wrote:
> On Fri, Jul 08, 2005 at 02:34:56PM -0400, John W. Linville wrote:
> > @@ -301,6 +335,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);
> > +
> 
> This code doesn't even build, as need_restore isn't a global variable.

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 */

 
> Care to redo this patch (and merge it with your other one) and resend
> it?

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
linville@tuxdriver.com

Re: [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0

[John W. Linville]

On Tue, Jul 26, 2005 at 09:36:02PM -0400, John W. Linville wrote:
> On Tue, Jul 26, 2005 at 04:49:34PM -0700, Greg KH wrote:

> > This code doesn't even build, as need_restore isn't a global variable.
> 
> Hmmm...you must be missing this hunk from the patch posted on July 8?

> > Care to redo this patch (and merge it with your other one) and resend
> > it?
> 
> I'll be happy to do so, and include the other comment tweaks that
> Grant requested.  I should get to it tomorrow morning.

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
linville@tuxdriver.com

[patch 2.6.13-rc3] pci: restore BAR values after D3hot->D0 for devices that need it

[John W. Linville]

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.

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
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(-)

diff --git a/arch/sparc64/kernel/pci.c b/arch/sparc64/kernel/pci.c
--- a/arch/sparc64/kernel/pci.c
+++ b/arch/sparc64/kernel/pci.c
@@ -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;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -222,6 +222,37 @@ 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.
+ */
+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 ++)
+		pci_update_resource(dev, &dev->resource[i], i);
+}
+
+/**
  * 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 +270,7 @@ int (*platform_pci_set_power_state)(stru
 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 +309,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;
 	}
@@ -308,6 +342,22 @@ pci_set_power_state(struct pci_dev *dev,
 		platform_pci_set_power_state(dev, state);
 
 	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".
+	 * For example, at least some versions of the 3c905B and the
+	 * 3c556B exhibit this behaviour.
+	 *
+	 * At least some laptop BIOSen (e.g. the Thinkpad T21) leave
+	 * devices in a D3hot state at boot.  Consequently, 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;
 }
 
@@ -805,6 +855,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);
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -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;
diff --git a/include/linux/pci.h b/include/linux/pci.h
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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
linville@tuxdriver.com

Re: [patch 2.6.13-rc3] pci: restore BAR values after D3hot->D0 for devices that need it

[Ralf Baechle]

On Wed, Jul 27, 2005 at 10:19:44AM -0400, John W. Linville wrote:

> 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.
> 
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> ---
> 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.

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

Re: [patch 2.6.13-rc3] pci: restore BAR values after D3hot->D0 for devices that need it

[Greg KH]

On Sun, Jul 31, 2005 at 08:36:53PM +0100, Ralf Baechle wrote:
> On Wed, Jul 27, 2005 at 10:19:44AM -0400, John W. Linville wrote:
> 
> > 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.
> > 
> > Signed-off-by: John W. Linville <linville@tuxdriver.com>
> > ---
> > 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.
> 
> 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.

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

Re: [patch 2.6.13-rc3] pci: restore BAR values after D3hot->D0 for devices that need it

[Jesse Brandeburg]

On 7/27/05, John W. Linville <linville@tuxdriver.com> wrote:
> 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.
> 

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

[patch 2.6.14-rc1] pci: only call pci_restore_bars at boot

[John W. Linville]

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.

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
Many thanks to Keith Owens <kaos@sgi.com> 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(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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 */

Re: [patch 2.6.14-rc1] pci: only call pci_restore_bars at boot

[Jeff Garzik]

John W. Linville wrote:
> 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.
> 
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> ---
> Many thanks to Keith Owens <kaos@sgi.com> 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(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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;

This seems like it will break a lot of stuff that -does- need the BARs 
restored when resuming from D3.

	Jeff

Re: [patch 2.6.14-rc1] pci: only call pci_restore_bars at boot

[David S. Miller]

From: Jeff Garzik <jgarzik@pobox.com>
Date: Wed, 14 Sep 2005 11:08:12 -0400

> This seems like it will break a lot of stuff that -does- need the BARs 
> restored when resuming from D3.

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.

Re: [patch 2.6.14-rc1] pci: only call pci_restore_bars at boot

[John W. Linville]

On Wed, Sep 14, 2005 at 09:26:50AM -0700, David S. Miller wrote:
> From: Jeff Garzik <jgarzik@pobox.com>
> Date: Wed, 14 Sep 2005 11:08:12 -0400
> 
> > This seems like it will break a lot of stuff that -does- need the BARs 
> > restored when resuming from D3.
> 
> 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.

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
linville@tuxdriver.com

Re: [patch 2.6.14-rc1] pci: only call pci_restore_bars at boot

[Ivan Kokshaysky]

On Wed, Sep 14, 2005 at 09:26:50AM -0700, David S. Miller wrote:
> 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?

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

Ivan.

Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars

[John W. Linville]

On Sat, Jul 02, 2005 at 01:29:54AM -0600, Grant Grundler wrote:
> On Thu, Jun 30, 2005 at 10:26:37PM -0400, John W. Linville wrote:

> > +	/* Some devices lose PCI config header data during D3hot->D0
> 
> Can you name some of those devices here?
> I just want to know what sort of devices need to be tested 
> if this code changes in the future.

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?

> 
> > +	   transition.	Since some firmware leaves devices in D3hot
> > +	   state at boot, this information needs to be restored.
> 
> Again, which firmware?
> Examples are good since it makes it possible to track down
> the offending devices for testing.

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.

> The following chunk looks like it will have issues with 64-bit BARs:

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
linville@tuxdriver.com

Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars

[Grant Grundler]

On Tue, Jul 05, 2005 at 01:46:20PM -0400, John W. Linville wrote:
> On Sat, Jul 02, 2005 at 01:29:54AM -0600, Grant Grundler wrote:
> > On Thu, Jun 30, 2005 at 10:26:37PM -0400, John W. Linville wrote:
> 
> > > +	/* Some devices lose PCI config header data during D3hot->D0
> > 
> > Can you name some of those devices here?
> > I just want to know what sort of devices need to be tested 
> > if this code changes in the future.
> 
> 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.

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.

> 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".

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.

> Since this behaviour is ratified by
> the spec, I think we need to accomodate it.

Yes - sounds reasonable to me too.

> 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?

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.

> 
> > 
> > > +	   transition.	Since some firmware leaves devices in D3hot
> > > +	   state at boot, this information needs to be restored.
> > 
> > Again, which firmware?
> > Examples are good since it makes it possible to track down
> > the offending devices for testing.
> 
> 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.

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.

> > The following chunk looks like it will have issues with 64-bit BARs:
> 
> 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... :-)

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).

> So, is the current patch acceptable?

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.

> Or shall I add the check for the "no soft reset" bit in the PMCSR register?

I don't see why that's necessary.

thanks,
grant