From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com ([134.134.136.20]:39797 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752438AbcDRLF7 (ORCPT ); Mon, 18 Apr 2016 07:05:59 -0400 Message-ID: <1460977553.3172.37.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 14:05:53 +0300 In-Reply-To: <20160418085211.GF10708@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> <1460967394.3172.11.camel@intel.com> <20160418082440.GE10708@nuc-i3427.alporthouse.com> <1460968625.3172.21.camel@intel.com> <20160418085211.GF10708@nuc-i3427.alporthouse.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: On ma, 2016-04-18 at 09:52 +0100, Chris Wilson wrote: > On Mon, Apr 18, 2016 at 11:37:05AM +0300, Imre Deak wrote: > > I don't know. I guess you mean that enabling the device first seems > > like the first logical step. To me enabling power is the first > > logical > > step and enabling the device itself is the second. > > It is not clear exactly, but what is clear is that > pci_enable_device() > sets the power state of this device only after doing so for the > bridge > hierachy. That bridge enabling in pci_enable_deivce() won't make a difference since the suspend/resume order is already defined so that bridge devices are suspended last and resumed first. Otoh, the current order is to call pci_set_power_state() first and then pci_enable_device() both for our device and all the rest of PCI drivers/devices that I checked so far. Note that we can't even change this order during the suspend/resume phase (as opposed to the freeze/thaw phase) since then it's the PCI core that imposes the order already. So if I would change this now as you suggest, we would have a different order during the suspend/resume and the freeze/thaw phases. > The order in this patch is inconsistent. I attribute this inconsistency to the sloppiness of the PCI API. I don't want to change the order in this fix, but I can follow up with a patch that removes the pci_disable_device()/pci_enable_device() from our suspend/resume hooks. We can't anyway depend on these doing anything, since they are dependent on the device enable refcount which is out of reach of the driver. I can also add now a code comment about this. --Imre