Re: Suspend problems in 2.6.31-rc6

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Monday 24 August 2009, Zhenyu Wang wrote:
> On 2009.08.21 19:01:06 +0200, Rafael J. Wysocki wrote:
> > Yes.  Moreover, it doesn't need to be restored at that point, because it has
> > already been restored earlier by the PCI bus type code.
> 
> true.
> 
> > 
> > So, Zhenyu, it is completely unnecessary to call pci_restore_state() from
> > a driver's ->resume() routine, because the PCI bus type driver has
> > already called it in its ->resume_noirq().  And please note that
> > pci_pm_default_resume_noirq() clears state_saved, which is why the Alek's
> > patch helps.
> > 
> > Without the Alek's patch on the Alan's system the config registers of both
> > PCI devices are restored twice, first by the PCI bus type at the "noirq" stage
> > and next by the driver.  The Alek's patch turns the second restorations into
> > NOPs, which is fine, because they are unnecessary in general and they break
> > things on the Alan's system.
> > 
> 
> yeah, Alex's patch is ok in general, and Alan's problem seems specific on 845G.
> So the origin restore order issue for us seems gone with pci layer early resume,
> we could remove the restore in intel_agp and drm/i915, although Alex's patch makes
> them noop.
> 
> How about this one? Alan, could you help to test this with drm/i915 module loaded?
> 
> thanks.
> 
> From 7ab123a67e3ab3f31051d95e74362ae711e0657e Mon Sep 17 00:00:00 2001
> From: Zhenyu Wang <zhenyuw@linux.intel.com>
> Date: Mon, 24 Aug 2009 10:29:07 +0800
> Subject: [PATCH] agp/intel: remove restore in resume
> 
> As earlier pci driver resume has already restored space for
> host bridge and graphics device, don't need to restore it
> again which might cause problem on some chips, e.g 845G tested
> by Alan.
> 
> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> ---
>  drivers/char/agp/intel-agp.c    |    9 ---------
>  drivers/gpu/drm/i915/i915_drv.c |    1 -
>  2 files changed, 0 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c
> index 8c9d50d..ed43ab2 100644
> --- a/drivers/char/agp/intel-agp.c
> +++ b/drivers/char/agp/intel-agp.c
> @@ -2308,15 +2308,6 @@ static int agp_intel_resume(struct pci_dev *pdev)
>  	struct agp_bridge_data *bridge = pci_get_drvdata(pdev);
>  	int ret_val;
>  
> -	pci_restore_state(pdev);
> -
> -	/* We should restore our graphics device's config space,
> -	 * as host bridge (00:00) resumes before graphics device (02:00),
> -	 * then our access to its pci space can work right.
> -	 */
> -	if (intel_private.pcidev)
> -		pci_restore_state(intel_private.pcidev);
> -
>  	if (bridge->driver == &intel_generic_driver)
>  		intel_configure();
>  	else if (bridge->driver == &intel_850_driver)
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index fc4b68a..645f298 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -95,7 +95,6 @@ static int i915_resume(struct drm_device *dev)
>  	int ret = 0;
>  
>  	pci_set_power_state(dev->pdev, PCI_D0);
> -	pci_restore_state(dev->pdev);

You can also drop the pci_set_power_state() above, the device is already in D0
at this point.

>  	if (pci_enable_device(dev->pdev))
>  		return -1;
>  	pci_set_master(dev->pdev);
> 

Thanks,
Rafael