Re: Suspend problems in 2.6.31-rc6

Rafael J. Wysocki
2009-08-24T18:55:49Z

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