Re: [Intel-gfx] [PATCH 2/4] drm/i915: Fix system resume if PCI device remained enabled

[Imre Deak <imre.deak@intel.com>]

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