[PATCH] [PCI] Prevent user config space access during power state transitions

[PATCH] [PCI] Prevent user config space access during power state transitions

[Matthew Wilcox]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]

Section 5.3 of PCI Bus Power Management 1.2 states:

  There is a minimum recovery time requirement of 200 µs between when
  a function is programmed from D2 to D0 and when the function can be
  next accessed as a target (including PCI configuration accesses). If
  an access is attempted in violation of the specified minimum recovery
  time, undefined system behavior may result.

We have to prevent the user running lspci during this time, and
fortunately we already have the pci_block_user_cfg_access() API to
do this.

Signed-off-by: Matthew Wilcox <matthew@wil.cx>
---
 drivers/pci/pci.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a544997..1bb059a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -366,6 +366,11 @@ pci_set_power_state(struct pci_dev *dev,
 		break;
 	}
 
+	/* We have to prevent accesses to config space while transitioning
+	 * between power states
+	 */
+	pci_block_user_cfg_access(dev);
+
 	/* enter specified state */
 	pci_write_config_word(dev, pm + PCI_PM_CTRL, pmcsr);
 
@@ -383,6 +388,9 @@ pci_set_power_state(struct pci_dev *dev,
 	if (platform_pci_set_power_state)
 		platform_pci_set_power_state(dev, state);
 
+	/* Should be safe to allow userspace access to the device again now */
+	pci_unblock_user_cfg_access(dev);
+
 	dev->current_state = state;
 
 	/* According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT
-- 
1.4.1.1

Re: [PATCH] [PCI] Prevent user config space access during power state transitions

[Andrew Morton]

On Tue, 10 Oct 2006 07:38:17 -0600
Matthew Wilcox <matthew@wil.cx> wrote:

> Section 5.3 of PCI Bus Power Management 1.2 states:
> 
>   There is a minimum recovery time requirement of 200 µs between when
>   a function is programmed from D2 to D0 and when the function can be
>   next accessed as a target (including PCI configuration accesses). If
>   an access is attempted in violation of the specified minimum recovery
>   time, undefined system behavior may result.
> 
> We have to prevent the user running lspci during this time, and
> fortunately we already have the pci_block_user_cfg_access() API to
> do this.
> 
> Signed-off-by: Matthew Wilcox <matthew@wil.cx>
> ---
>  drivers/pci/pci.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a544997..1bb059a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -366,6 +366,11 @@ pci_set_power_state(struct pci_dev *dev,
>  		break;
>  	}
>  
> +	/* We have to prevent accesses to config space while transitioning
> +	 * between power states
> +	 */
> +	pci_block_user_cfg_access(dev);
> +
>  	/* enter specified state */
>  	pci_write_config_word(dev, pm + PCI_PM_CTRL, pmcsr);
>  
> @@ -383,6 +388,9 @@ pci_set_power_state(struct pci_dev *dev,
>  	if (platform_pci_set_power_state)
>  		platform_pci_set_power_state(dev, state);
>  
> +	/* Should be safe to allow userspace access to the device again now */
> +	pci_unblock_user_cfg_access(dev);
> +
>  	dev->current_state = state;
>  
>  	/* According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT

This patch independently causes the same failure: the Vaio doesn't power
off after suspend-to-disk and after a manual power cycle and resume,
networking is dead.

The message `acpi_power_off called' never comes out, so something probably
got stuck.

Or maybe something failed somewhere and the error code which would have
helped us solve this bug was simply ignored.



Note that pci_block_user_cfg_access() calls pci_save_state(), which can
fail.  But pci_block_user_cfg_access() discards that information and
returns void.  If this happens, userspace config space reads will return...
what?  

Re: [PATCH] [PCI] Prevent user config space access during power state transitions

[Greg KH]

On Fri, Oct 13, 2006 at 10:26:08PM -0700, Andrew Morton wrote:
> On Tue, 10 Oct 2006 07:38:17 -0600
> Matthew Wilcox <matthew@wil.cx> wrote:
> 
> > Section 5.3 of PCI Bus Power Management 1.2 states:
> > 
> >   There is a minimum recovery time requirement of 200 ?s between when
> >   a function is programmed from D2 to D0 and when the function can be
> >   next accessed as a target (including PCI configuration accesses). If
> >   an access is attempted in violation of the specified minimum recovery
> >   time, undefined system behavior may result.
> > 
> > We have to prevent the user running lspci during this time, and
> > fortunately we already have the pci_block_user_cfg_access() API to
> > do this.
> > 
> > Signed-off-by: Matthew Wilcox <matthew@wil.cx>
> > ---
> >  drivers/pci/pci.c |    8 ++++++++
> >  1 files changed, 8 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index a544997..1bb059a 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -366,6 +366,11 @@ pci_set_power_state(struct pci_dev *dev,
> >  		break;
> >  	}
> >  
> > +	/* We have to prevent accesses to config space while transitioning
> > +	 * between power states
> > +	 */
> > +	pci_block_user_cfg_access(dev);
> > +
> >  	/* enter specified state */
> >  	pci_write_config_word(dev, pm + PCI_PM_CTRL, pmcsr);
> >  
> > @@ -383,6 +388,9 @@ pci_set_power_state(struct pci_dev *dev,
> >  	if (platform_pci_set_power_state)
> >  		platform_pci_set_power_state(dev, state);
> >  
> > +	/* Should be safe to allow userspace access to the device again now */
> > +	pci_unblock_user_cfg_access(dev);
> > +
> >  	dev->current_state = state;
> >  
> >  	/* According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT
> 
> This patch independently causes the same failure: the Vaio doesn't power
> off after suspend-to-disk and after a manual power cycle and resume,
> networking is dead.
> 
> The message `acpi_power_off called' never comes out, so something probably
> got stuck.
> 
> Or maybe something failed somewhere and the error code which would have
> helped us solve this bug was simply ignored.
> 
> 
> 
> Note that pci_block_user_cfg_access() calls pci_save_state(), which can
> fail.  But pci_block_user_cfg_access() discards that information and
> returns void.  If this happens, userspace config space reads will return...
> what?  

There is a thread right now on the linux-pm mailing list about how this
patch is wrong.  I'm going to drop it right now...

thanks,

greg k-h