* [RFC] firmware leaves device in D3hot at boot @ 2005-06-23 19:14 John W. Linville 2005-06-24 2:28 ` John W. Linville 0 siblings, 1 reply; 47+ messages in thread From: John W. Linville @ 2005-06-23 19:14 UTC (permalink / raw) To: linux-pm, linux-pci; +Cc: linux-kernel, greg 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC] firmware leaves device in D3hot at boot 2005-06-23 19:14 [RFC] firmware leaves device in D3hot at boot John W. Linville @ 2005-06-24 2:28 ` John W. Linville 2005-06-30 17:10 ` Greg KH 0 siblings, 1 reply; 47+ messages in thread From: John W. Linville @ 2005-06-24 2:28 UTC (permalink / raw) To: linux-pm, linux-pci; +Cc: linux-kernel, greg 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC] firmware leaves device in D3hot at boot 2005-06-24 2:28 ` John W. Linville @ 2005-06-30 17:10 ` Greg KH 2005-07-01 1:41 ` John W. Linville 0 siblings, 1 reply; 47+ messages in thread From: Greg KH @ 2005-06-30 17:10 UTC (permalink / raw) To: linux-pm, linux-pci, linux-kernel 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [RFC] firmware leaves device in D3hot at boot 2005-06-30 17:10 ` Greg KH @ 2005-07-01 1:41 ` John W. Linville 2005-07-01 2:26 ` [patch 2.6.12] pci: restore BAR values in pci_enable_device John W. Linville 2005-07-01 2:26 ` [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars John W. Linville 0 siblings, 2 replies; 47+ messages in thread From: John W. Linville @ 2005-07-01 1:41 UTC (permalink / raw) To: Greg KH; +Cc: linux-pm, linux-pci, linux-kernel 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [patch 2.6.12] pci: restore BAR values in pci_enable_device 2005-07-01 1:41 ` John W. Linville @ 2005-07-01 2:26 ` John W. Linville 2005-07-01 2:26 ` [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars John W. Linville 1 sibling, 0 replies; 47+ messages in thread From: John W. Linville @ 2005-07-01 2:26 UTC (permalink / raw) To: linux-pci; +Cc: linux-pm, linux-kernel, Greg KH, Adam Belay, Russell King 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, ®ion, &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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars 2005-07-01 1:41 ` John W. Linville 2005-07-01 2:26 ` [patch 2.6.12] pci: restore BAR values in pci_enable_device John W. Linville @ 2005-07-01 2:26 ` John W. Linville 2005-07-02 7:29 ` Grant Grundler 1 sibling, 1 reply; 47+ messages in thread From: John W. Linville @ 2005-07-01 2:26 UTC (permalink / raw) To: linux-pci; +Cc: linux-pm, linux-kernel, Greg KH, Adam Belay, Russell King 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, ®ion, &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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars 2005-07-01 2:26 ` [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars John W. Linville @ 2005-07-02 7:29 ` Grant Grundler 2005-07-02 8:09 ` Russell King 2005-07-05 17:46 ` [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars John W. Linville 0 siblings, 2 replies; 47+ messages in thread From: Grant Grundler @ 2005-07-02 7:29 UTC (permalink / raw) To: John W. Linville Cc: linux-pci, linux-pm, linux-kernel, Greg KH, Adam Belay, Russell King 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars 2005-07-02 7:29 ` Grant Grundler @ 2005-07-02 8:09 ` Russell King 2005-07-05 20:05 ` Matthew Wilcox 2005-07-05 17:46 ` [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars John W. Linville 1 sibling, 1 reply; 47+ messages in thread From: Russell King @ 2005-07-02 8:09 UTC (permalink / raw) To: Grant Grundler Cc: John W. Linville, linux-pci, linux-pm, linux-kernel, Greg KH, Adam Belay 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars 2005-07-02 8:09 ` Russell King @ 2005-07-05 20:05 ` Matthew Wilcox 2005-07-05 21:46 ` Russell King 0 siblings, 1 reply; 47+ messages in thread From: Matthew Wilcox @ 2005-07-05 20:05 UTC (permalink / raw) To: Russell King Cc: Grant Grundler, John W. Linville, linux-pci, linux-pm, linux-kernel, Greg KH, Adam Belay 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars 2005-07-05 20:05 ` Matthew Wilcox @ 2005-07-05 21:46 ` Russell King 2005-07-05 23:34 ` Ivan Kokshaysky 0 siblings, 1 reply; 47+ messages in thread From: Russell King @ 2005-07-05 21:46 UTC (permalink / raw) To: Matthew Wilcox Cc: Grant Grundler, John W. Linville, linux-pci, linux-pm, linux-kernel, Greg KH, Adam Belay 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars 2005-07-05 21:46 ` Russell King @ 2005-07-05 23:34 ` Ivan Kokshaysky 2005-07-06 7:46 ` Russell King 2005-07-08 0:57 ` John W. Linville 0 siblings, 2 replies; 47+ messages in thread From: Ivan Kokshaysky @ 2005-07-05 23:34 UTC (permalink / raw) To: Russell King Cc: Matthew Wilcox, Grant Grundler, John W. Linville, linux-pci, linux-pm, linux-kernel, Greg KH, Adam Belay 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars 2005-07-05 23:34 ` Ivan Kokshaysky @ 2005-07-06 7:46 ` Russell King 2005-07-08 0:57 ` John W. Linville 1 sibling, 0 replies; 47+ messages in thread From: Russell King @ 2005-07-06 7:46 UTC (permalink / raw) To: Ivan Kokshaysky Cc: Matthew Wilcox, Grant Grundler, John W. Linville, linux-pci, linux-pm, linux-kernel, Greg KH, Adam Belay 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars 2005-07-05 23:34 ` Ivan Kokshaysky 2005-07-06 7:46 ` Russell King @ 2005-07-08 0:57 ` John W. Linville 2005-07-08 0:59 ` [patch 2.6.13-rc2] pci: restore BAR values in pci_set_power_state for D3hot->D0 John W. Linville 2005-07-08 3:11 ` [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars David S. Miller 1 sibling, 2 replies; 47+ messages in thread From: John W. Linville @ 2005-07-08 0:57 UTC (permalink / raw) To: Ivan Kokshaysky Cc: Russell King, Matthew Wilcox, Grant Grundler, linux-pci, linux-pm, linux-kernel, Greg KH, Adam Belay 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [patch 2.6.13-rc2] pci: restore BAR values in pci_set_power_state for D3hot->D0 2005-07-08 0:57 ` John W. Linville @ 2005-07-08 0:59 ` John W. Linville 2005-07-08 3:43 ` [linux-pm] " david-b 2005-07-08 3:11 ` [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars David S. Miller 1 sibling, 1 reply; 47+ messages in thread From: John W. Linville @ 2005-07-08 0:59 UTC (permalink / raw) To: Ivan Kokshaysky Cc: Russell King, Matthew Wilcox, Grant Grundler, linux-pci, linux-pm, linux-kernel, Greg KH, Adam Belay 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, ®ion, &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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [linux-pm] [patch 2.6.13-rc2] pci: restore BAR values in pci_set_power_state for D3hot->D0 2005-07-08 0:59 ` [patch 2.6.13-rc2] pci: restore BAR values in pci_set_power_state for D3hot->D0 John W. Linville @ 2005-07-08 3:43 ` david-b 2005-07-08 12:37 ` John W. Linville 0 siblings, 1 reply; 47+ messages in thread From: david-b @ 2005-07-08 3:43 UTC (permalink / raw) To: linville, ink Cc: rmk+lkml, matthew, linux-pm, linux-pci, linux-kernel, grundler > 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [linux-pm] [patch 2.6.13-rc2] pci: restore BAR values in pci_set_power_state for D3hot->D0 2005-07-08 3:43 ` [linux-pm] " david-b @ 2005-07-08 12:37 ` John W. Linville 0 siblings, 0 replies; 47+ messages in thread From: John W. Linville @ 2005-07-08 12:37 UTC (permalink / raw) To: david-b Cc: ink, rmk+lkml, matthew, linux-pm, linux-pci, linux-kernel, grundler 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars 2005-07-08 0:57 ` John W. Linville 2005-07-08 0:59 ` [patch 2.6.13-rc2] pci: restore BAR values in pci_set_power_state for D3hot->D0 John W. Linville @ 2005-07-08 3:11 ` David S. Miller 2005-07-08 5:51 ` Ivan Kokshaysky 1 sibling, 1 reply; 47+ messages in thread From: David S. Miller @ 2005-07-08 3:11 UTC (permalink / raw) To: linville Cc: ink, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, greg, ambx1 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars 2005-07-08 3:11 ` [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars David S. Miller @ 2005-07-08 5:51 ` Ivan Kokshaysky 2005-07-08 6:35 ` David S. Miller 0 siblings, 1 reply; 47+ messages in thread From: Ivan Kokshaysky @ 2005-07-08 5:51 UTC (permalink / raw) To: David S. Miller Cc: linville, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, greg, ambx1 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars 2005-07-08 5:51 ` Ivan Kokshaysky @ 2005-07-08 6:35 ` David S. Miller 2005-07-08 7:03 ` Ivan Kokshaysky 0 siblings, 1 reply; 47+ messages in thread From: David S. Miller @ 2005-07-08 6:35 UTC (permalink / raw) To: ink Cc: linville, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, greg, ambx1 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? ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars 2005-07-08 6:35 ` David S. Miller @ 2005-07-08 7:03 ` Ivan Kokshaysky 2005-07-08 7:33 ` David S. Miller 0 siblings, 1 reply; 47+ messages in thread From: Ivan Kokshaysky @ 2005-07-08 7:03 UTC (permalink / raw) To: David S. Miller Cc: linville, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, greg, ambx1 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars 2005-07-08 7:03 ` Ivan Kokshaysky @ 2005-07-08 7:33 ` David S. Miller 2005-07-08 8:20 ` Ivan Kokshaysky 0 siblings, 1 reply; 47+ messages in thread From: David S. Miller @ 2005-07-08 7:33 UTC (permalink / raw) To: ink Cc: linville, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, greg, ambx1 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? ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars 2005-07-08 7:33 ` David S. Miller @ 2005-07-08 8:20 ` Ivan Kokshaysky 2005-07-08 18:34 ` [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 John W. Linville 0 siblings, 1 reply; 47+ messages in thread From: Ivan Kokshaysky @ 2005-07-08 8:20 UTC (permalink / raw) To: David S. Miller Cc: linville, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, greg, ambx1 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 2005-07-08 8:20 ` Ivan Kokshaysky @ 2005-07-08 18:34 ` John W. Linville 2005-07-08 19:08 ` David S. Miller ` (4 more replies) 0 siblings, 5 replies; 47+ messages in thread From: John W. Linville @ 2005-07-08 18:34 UTC (permalink / raw) To: Ivan Kokshaysky Cc: David S. Miller, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, greg, ambx1 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 2005-07-08 18:34 ` [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 John W. Linville @ 2005-07-08 19:08 ` David S. Miller 2005-07-10 17:53 ` Ivan Kokshaysky ` (3 subsequent siblings) 4 siblings, 0 replies; 47+ messages in thread From: David S. Miller @ 2005-07-08 19:08 UTC (permalink / raw) To: linville Cc: ink, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, greg, ambx1 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 2005-07-08 18:34 ` [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 John W. Linville 2005-07-08 19:08 ` David S. Miller @ 2005-07-10 17:53 ` Ivan Kokshaysky 2005-07-11 12:48 ` Lennert Buytenhek ` (2 subsequent siblings) 4 siblings, 0 replies; 47+ messages in thread From: Ivan Kokshaysky @ 2005-07-10 17:53 UTC (permalink / raw) To: David S. Miller, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, greg, ambx1 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 2005-07-08 18:34 ` [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 John W. Linville 2005-07-08 19:08 ` David S. Miller 2005-07-10 17:53 ` Ivan Kokshaysky @ 2005-07-11 12:48 ` Lennert Buytenhek 2005-07-11 13:15 ` John W. Linville 2005-07-12 2:28 ` [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 Adam Belay 2005-07-26 23:49 ` Greg KH 4 siblings, 1 reply; 47+ messages in thread From: Lennert Buytenhek @ 2005-07-11 12:48 UTC (permalink / raw) To: Ivan Kokshaysky, David S. Miller, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, greg, ambx1 Cc: byjac, herbertb 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 2005-07-11 12:48 ` Lennert Buytenhek @ 2005-07-11 13:15 ` John W. Linville 2005-07-11 13:19 ` [patch 2.6.13-rc2] PCI: Add symbol exports for pci_restore_bars John W. Linville 0 siblings, 1 reply; 47+ messages in thread From: John W. Linville @ 2005-07-11 13:15 UTC (permalink / raw) To: Lennert Buytenhek Cc: Ivan Kokshaysky, David S. Miller, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, greg, ambx1, byjac, herbertb 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [patch 2.6.13-rc2] PCI: Add symbol exports for pci_restore_bars 2005-07-11 13:15 ` John W. Linville @ 2005-07-11 13:19 ` John W. Linville 2005-07-11 17:18 ` Greg KH 0 siblings, 1 reply; 47+ messages in thread From: John W. Linville @ 2005-07-11 13:19 UTC (permalink / raw) To: Lennert Buytenhek Cc: Ivan Kokshaysky, David S. Miller, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, greg, ambx1, byjac, herbertb 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.13-rc2] PCI: Add symbol exports for pci_restore_bars 2005-07-11 13:19 ` [patch 2.6.13-rc2] PCI: Add symbol exports for pci_restore_bars John W. Linville @ 2005-07-11 17:18 ` Greg KH 2005-07-11 17:36 ` John W. Linville 0 siblings, 1 reply; 47+ messages in thread From: Greg KH @ 2005-07-11 17:18 UTC (permalink / raw) To: John W. Linville Cc: Lennert Buytenhek, Ivan Kokshaysky, David S. Miller, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, ambx1, byjac, herbertb 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.13-rc2] PCI: Add symbol exports for pci_restore_bars 2005-07-11 17:18 ` Greg KH @ 2005-07-11 17:36 ` John W. Linville 2005-07-11 17:38 ` [patch 2.6.13-rc2] PCI: Add GPL symbol export " John W. Linville 0 siblings, 1 reply; 47+ messages in thread From: John W. Linville @ 2005-07-11 17:36 UTC (permalink / raw) To: Greg KH Cc: Lennert Buytenhek, Ivan Kokshaysky, David S. Miller, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, ambx1, byjac, herbertb 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [patch 2.6.13-rc2] PCI: Add GPL symbol export for pci_restore_bars 2005-07-11 17:36 ` John W. Linville @ 2005-07-11 17:38 ` John W. Linville 0 siblings, 0 replies; 47+ messages in thread From: John W. Linville @ 2005-07-11 17:38 UTC (permalink / raw) To: Greg KH Cc: Lennert Buytenhek, Ivan Kokshaysky, David S. Miller, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, ambx1, byjac, herbertb 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 2005-07-08 18:34 ` [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 John W. Linville ` (2 preceding siblings ...) 2005-07-11 12:48 ` Lennert Buytenhek @ 2005-07-12 2:28 ` Adam Belay 2005-07-13 17:34 ` John W. Linville 2005-07-26 23:49 ` Greg KH 4 siblings, 1 reply; 47+ messages in thread From: Adam Belay @ 2005-07-12 2:28 UTC (permalink / raw) To: John W. Linville Cc: Ivan Kokshaysky, linux-pm, linux-kernel, greg, grundler, linux-pci, rmk+lkml, matthew, David S. Miller 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 2005-07-12 2:28 ` [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 Adam Belay @ 2005-07-13 17:34 ` John W. Linville 0 siblings, 0 replies; 47+ messages in thread From: John W. Linville @ 2005-07-13 17:34 UTC (permalink / raw) To: Adam Belay, Ivan Kokshaysky, linux-pm, linux-kernel, greg, grundler, linux-pci, rmk+lkml, matthew, David S. Miller 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 2005-07-08 18:34 ` [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 John W. Linville ` (3 preceding siblings ...) 2005-07-12 2:28 ` [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 Adam Belay @ 2005-07-26 23:49 ` Greg KH 2005-07-27 1:36 ` John W. Linville 4 siblings, 1 reply; 47+ messages in thread From: Greg KH @ 2005-07-26 23:49 UTC (permalink / raw) To: Ivan Kokshaysky, David S. Miller, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, ambx1 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 2005-07-26 23:49 ` Greg KH @ 2005-07-27 1:36 ` John W. Linville 2005-07-27 14:12 ` John W. Linville 0 siblings, 1 reply; 47+ messages in thread From: John W. Linville @ 2005-07-27 1:36 UTC (permalink / raw) To: Greg KH Cc: Ivan Kokshaysky, David S. Miller, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, ambx1 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 2005-07-27 1:36 ` John W. Linville @ 2005-07-27 14:12 ` John W. Linville 2005-07-27 14:19 ` [patch 2.6.13-rc3] pci: restore BAR values after D3hot->D0 for devices that need it John W. Linville 0 siblings, 1 reply; 47+ messages in thread From: John W. Linville @ 2005-07-27 14:12 UTC (permalink / raw) To: Greg KH Cc: Ivan Kokshaysky, David S. Miller, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, ambx1 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [patch 2.6.13-rc3] pci: restore BAR values after D3hot->D0 for devices that need it 2005-07-27 14:12 ` John W. Linville @ 2005-07-27 14:19 ` John W. Linville 2005-07-31 19:36 ` Ralf Baechle ` (2 more replies) 0 siblings, 3 replies; 47+ messages in thread From: John W. Linville @ 2005-07-27 14:19 UTC (permalink / raw) To: Greg KH Cc: Ivan Kokshaysky, David S. Miller, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, ambx1 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.13-rc3] pci: restore BAR values after D3hot->D0 for devices that need it 2005-07-27 14:19 ` [patch 2.6.13-rc3] pci: restore BAR values after D3hot->D0 for devices that need it John W. Linville @ 2005-07-31 19:36 ` Ralf Baechle 2005-08-02 17:31 ` Greg KH 2005-08-02 16:41 ` Jesse Brandeburg 2005-09-14 13:52 ` [patch 2.6.14-rc1] pci: only call pci_restore_bars at boot John W. Linville 2 siblings, 1 reply; 47+ messages in thread From: Ralf Baechle @ 2005-07-31 19:36 UTC (permalink / raw) To: Greg KH, Ivan Kokshaysky, David S. Miller, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, ambx1 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.13-rc3] pci: restore BAR values after D3hot->D0 for devices that need it 2005-07-31 19:36 ` Ralf Baechle @ 2005-08-02 17:31 ` Greg KH 0 siblings, 0 replies; 47+ messages in thread From: Greg KH @ 2005-08-02 17:31 UTC (permalink / raw) To: Ralf Baechle Cc: Ivan Kokshaysky, David S. Miller, rmk+lkml, matthew, grundler, linux-pci, linux-pm, linux-kernel, ambx1 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.13-rc3] pci: restore BAR values after D3hot->D0 for devices that need it 2005-07-27 14:19 ` [patch 2.6.13-rc3] pci: restore BAR values after D3hot->D0 for devices that need it John W. Linville 2005-07-31 19:36 ` Ralf Baechle @ 2005-08-02 16:41 ` Jesse Brandeburg 2005-09-14 13:52 ` [patch 2.6.14-rc1] pci: only call pci_restore_bars at boot John W. Linville 2 siblings, 0 replies; 47+ messages in thread From: Jesse Brandeburg @ 2005-08-02 16:41 UTC (permalink / raw) To: linux-pci, linux-pm, linux-kernel 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [patch 2.6.14-rc1] pci: only call pci_restore_bars at boot 2005-07-27 14:19 ` [patch 2.6.13-rc3] pci: restore BAR values after D3hot->D0 for devices that need it John W. Linville 2005-07-31 19:36 ` Ralf Baechle 2005-08-02 16:41 ` Jesse Brandeburg @ 2005-09-14 13:52 ` John W. Linville 2005-09-14 15:08 ` Jeff Garzik 2 siblings, 1 reply; 47+ messages in thread From: John W. Linville @ 2005-09-14 13:52 UTC (permalink / raw) To: linux-kernel, linux-pci, linux-pm Cc: torvalds, akpm, ink, kaos, greg, davem, rmk+lkml, matthew, grundler, ambx1 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 */ ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.14-rc1] pci: only call pci_restore_bars at boot 2005-09-14 13:52 ` [patch 2.6.14-rc1] pci: only call pci_restore_bars at boot John W. Linville @ 2005-09-14 15:08 ` Jeff Garzik 2005-09-14 16:26 ` David S. Miller 0 siblings, 1 reply; 47+ messages in thread From: Jeff Garzik @ 2005-09-14 15:08 UTC (permalink / raw) To: John W. Linville Cc: linux-kernel, linux-pci, linux-pm, torvalds, akpm, ink, kaos, greg, davem, rmk+lkml, matthew, grundler, ambx1 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.14-rc1] pci: only call pci_restore_bars at boot 2005-09-14 15:08 ` Jeff Garzik @ 2005-09-14 16:26 ` David S. Miller 2005-09-14 16:47 ` John W. Linville 2005-09-14 18:22 ` Ivan Kokshaysky 0 siblings, 2 replies; 47+ messages in thread From: David S. Miller @ 2005-09-14 16:26 UTC (permalink / raw) To: jgarzik Cc: linville, linux-kernel, linux-pci, linux-pm, torvalds, akpm, ink, kaos, greg, rmk+lkml, matthew, grundler, ambx1 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.14-rc1] pci: only call pci_restore_bars at boot 2005-09-14 16:26 ` David S. Miller @ 2005-09-14 16:47 ` John W. Linville 2005-09-14 18:22 ` Ivan Kokshaysky 1 sibling, 0 replies; 47+ messages in thread From: John W. Linville @ 2005-09-14 16:47 UTC (permalink / raw) To: David S. Miller Cc: jgarzik, linux-kernel, linux-pci, linux-pm, torvalds, akpm, ink, kaos, greg, rmk+lkml, matthew, grundler, ambx1 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.14-rc1] pci: only call pci_restore_bars at boot 2005-09-14 16:26 ` David S. Miller 2005-09-14 16:47 ` John W. Linville @ 2005-09-14 18:22 ` Ivan Kokshaysky 1 sibling, 0 replies; 47+ messages in thread From: Ivan Kokshaysky @ 2005-09-14 18:22 UTC (permalink / raw) To: David S. Miller Cc: jgarzik, linville, linux-kernel, linux-pci, linux-pm, torvalds, akpm, kaos, greg, rmk+lkml, matthew, grundler, ambx1 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. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars 2005-07-02 7:29 ` Grant Grundler 2005-07-02 8:09 ` Russell King @ 2005-07-05 17:46 ` John W. Linville 2005-07-18 12:17 ` Grant Grundler 1 sibling, 1 reply; 47+ messages in thread From: John W. Linville @ 2005-07-05 17:46 UTC (permalink / raw) To: Grant Grundler Cc: linux-pci, linux-pm, linux-kernel, Greg KH, Adam Belay, 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: > > + /* 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars 2005-07-05 17:46 ` [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars John W. Linville @ 2005-07-18 12:17 ` Grant Grundler 0 siblings, 0 replies; 47+ messages in thread From: Grant Grundler @ 2005-07-18 12:17 UTC (permalink / raw) To: John W. Linville Cc: Grant Grundler, linux-pci, linux-pm, linux-kernel, Greg KH, Adam Belay, Russell King 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
end of thread, other threads:[~2005-09-14 18:22 UTC | newest] Thread overview: 47+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2005-06-23 19:14 [RFC] firmware leaves device in D3hot at boot John W. Linville 2005-06-24 2:28 ` John W. Linville 2005-06-30 17:10 ` Greg KH 2005-07-01 1:41 ` John W. Linville 2005-07-01 2:26 ` [patch 2.6.12] pci: restore BAR values in pci_enable_device John W. Linville 2005-07-01 2:26 ` [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars John W. Linville 2005-07-02 7:29 ` Grant Grundler 2005-07-02 8:09 ` Russell King 2005-07-05 20:05 ` Matthew Wilcox 2005-07-05 21:46 ` Russell King 2005-07-05 23:34 ` Ivan Kokshaysky 2005-07-06 7:46 ` Russell King 2005-07-08 0:57 ` John W. Linville 2005-07-08 0:59 ` [patch 2.6.13-rc2] pci: restore BAR values in pci_set_power_state for D3hot->D0 John W. Linville 2005-07-08 3:43 ` [linux-pm] " david-b 2005-07-08 12:37 ` John W. Linville 2005-07-08 3:11 ` [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars David S. Miller 2005-07-08 5:51 ` Ivan Kokshaysky 2005-07-08 6:35 ` David S. Miller 2005-07-08 7:03 ` Ivan Kokshaysky 2005-07-08 7:33 ` David S. Miller 2005-07-08 8:20 ` Ivan Kokshaysky 2005-07-08 18:34 ` [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 John W. Linville 2005-07-08 19:08 ` David S. Miller 2005-07-10 17:53 ` Ivan Kokshaysky 2005-07-11 12:48 ` Lennert Buytenhek 2005-07-11 13:15 ` John W. Linville 2005-07-11 13:19 ` [patch 2.6.13-rc2] PCI: Add symbol exports for pci_restore_bars John W. Linville 2005-07-11 17:18 ` Greg KH 2005-07-11 17:36 ` John W. Linville 2005-07-11 17:38 ` [patch 2.6.13-rc2] PCI: Add GPL symbol export " John W. Linville 2005-07-12 2:28 ` [patch 2.6.13-rc2] pci: restore BAR values from pci_set_power_state for D3hot->D0 Adam Belay 2005-07-13 17:34 ` John W. Linville 2005-07-26 23:49 ` Greg KH 2005-07-27 1:36 ` John W. Linville 2005-07-27 14:12 ` John W. Linville 2005-07-27 14:19 ` [patch 2.6.13-rc3] pci: restore BAR values after D3hot->D0 for devices that need it John W. Linville 2005-07-31 19:36 ` Ralf Baechle 2005-08-02 17:31 ` Greg KH 2005-08-02 16:41 ` Jesse Brandeburg 2005-09-14 13:52 ` [patch 2.6.14-rc1] pci: only call pci_restore_bars at boot John W. Linville 2005-09-14 15:08 ` Jeff Garzik 2005-09-14 16:26 ` David S. Miller 2005-09-14 16:47 ` John W. Linville 2005-09-14 18:22 ` Ivan Kokshaysky 2005-07-05 17:46 ` [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars John W. Linville 2005-07-18 12:17 ` Grant Grundler
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).