From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH 2/4] drm/i915: Fix system resume if PCI device remained enabled Date: Mon, 18 Apr 2016 11:16:34 +0300 Message-ID: <1460967394.3172.11.camel@intel.com> References: <1460963062-13211-1-git-send-email-imre.deak@intel.com> <1460963062-13211-3-git-send-email-imre.deak@intel.com> <20160418080646.GD10708@nuc-i3427.alporthouse.com> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 7B2A36E36C for ; Mon, 18 Apr 2016 08:16:38 +0000 (UTC) In-Reply-To: <20160418080646.GD10708@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org T24gbWEsIDIwMTYtMDQtMTggYXQgMDk6MDYgKzAxMDAsIENocmlzIFdpbHNvbiB3cm90ZToKPiBP biBNb24sIEFwciAxOCwgMjAxNiBhdCAxMDowNDoyMEFNICswMzAwLCBJbXJlIERlYWsgd3JvdGU6 Cj4gPiBEdXJpbmcgc3lzdGVtIHJlc3VtZSB3ZSBkZXBlbmRlZCBvbiBwY2lfZW5hYmxlX2Rldmlj ZSgpIGFsc28KPiA+IHB1dHRpbmcgdGhlCj4gPiBkZXZpY2UgaW50byBQQ0kgRDAgc3RhdGUuIFRo aXMgd29uJ3Qgd29yayBpZiB0aGUgUENJIGRldmljZSB3YXMKPiA+IGFscmVhZHkKPiA+IGVuYWJs ZWQgYnV0IHN0aWxsIGluIEQzIHN0YXRlLiBUaGlzIGlzIGJlY2F1c2UgcGNpX2VuYWJsZV9kZXZp Y2UoKQo+ID4gaXMKPiA+IHJlZmNvdW50ZWQgYW5kIHdpbGwgbm90IGNoYW5nZSB0aGUgSFcgc3Rh dGUgaWYgY2FsbGVkIHdpdGggYSBub24tCj4gPiB6ZXJvCj4gPiByZWZjb3VudC4gTGVhdmluZyB0 aGUgZGV2aWNlIGluIEQzIHdpbGwgbWFrZSBhbGwgc3Vic2VxdWVudCBkZXZpY2UKPiA+IGFjY2Vz c2VzIGZhaWwuCj4gPiAKPiA+IFRoaXMgZGlkbid0IGNhdXNlIGEgcHJvYmxlbSBtb3N0IG9mIHRo ZSB0aW1lLCBzaW5jZSB3ZSByZXN1bWVkIHdpdGgKPiA+IGFuCj4gPiBlbmFibGUgcmVmY291bnQg b2YgMC4gQnV0IGl0IGZhaWxzIGF0IGxlYXN0IGFmdGVyIG1vZHVsZSByZWxvYWQKPiA+IGJlY2F1 c2UKPiA+IGFmdGVyIHRoYXQgd2UgYWxzbyBoYXBwZW4gdG8gbGVhayBhIFBDSSBkZXZpY2UgZW5h YmxlIHJlZmVyZW5jZToKPiA+IER1cmluZwo+ID4gcHJvYmluZyB3ZSBjYWxsIGRybV9nZXRfcGNp X2RldigpIHdoaWNoIHdpbGwgZW5hYmxlIHRoZSBQQ0kgZGV2aWNlLAo+ID4gYnV0Cj4gPiBkdXJp bmcgZGV2aWNlIHJlbW92YWwgZHJtX3B1dF9kZXYoKSB3b24ndCBkaXNhYmxlIGl0LiBUaGlzIGlz IGEgYnVnCj4gPiBvZgo+ID4gaXRzIG93biBpbiBEUk0gY29yZSwgYnV0IHdpdGhvdXQgbXVjaCBo YXJtIGFzIGl0IG9ubHkgbGVhdmVzIHRoZQo+ID4gUENJCj4gPiBkZXZpY2UgZW5hYmxlZC4gRml4 aW5nIGl0IGlzIGFsc28gYSBiaXQgbW9yZSBpbnZvbHZlZCwgZHVlIHRvIERSTQo+ID4gbWlkLWxh eWVyaW5nIGFuZCBiZWNhdXNlIGl0IGFmZmVjdHMgbm9uLWk5MTUgZHJpdmVycyB0b28uIFRoZSBm aXgKPiA+IGluCj4gPiB0aGlzIHBhdGNoIGlzIHZhbGlkIHJlZ2FyZGxlc3Mgb2YgdGhlIHByb2Js ZW0gaW4gRFJNIGNvcmUuCj4gPiAKPiA+IENDOiBWaWxsZSBTeXJqw6Rsw6QgPHZpbGxlLnN5cmph bGFAbGludXguaW50ZWwuY29tPgo+ID4gQ0M6IHN0YWJsZUB2Z2VyLmtlcm5lbC5vcmcKPiA+IFNp Z25lZC1vZmYtYnk6IEltcmUgRGVhayA8aW1yZS5kZWFrQGludGVsLmNvbT4KPiA+IC0tLQo+ID4g wqBkcml2ZXJzL2dwdS9kcm0vaTkxNS9pOTE1X2Rydi5jIHwgOSArKysrKysrKy0KPiA+IMKgMSBm aWxlIGNoYW5nZWQsIDggaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbigtKQo+ID4gCj4gPiBkaWZm IC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9kcnYuYwo+ID4gYi9kcml2ZXJzL2dw dS9kcm0vaTkxNS9pOTE1X2Rydi5jCj4gPiBpbmRleCBkNTUwYWUyLi43ZWFhOTNlIDEwMDY0NAo+ ID4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9kcnYuYwo+ID4gKysrIGIvZHJpdmVy cy9ncHUvZHJtL2k5MTUvaTkxNV9kcnYuYwo+ID4gQEAgLTgwMyw3ICs4MDMsNyBAQCBzdGF0aWMg aW50IGk5MTVfZHJtX3Jlc3VtZShzdHJ1Y3QgZHJtX2RldmljZQo+ID4gKmRldikKPiA+IMKgc3Rh dGljIGludCBpOTE1X2RybV9yZXN1bWVfZWFybHkoc3RydWN0IGRybV9kZXZpY2UgKmRldikKPiA+ IMKgewo+ID4gwqAJc3RydWN0IGRybV9pOTE1X3ByaXZhdGUgKmRldl9wcml2ID0gZGV2LT5kZXZf cHJpdmF0ZTsKPiA+IC0JaW50IHJldCA9IDA7Cj4gPiArCWludCByZXQ7Cj4gPiDCoAo+ID4gwqAJ LyoKPiA+IMKgCcKgKiBXZSBoYXZlIGEgcmVzdW1lIG9yZGVyaW5nIGlzc3VlIHdpdGggdGhlIHNu ZC1oZGEgZHJpdmVyCj4gPiBhbHNvCj4gPiBAQCAtODE0LDYgKzgxNCwxMyBAQCBzdGF0aWMgaW50 IGk5MTVfZHJtX3Jlc3VtZV9lYXJseShzdHJ1Y3QKPiA+IGRybV9kZXZpY2UgKmRldikKPiA+IMKg CcKgKiBGSVhNRTogVGhpcyBzaG91bGQgYmUgc29sdmVkIHdpdGggYSBzcGVjaWFsIGhkbWkgc2lu awo+ID4gZGV2aWNlIG9yCj4gPiDCoAnCoCogc2ltaWxhciBzbyB0aGF0IHBvd2VyIGRvbWFpbnMg Y2FuIGJlIGVtcGxveWVkLgo+ID4gwqAJwqAqLwo+ID4gKwo+ID4gKwlyZXQgPSBwY2lfc2V0X3Bv d2VyX3N0YXRlKGRldi0+cGRldiwgUENJX0QwKTsKPiA+ICsJaWYgKHJldCkgewo+ID4gKwkJRFJN X0VSUk9SKCJmYWlsZWQgdG8gc2V0IFBDSSBEMCBwb3dlciBzdGF0ZQo+ID4gKCVkKVxuIiwgcmV0 KTsKPiA+ICsJCWdvdG8gb3V0Owo+ID4gKwl9Cj4gCj4gVGhlIGRldmljZSBzaG91bGQgYmUgZW5h YmxlZCBmaXJzdCwgb3RoZXJ3aXNlIHdlIGFyZSBub3QgbWVhbnQgdG8gYmUKPiB0b3VjaGluZyBp dHMgSU8gc3BhY2UgYXQgYWxsIChzdWNoIGFzIHR3aWRkbGluZyBwb3dlciBzdGF0ZSkuIEF0Cj4g bGVhc3QKPiB0aGF0IGlzIHRoZSBvcmRlciBwY2lfZW5hYmxlX2RldmljZSgpIHVzZXMuCgpJdCdz IG5vdCBNTUlPIG9yIChwb3J0KSBJTyBidXQgb25seSBhIFBDSSBjb25maWcgc3BhY2UgYWNjZXNz CnRoYXTCoHBjaV9zZXRfcG93ZXJfc3RhdGUoKSByZXF1aXJlcywgc28gZG9lc24ndCBuZWVkIHRo ZSBlbmFibGluZyAKb2YgUENJIHJlc291cmNlcy4gQUZBSUNTwqBwY2lfZW5hYmxlX2RldmljZSgp IGVuYWJsZXMgcG93ZXIgYXMgdGhlIGZpcnN0CnRoaW5nLgoKPiBFaXRoZXIgd2F5LCB1cG9uIGZh aWx1cmUgd2Ugc2hvdWxkIGJlIHVud2luZGluZy4KCkknZCByYXRoZXIgd291bGRuJ3QgcHV0IGJh Y2sgdGhlIGRldmljZSB0byBEMyBzdGF0ZSwgYXMgZnVydGhlciBkZXZpY2UKYWNjZXNzIG1heSBz dGlsbCBwb3NzaWJsZSBldmVuIHRob3VnaCByZXN1bWUgZmFpbGVkLgoKLS1JbXJlCl9fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCkludGVsLWdmeCBtYWlsaW5n IGxpc3QKSW50ZWwtZ2Z4QGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVk ZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2ludGVsLWdmeAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com ([192.55.52.93]:9109 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750915AbcDRIQj (ORCPT ); Mon, 18 Apr 2016 04:16:39 -0400 Message-ID: <1460967394.3172.11.camel@intel.com> Subject: Re: [Intel-gfx] [PATCH 2/4] drm/i915: Fix system resume if PCI device remained enabled From: Imre Deak Reply-To: imre.deak@intel.com To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org Date: Mon, 18 Apr 2016 11:16:34 +0300 In-Reply-To: <20160418080646.GD10708@nuc-i3427.alporthouse.com> References: <1460963062-13211-1-git-send-email-imre.deak@intel.com> <1460963062-13211-3-git-send-email-imre.deak@intel.com> <20160418080646.GD10708@nuc-i3427.alporthouse.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: stable-owner@vger.kernel.org List-ID: On ma, 2016-04-18 at 09:06 +0100, Chris Wilson wrote: > On Mon, Apr 18, 2016 at 10:04:20AM +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 > > already > > 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. > > > > This didn't cause a problem most of the time, since we resumed with > > an > > enable refcount of 0. But it fails at least after module reload > > because > > after that we also happen to leak a PCI device enable reference: > > During > > probing we call drm_get_pci_dev() which will enable the PCI device, > > but > > during device removal drm_put_dev() won't disable it. This is a bug > > of > > 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. > > > > CC: Ville Syrjälä > > CC: stable@vger.kernel.org > > Signed-off-by: Imre Deak > > --- > >  drivers/gpu/drm/i915/i915_drv.c | 9 ++++++++- > >  1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > b/drivers/gpu/drm/i915/i915_drv.c > > index d550ae2..7eaa93e 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 = dev->dev_private; > > - int ret = 0; > > + int ret; > >   > >   /* > >    * We have a resume ordering issue with the snd-hda driver > > also > > @@ -814,6 +814,13 @@ static int i915_drm_resume_early(struct > > drm_device *dev) > >    * FIXME: This should be solved with a special hdmi sink > > device or > >    * similar so that power domains can be employed. > >    */ > > + > > + ret = pci_set_power_state(dev->pdev, PCI_D0); > > + if (ret) { > > + DRM_ERROR("failed to set PCI D0 power state > > (%d)\n", ret); > > + goto out; > > + } > > The device should be enabled first, otherwise we are not meant to be > touching its IO space at all (such as twiddling power state). At > least > that is the order pci_enable_device() uses. It's not MMIO or (port) IO but only a PCI config space access that pci_set_power_state() requires, so doesn't need the enabling of PCI resources. AFAICS pci_enable_device() enables power as the first thing. > Either way, upon failure we should be unwinding. I'd rather wouldn't put back the device to D3 state, as further device access may still possible even though resume failed. --Imre