From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v2 2/4] drm/i915: Fix system resume if PCI device remained enabled Date: Mon, 18 Apr 2016 17:59:55 +0300 Message-ID: <20160418145955.GF4329@intel.com> References: <1460963062-13211-3-git-send-email-imre.deak@intel.com> <1460979954-14503-1-git-send-email-imre.deak@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: <1460979954-14503-1-git-send-email-imre.deak@intel.com> Sender: stable-owner@vger.kernel.org To: Imre Deak Cc: intel-gfx@lists.freedesktop.org, Chris Wilson , stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org On Mon, Apr 18, 2016 at 02:45:54PM +0300, Imre Deak wrote: > During system resume we depended on pci_enable_device() also putting = the > device into PCI D0 state. This won't work if the PCI device was alrea= dy > enabled but still in D3 state. This is because pci_enable_device() is > refcounted and will not change the HW state if called with a non-zero > refcount. Leaving the device in D3 will make all subsequent device > accesses fail. >=20 > This didn't cause a problem most of the time, since we resumed with a= n > enable refcount of 0. But it fails at least after module reload becau= se > after that we also happen to leak a PCI device enable reference: Duri= ng > probing we call drm_get_pci_dev() which will enable the PCI device, b= ut > during device removal drm_put_dev() won't disable it. This is a bug o= f > its own in DRM core, but without much harm as it only leaves the PCI > device enabled. Fixing it is also a bit more involved, due to DRM > mid-layering and because it affects non-i915 drivers too. The fix in > this patch is valid regardless of the problem in DRM core. >=20 > v2: > - Add a code comment about the relation of this fix to the freeze/tha= w > vs. the suspend/resume phases. (Ville) > - Add a code comment about the inconsistent ordering of set power sta= te > and device enable calls. (Chris) >=20 > CC: Ville Syrj=E4l=E4 > CC: Chris Wilson > CC: stable@vger.kernel.org > Signed-off-by: Imre Deak The PCI PM code is a bit of a mess, but it would be appear this should do what we need. Reviewed-by: Ville Syrj=E4l=E4 > --- > drivers/gpu/drm/i915/i915_drv.c | 32 +++++++++++++++++++++++++++++++= - > 1 file changed, 31 insertions(+), 1 deletion(-) >=20 > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i= 915_drv.c > index d550ae2..3b79e97 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -803,7 +803,7 @@ static int i915_drm_resume(struct drm_device *dev= ) > static int i915_drm_resume_early(struct drm_device *dev) > { > struct drm_i915_private *dev_priv =3D dev->dev_private; > - int ret =3D 0; > + int ret; > =20 > /* > * We have a resume ordering issue with the snd-hda driver also > @@ -814,6 +814,36 @@ static int i915_drm_resume_early(struct drm_devi= ce *dev) > * FIXME: This should be solved with a special hdmi sink device or > * similar so that power domains can be employed. > */ > + > + /* > + * Note that we need to set the power state explicitly, since we > + * powered off the device during freeze and the PCI core won't powe= r > + * it back up for us during thaw. Powering off the device during > + * freeze is not a hard requirement though, and during the > + * suspend/resume phases the PCI core makes sure we get here with t= he > + * device powered on. So in case we change our freeze logic and kee= p > + * the device powered we can also remove the following set power st= ate > + * call. > + */ > + ret =3D pci_set_power_state(dev->pdev, PCI_D0); > + if (ret) { > + DRM_ERROR("failed to set PCI D0 power state (%d)\n", ret); > + goto out; > + } > + > + /* > + * Note that pci_enable_device() first enables any parent bridge > + * device and only then sets the power state for this device. The > + * bridge enabling is a nop though, since bridge devices are resume= d > + * first. The order of enabling power and enabling the device is > + * imposed by the PCI core as described above, so here we preserve = the > + * same order for the freeze/thaw phases. > + * > + * TODO: eventually we should remove pci_disable_device() / > + * pci_enable_enable_device() from suspend/resume. Due to how they > + * depend on the device enable refcount we can't anyway depend on t= hem > + * disabling/enabling the device. > + */ > if (pci_enable_device(dev->pdev)) { > ret =3D -EIO; > goto out; > --=20 > 2.5.0 --=20 Ville Syrj=E4l=E4 Intel OTC