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 <[email protected]>
---
Many thanks to Keith Owens <[email protected]> for a) narrowing-down the
problem; and, b) quickly testing the fix and reporting the results.
drivers/pci/pci.c | 16 ++++++++++++----
1 files changed, 12 insertions(+), 4 deletions(-)
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 */
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 <[email protected]>
> ---
> Many thanks to Keith Owens <[email protected]> for a) narrowing-down the
> problem; and, b) quickly testing the fix and reporting the results.
>
> drivers/pci/pci.c | 16 ++++++++++++----
> 1 files changed, 12 insertions(+), 4 deletions(-)
>
> 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
From: Jeff Garzik <[email protected]>
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.
On Wed, Sep 14, 2005 at 09:26:50AM -0700, David S. Miller wrote:
> From: Jeff Garzik <[email protected]>
> 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
[email protected]
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.