From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: [PATCH v2 2/4] drm/i915: Fix system resume if PCI device remained enabled Date: Mon, 18 Apr 2016 14:45:54 +0300 Message-ID: <1460979954-14503-1-git-send-email-imre.deak@intel.com> References: <1460963062-13211-3-git-send-email-imre.deak@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTP id 6D7D56E5B6 for ; Mon, 18 Apr 2016 11:45:57 +0000 (UTC) In-Reply-To: <1460963062-13211-3-git-send-email-imre.deak@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: intel-gfx@lists.freedesktop.org Cc: stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org RHVyaW5nIHN5c3RlbSByZXN1bWUgd2UgZGVwZW5kZWQgb24gcGNpX2VuYWJsZV9kZXZpY2UoKSBh bHNvIHB1dHRpbmcgdGhlCmRldmljZSBpbnRvIFBDSSBEMCBzdGF0ZS4gVGhpcyB3b24ndCB3b3Jr IGlmIHRoZSBQQ0kgZGV2aWNlIHdhcyBhbHJlYWR5CmVuYWJsZWQgYnV0IHN0aWxsIGluIEQzIHN0 YXRlLiBUaGlzIGlzIGJlY2F1c2UgcGNpX2VuYWJsZV9kZXZpY2UoKSBpcwpyZWZjb3VudGVkIGFu ZCB3aWxsIG5vdCBjaGFuZ2UgdGhlIEhXIHN0YXRlIGlmIGNhbGxlZCB3aXRoIGEgbm9uLXplcm8K cmVmY291bnQuIExlYXZpbmcgdGhlIGRldmljZSBpbiBEMyB3aWxsIG1ha2UgYWxsIHN1YnNlcXVl bnQgZGV2aWNlCmFjY2Vzc2VzIGZhaWwuCgpUaGlzIGRpZG4ndCBjYXVzZSBhIHByb2JsZW0gbW9z dCBvZiB0aGUgdGltZSwgc2luY2Ugd2UgcmVzdW1lZCB3aXRoIGFuCmVuYWJsZSByZWZjb3VudCBv ZiAwLiBCdXQgaXQgZmFpbHMgYXQgbGVhc3QgYWZ0ZXIgbW9kdWxlIHJlbG9hZCBiZWNhdXNlCmFm dGVyIHRoYXQgd2UgYWxzbyBoYXBwZW4gdG8gbGVhayBhIFBDSSBkZXZpY2UgZW5hYmxlIHJlZmVy ZW5jZTogRHVyaW5nCnByb2Jpbmcgd2UgY2FsbCBkcm1fZ2V0X3BjaV9kZXYoKSB3aGljaCB3aWxs IGVuYWJsZSB0aGUgUENJIGRldmljZSwgYnV0CmR1cmluZyBkZXZpY2UgcmVtb3ZhbCBkcm1fcHV0 X2RldigpIHdvbid0IGRpc2FibGUgaXQuIFRoaXMgaXMgYSBidWcgb2YKaXRzIG93biBpbiBEUk0g Y29yZSwgYnV0IHdpdGhvdXQgbXVjaCBoYXJtIGFzIGl0IG9ubHkgbGVhdmVzIHRoZSBQQ0kKZGV2 aWNlIGVuYWJsZWQuIEZpeGluZyBpdCBpcyBhbHNvIGEgYml0IG1vcmUgaW52b2x2ZWQsIGR1ZSB0 byBEUk0KbWlkLWxheWVyaW5nIGFuZCBiZWNhdXNlIGl0IGFmZmVjdHMgbm9uLWk5MTUgZHJpdmVy cyB0b28uIFRoZSBmaXggaW4KdGhpcyBwYXRjaCBpcyB2YWxpZCByZWdhcmRsZXNzIG9mIHRoZSBw cm9ibGVtIGluIERSTSBjb3JlLgoKdjI6Ci0gQWRkIGEgY29kZSBjb21tZW50IGFib3V0IHRoZSBy ZWxhdGlvbiBvZiB0aGlzIGZpeCB0byB0aGUgZnJlZXplL3RoYXcKICB2cy4gdGhlIHN1c3BlbmQv cmVzdW1lIHBoYXNlcy4gKFZpbGxlKQotIEFkZCBhIGNvZGUgY29tbWVudCBhYm91dCB0aGUgaW5j b25zaXN0ZW50IG9yZGVyaW5nIG9mIHNldCBwb3dlciBzdGF0ZQogIGFuZCBkZXZpY2UgZW5hYmxl IGNhbGxzLiAoQ2hyaXMpCgpDQzogVmlsbGUgU3lyasOkbMOkIDx2aWxsZS5zeXJqYWxhQGxpbnV4 LmludGVsLmNvbT4KQ0M6IENocmlzIFdpbHNvbiA8Y2hyaXNAY2hyaXMtd2lsc29uLmNvLnVrPgpD Qzogc3RhYmxlQHZnZXIua2VybmVsLm9yZwpTaWduZWQtb2ZmLWJ5OiBJbXJlIERlYWsgPGltcmUu ZGVha0BpbnRlbC5jb20+Ci0tLQogZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9kcnYuYyB8IDMy ICsrKysrKysrKysrKysrKysrKysrKysrKysrKysrKystCiAxIGZpbGUgY2hhbmdlZCwgMzEgaW5z ZXJ0aW9ucygrKSwgMSBkZWxldGlvbigtKQoKZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9p OTE1L2k5MTVfZHJ2LmMgYi9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pOTE1X2Rydi5jCmluZGV4IGQ1 NTBhZTIuLjNiNzllOTcgMTAwNjQ0Ci0tLSBhL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2k5MTVfZHJ2 LmMKKysrIGIvZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9kcnYuYwpAQCAtODAzLDcgKzgwMyw3 IEBAIHN0YXRpYyBpbnQgaTkxNV9kcm1fcmVzdW1lKHN0cnVjdCBkcm1fZGV2aWNlICpkZXYpCiBz dGF0aWMgaW50IGk5MTVfZHJtX3Jlc3VtZV9lYXJseShzdHJ1Y3QgZHJtX2RldmljZSAqZGV2KQog ewogCXN0cnVjdCBkcm1faTkxNV9wcml2YXRlICpkZXZfcHJpdiA9IGRldi0+ZGV2X3ByaXZhdGU7 Ci0JaW50IHJldCA9IDA7CisJaW50IHJldDsKIAogCS8qCiAJICogV2UgaGF2ZSBhIHJlc3VtZSBv cmRlcmluZyBpc3N1ZSB3aXRoIHRoZSBzbmQtaGRhIGRyaXZlciBhbHNvCkBAIC04MTQsNiArODE0 LDM2IEBAIHN0YXRpYyBpbnQgaTkxNV9kcm1fcmVzdW1lX2Vhcmx5KHN0cnVjdCBkcm1fZGV2aWNl ICpkZXYpCiAJICogRklYTUU6IFRoaXMgc2hvdWxkIGJlIHNvbHZlZCB3aXRoIGEgc3BlY2lhbCBo ZG1pIHNpbmsgZGV2aWNlIG9yCiAJICogc2ltaWxhciBzbyB0aGF0IHBvd2VyIGRvbWFpbnMgY2Fu IGJlIGVtcGxveWVkLgogCSAqLworCisJLyoKKwkgKiBOb3RlIHRoYXQgd2UgbmVlZCB0byBzZXQg dGhlIHBvd2VyIHN0YXRlIGV4cGxpY2l0bHksIHNpbmNlIHdlCisJICogcG93ZXJlZCBvZmYgdGhl IGRldmljZSBkdXJpbmcgZnJlZXplIGFuZCB0aGUgUENJIGNvcmUgd29uJ3QgcG93ZXIKKwkgKiBp dCBiYWNrIHVwIGZvciB1cyBkdXJpbmcgdGhhdy4gUG93ZXJpbmcgb2ZmIHRoZSBkZXZpY2UgZHVy aW5nCisJICogZnJlZXplIGlzIG5vdCBhIGhhcmQgcmVxdWlyZW1lbnQgdGhvdWdoLCBhbmQgZHVy aW5nIHRoZQorCSAqIHN1c3BlbmQvcmVzdW1lIHBoYXNlcyB0aGUgUENJIGNvcmUgbWFrZXMgc3Vy ZSB3ZSBnZXQgaGVyZSB3aXRoIHRoZQorCSAqIGRldmljZSBwb3dlcmVkIG9uLiBTbyBpbiBjYXNl IHdlIGNoYW5nZSBvdXIgZnJlZXplIGxvZ2ljIGFuZCBrZWVwCisJICogdGhlIGRldmljZSBwb3dl cmVkIHdlIGNhbiBhbHNvIHJlbW92ZSB0aGUgZm9sbG93aW5nIHNldCBwb3dlciBzdGF0ZQorCSAq IGNhbGwuCisJICovCisJcmV0ID0gcGNpX3NldF9wb3dlcl9zdGF0ZShkZXYtPnBkZXYsIFBDSV9E MCk7CisJaWYgKHJldCkgeworCQlEUk1fRVJST1IoImZhaWxlZCB0byBzZXQgUENJIEQwIHBvd2Vy IHN0YXRlICglZClcbiIsIHJldCk7CisJCWdvdG8gb3V0OworCX0KKworCS8qCisJICogTm90ZSB0 aGF0IHBjaV9lbmFibGVfZGV2aWNlKCkgZmlyc3QgZW5hYmxlcyBhbnkgcGFyZW50IGJyaWRnZQor CSAqIGRldmljZSBhbmQgb25seSB0aGVuIHNldHMgdGhlIHBvd2VyIHN0YXRlIGZvciB0aGlzIGRl dmljZS4gVGhlCisJICogYnJpZGdlIGVuYWJsaW5nIGlzIGEgbm9wIHRob3VnaCwgc2luY2UgYnJp ZGdlIGRldmljZXMgYXJlIHJlc3VtZWQKKwkgKiBmaXJzdC4gVGhlIG9yZGVyIG9mIGVuYWJsaW5n IHBvd2VyIGFuZCBlbmFibGluZyB0aGUgZGV2aWNlIGlzCisJICogaW1wb3NlZCBieSB0aGUgUENJ IGNvcmUgYXMgZGVzY3JpYmVkIGFib3ZlLCBzbyBoZXJlIHdlIHByZXNlcnZlIHRoZQorCSAqIHNh bWUgb3JkZXIgZm9yIHRoZSBmcmVlemUvdGhhdyBwaGFzZXMuCisJICoKKwkgKiBUT0RPOiBldmVu dHVhbGx5IHdlIHNob3VsZCByZW1vdmUgcGNpX2Rpc2FibGVfZGV2aWNlKCkgLworCSAqIHBjaV9l bmFibGVfZW5hYmxlX2RldmljZSgpIGZyb20gc3VzcGVuZC9yZXN1bWUuIER1ZSB0byBob3cgdGhl eQorCSAqIGRlcGVuZCBvbiB0aGUgZGV2aWNlIGVuYWJsZSByZWZjb3VudCB3ZSBjYW4ndCBhbnl3 YXkgZGVwZW5kIG9uIHRoZW0KKwkgKiBkaXNhYmxpbmcvZW5hYmxpbmcgdGhlIGRldmljZS4KKwkg Ki8KIAlpZiAocGNpX2VuYWJsZV9kZXZpY2UoZGV2LT5wZGV2KSkgewogCQlyZXQgPSAtRUlPOwog CQlnb3RvIG91dDsKLS0gCjIuNS4wCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fXwpJbnRlbC1nZnggbWFpbGluZyBsaXN0CkludGVsLWdmeEBsaXN0cy5mcmVl ZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5m by9pbnRlbC1nZngK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com ([192.55.52.120]:61686 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751451AbcDRLp6 (ORCPT ); Mon, 18 Apr 2016 07:45:58 -0400 From: Imre Deak To: intel-gfx@lists.freedesktop.org Cc: =?UTF-8?q?Ville=20Syrj=C3=A4l=C3=A4?= , Chris Wilson , stable@vger.kernel.org Subject: [PATCH v2 2/4] drm/i915: Fix system resume if PCI device remained enabled Date: Mon, 18 Apr 2016 14:45:54 +0300 Message-Id: <1460979954-14503-1-git-send-email-imre.deak@intel.com> In-Reply-To: <1460963062-13211-3-git-send-email-imre.deak@intel.com> References: <1460963062-13211-3-git-send-email-imre.deak@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: stable-owner@vger.kernel.org List-ID: 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. v2: - Add a code comment about the relation of this fix to the freeze/thaw vs. the suspend/resume phases. (Ville) - Add a code comment about the inconsistent ordering of set power state and device enable calls. (Chris) CC: Ville Syrjälä CC: Chris Wilson CC: stable@vger.kernel.org Signed-off-by: Imre Deak --- drivers/gpu/drm/i915/i915_drv.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_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 = dev->dev_private; - int ret = 0; + int ret; /* * We have a resume ordering issue with the snd-hda driver also @@ -814,6 +814,36 @@ 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. */ + + /* + * Note that we need to set the power state explicitly, since we + * powered off the device during freeze and the PCI core won't power + * 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 the + * device powered on. So in case we change our freeze logic and keep + * the device powered we can also remove the following set power state + * call. + */ + 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; + } + + /* + * 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 resumed + * 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 them + * disabling/enabling the device. + */ if (pci_enable_device(dev->pdev)) { ret = -EIO; goto out; -- 2.5.0