From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 2/4] drm/i915: Fix system resume if PCI device remained enabled Date: Mon, 18 Apr 2016 12:04:03 +0300 Message-ID: <20160418090403.GC4329@intel.com> References: <1460963062-13211-1-git-send-email-imre.deak@intel.com> <1460963062-13211-3-git-send-email-imre.deak@intel.com> <20160418082822.GY4329@intel.com> <1460968358.3172.17.camel@intel.com> <20160418084451.GB4329@intel.com> <1460969671.3172.25.camel@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by gabe.freedesktop.org (Postfix) with ESMTP id 906806E556 for ; Mon, 18 Apr 2016 09:04:08 +0000 (UTC) Content-Disposition: inline In-Reply-To: <1460969671.3172.25.camel@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Imre Deak Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org T24gTW9uLCBBcHIgMTgsIDIwMTYgYXQgMTE6NTQ6MzFBTSArMDMwMCwgSW1yZSBEZWFrIHdyb3Rl Ogo+IE9uIG1hLCAyMDE2LTA0LTE4IGF0IDExOjQ0ICswMzAwLCBWaWxsZSBTeXJqw6Rsw6Qgd3Jv dGU6Cj4gPiBPbiBNb24sIEFwciAxOCwgMjAxNiBhdCAxMTozMjozOEFNICswMzAwLCBJbXJlIERl YWsgd3JvdGU6Cj4gPiA+IE9uIG1hLCAyMDE2LTA0LTE4IGF0IDExOjI4ICswMzAwLCBWaWxsZSBT eXJqw6Rsw6Qgd3JvdGU6Cj4gPiA+ID4gT24gTW9uLCBBcHIgMTgsIDIwMTYgYXQgMTA6MDQ6MjBB TSArMDMwMCwgSW1yZSBEZWFrIHdyb3RlOgo+ID4gPiA+ID4gRHVyaW5nIHN5c3RlbSByZXN1bWUg d2UgZGVwZW5kZWQgb24gcGNpX2VuYWJsZV9kZXZpY2UoKSBhbHNvCj4gPiA+ID4gPiBwdXR0aW5n IHRoZQo+ID4gPiA+ID4gZGV2aWNlIGludG8gUENJIEQwIHN0YXRlLiBUaGlzIHdvbid0IHdvcmsg aWYgdGhlIFBDSSBkZXZpY2Ugd2FzCj4gPiA+ID4gPiBhbHJlYWR5Cj4gPiA+ID4gPiBlbmFibGVk IGJ1dCBzdGlsbCBpbiBEMyBzdGF0ZS4gVGhpcyBpcyBiZWNhdXNlIHBjaV9lbmFibGVfZGV2aWNl KCkKPiA+ID4gPiA+IGlzCj4gPiA+ID4gPiByZWZjb3VudGVkIGFuZCB3aWxsIG5vdCBjaGFuZ2Ug dGhlIEhXIHN0YXRlIGlmIGNhbGxlZCB3aXRoIGEgbm9uLQo+ID4gPiA+ID4gemVybwo+ID4gPiA+ ID4gcmVmY291bnQuIExlYXZpbmcgdGhlIGRldmljZSBpbiBEMyB3aWxsIG1ha2UgYWxsIHN1YnNl cXVlbnQgZGV2aWNlCj4gPiA+ID4gPiBhY2Nlc3NlcyBmYWlsLgo+ID4gPiA+ID4gCj4gPiA+ID4g PiBUaGlzIGRpZG4ndCBjYXVzZSBhIHByb2JsZW0gbW9zdCBvZiB0aGUgdGltZSwgc2luY2Ugd2Ug cmVzdW1lZCB3aXRoCj4gPiA+ID4gPiBhbgo+ID4gPiA+ID4gZW5hYmxlIHJlZmNvdW50IG9mIDAu IEJ1dCBpdCBmYWlscyBhdCBsZWFzdCBhZnRlciBtb2R1bGUgcmVsb2FkCj4gPiA+ID4gPiBiZWNh dXNlCj4gPiA+ID4gPiBhZnRlciB0aGF0IHdlIGFsc28gaGFwcGVuIHRvIGxlYWsgYSBQQ0kgZGV2 aWNlIGVuYWJsZSByZWZlcmVuY2U6Cj4gPiA+ID4gPiBEdXJpbmcKPiA+ID4gPiA+IHByb2Jpbmcg d2UgY2FsbCBkcm1fZ2V0X3BjaV9kZXYoKSB3aGljaCB3aWxsIGVuYWJsZSB0aGUgUENJIGRldmlj ZSwKPiA+ID4gPiA+IGJ1dAo+ID4gPiA+ID4gZHVyaW5nIGRldmljZSByZW1vdmFsIGRybV9wdXRf ZGV2KCkgd29uJ3QgZGlzYWJsZSBpdC4gVGhpcyBpcyBhIGJ1Zwo+ID4gPiA+ID4gb2YKPiA+ID4g PiA+IGl0cyBvd24gaW4gRFJNIGNvcmUsIGJ1dCB3aXRob3V0IG11Y2ggaGFybSBhcyBpdCBvbmx5 IGxlYXZlcyB0aGUKPiA+ID4gPiA+IFBDSQo+ID4gPiA+ID4gZGV2aWNlIGVuYWJsZWQuIEZpeGlu ZyBpdCBpcyBhbHNvIGEgYml0IG1vcmUgaW52b2x2ZWQsIGR1ZSB0byBEUk0KPiA+ID4gPiA+IG1p ZC1sYXllcmluZyBhbmQgYmVjYXVzZSBpdCBhZmZlY3RzIG5vbi1pOTE1IGRyaXZlcnMgdG9vLiBU aGUgZml4Cj4gPiA+ID4gPiBpbgo+ID4gPiA+ID4gdGhpcyBwYXRjaCBpcyB2YWxpZCByZWdhcmRs ZXNzIG9mIHRoZSBwcm9ibGVtIGluIERSTSBjb3JlLgo+ID4gPiA+ID4gCj4gPiA+ID4gPiBDQzog VmlsbGUgU3lyasOkbMOkIDx2aWxsZS5zeXJqYWxhQGxpbnV4LmludGVsLmNvbT4KPiA+ID4gPiA+ IENDOiBzdGFibGVAdmdlci5rZXJuZWwub3JnCj4gPiA+ID4gPiBTaWduZWQtb2ZmLWJ5OiBJbXJl IERlYWsgPGltcmUuZGVha0BpbnRlbC5jb20+Cj4gPiA+ID4gPiAtLS0KPiA+ID4gPiA+IMKgZHJp dmVycy9ncHUvZHJtL2k5MTUvaTkxNV9kcnYuYyB8IDkgKysrKysrKystCj4gPiA+ID4gPiDCoDEg ZmlsZSBjaGFuZ2VkLCA4IGluc2VydGlvbnMoKyksIDEgZGVsZXRpb24oLSkKPiA+ID4gPiA+IAo+ ID4gPiA+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2k5MTVfZHJ2LmMKPiA+ ID4gPiA+IGIvZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9kcnYuYwo+ID4gPiA+ID4gaW5kZXgg ZDU1MGFlMi4uN2VhYTkzZSAxMDA2NDQKPiA+ID4gPiA+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9p OTE1L2k5MTVfZHJ2LmMKPiA+ID4gPiA+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2k5MTVf ZHJ2LmMKPiA+ID4gPiA+IEBAIC04MDMsNyArODAzLDcgQEAgc3RhdGljIGludCBpOTE1X2RybV9y ZXN1bWUoc3RydWN0IGRybV9kZXZpY2UKPiA+ID4gPiA+ICpkZXYpCj4gPiA+ID4gPiDCoHN0YXRp YyBpbnQgaTkxNV9kcm1fcmVzdW1lX2Vhcmx5KHN0cnVjdCBkcm1fZGV2aWNlICpkZXYpCj4gPiA+ ID4gPiDCoHsKPiA+ID4gPiA+IMKgCXN0cnVjdCBkcm1faTkxNV9wcml2YXRlICpkZXZfcHJpdiA9 IGRldi0+ZGV2X3ByaXZhdGU7Cj4gPiA+ID4gPiAtCWludCByZXQgPSAwOwo+ID4gPiA+ID4gKwlp bnQgcmV0Owo+ID4gPiA+ID4gwqAKPiA+ID4gPiA+IMKgCS8qCj4gPiA+ID4gPiDCoAnCoCogV2Ug aGF2ZSBhIHJlc3VtZSBvcmRlcmluZyBpc3N1ZSB3aXRoIHRoZSBzbmQtaGRhIGRyaXZlcgo+ID4g PiA+ID4gYWxzbwo+ID4gPiA+ID4gQEAgLTgxNCw2ICs4MTQsMTMgQEAgc3RhdGljIGludCBpOTE1 X2RybV9yZXN1bWVfZWFybHkoc3RydWN0Cj4gPiA+ID4gPiBkcm1fZGV2aWNlICpkZXYpCj4gPiA+ ID4gPiDCoAnCoCogRklYTUU6IFRoaXMgc2hvdWxkIGJlIHNvbHZlZCB3aXRoIGEgc3BlY2lhbCBo ZG1pIHNpbmsKPiA+ID4gPiA+IGRldmljZSBvcgo+ID4gPiA+ID4gwqAJwqAqIHNpbWlsYXIgc28g dGhhdCBwb3dlciBkb21haW5zIGNhbiBiZSBlbXBsb3llZC4KPiA+ID4gPiA+IMKgCcKgKi8KPiA+ ID4gPiA+ICsKPiA+ID4gPiA+ICsJcmV0ID0gcGNpX3NldF9wb3dlcl9zdGF0ZShkZXYtPnBkZXYs IFBDSV9EMCk7Cj4gPiA+ID4gPiArCWlmIChyZXQpIHsKPiA+ID4gPiA+ICsJCURSTV9FUlJPUigi ZmFpbGVkIHRvIHNldCBQQ0kgRDAgcG93ZXIgc3RhdGUKPiA+ID4gPiA+ICglZClcbiIsIHJldCk7 Cj4gPiA+ID4gPiArCQlnb3RvIG91dDsKPiA+ID4gPiA+ICsJfQo+ID4gPiA+IAo+ID4gPiA+IEht bS4gRG9lc24ndCB0aGlzIGFscmVhZHkgaGFwcGVuIGZyb20gcGNpIGJ1cyByZXN1bWVfbm9pcnEg aG9vaz8KPiA+ID4gCj4gPiA+IEl0IGRvZXMsIGJ1dCBub3QgZHVyaW5nIHRoYXdfbm9pcnEuCj4g PiAKPiA+IE1heWJlIHB1dCB0aGF0IGludG8gYSBjb21tZW50PyBJZiB3ZSBldmVyIGdldCB0byBk cm9wcGluZyB0aGUgZGV2aWNlCj4gPiBzdGF0ZSBmcm9iYmVyeSBmcm9tIGZyZWV6ZS90aGF3LCB0 aGVuIHdlIHNob3VsZCBhbHNvIGJlIGFibGUgdG8gdGhyb3cKPiA+IG91dCB0aGUgcGNpX3NldF9w b3dlcl9zdGF0ZSgpIGNhbGwgYXMgd2VsbC4KPiAKPiBZZXMsIGNhbiBhZGQgYSBjb21tZW50Lgo+ IAo+ID4gUGVyaGFwcyB3ZSBzaG91bGQgaGF2ZSBzb21lIGFzc2VydHMgYWJvdXQgdGhlIHN0YXRl IG9mIHRoZSBQQ0kgZGV2aWNlPwo+IAo+IFlvdSBtZWFuIGFmdGVyIGNhbGxpbmcgcGNpX2VuYWJs ZV9kZXZpY2UoKSB0aGF0IGl0J3MgaW5kZWVkIGluIEQwIGFuZAo+IGVuYWJsZWQ/IENhbiBkbyB0 aGF0IGFzIGEgZm9sbG93LXVwLgoKWWVhaCwgd2FzIHRoaW5raW5nIHRoYXQgd2UgY291bGQgYXNz ZXJ0IHRoYXQgd2UncmUgaW4gRDAsIHJlcXVpcmVkIEJBUnMsCkJNLCBwY2kgaW50L21zaSBldGMu IGFyZSBlbmFibGVkLgoKLS0gClZpbGxlIFN5cmrDpGzDpApJbnRlbCBPVEMKX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4IG1haWxpbmcgbGlz dApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0 b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com ([134.134.136.24]:35275 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751583AbcDRJEJ (ORCPT ); Mon, 18 Apr 2016 05:04:09 -0400 Date: Mon, 18 Apr 2016 12:04:03 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Imre Deak Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org Subject: Re: [PATCH 2/4] drm/i915: Fix system resume if PCI device remained enabled Message-ID: <20160418090403.GC4329@intel.com> References: <1460963062-13211-1-git-send-email-imre.deak@intel.com> <1460963062-13211-3-git-send-email-imre.deak@intel.com> <20160418082822.GY4329@intel.com> <1460968358.3172.17.camel@intel.com> <20160418084451.GB4329@intel.com> <1460969671.3172.25.camel@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1460969671.3172.25.camel@intel.com> Sender: stable-owner@vger.kernel.org List-ID: On Mon, Apr 18, 2016 at 11:54:31AM +0300, Imre Deak wrote: > On ma, 2016-04-18 at 11:44 +0300, Ville Syrj�l� wrote: > > On Mon, Apr 18, 2016 at 11:32:38AM +0300, Imre Deak wrote: > > > On ma, 2016-04-18 at 11:28 +0300, Ville Syrj�l� 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; > > > > > + } > > > > > > > > Hmm. Doesn't this already happen from pci bus resume_noirq hook? > > > > > > It does, but not during thaw_noirq. > > > > Maybe put that into a comment? If we ever get to dropping the device > > state frobbery from freeze/thaw, then we should also be able to throw > > out the pci_set_power_state() call as well. > > Yes, can add a comment. > > > Perhaps we should have some asserts about the state of the PCI device? > > You mean after calling pci_enable_device() that it's indeed in D0 and > enabled? Can do that as a follow-up. Yeah, was thinking that we could assert that we're in D0, required BARs, BM, pci int/msi etc. are enabled. -- Ville Syrj�l� Intel OTC