Re: [PATCH 1/6] PCI/PM: Respect pci_dev->skip_bus_pm in the .poweroff() path

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Sep 30, 2024 at 02:50:09PM -0500, Bjorn Helgaas wrote:
> On Thu, Sep 26, 2024 at 07:03:16PM +0300, Ville Syrjälä wrote:
> > On Wed, Sep 25, 2024 at 02:28:42PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Sep 25, 2024 at 05:45:21PM +0300, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > On some older laptops i915 needs to leave the GPU in
> > > > D0 when hibernating the system, or else the BIOS
> > > > hangs somewhere. Currently that is achieved by calling
> > > > pci_save_state() ahead of time, which then skips the
> > > > whole pci_prepare_to_sleep() stuff.
> 
> > > If there's a general requirement to leave all devices in D0 when
> > > hibernating, it would be nice to have have some documentation like an
> > > ACPI spec reference.
> > 
> > No, IIRC the ACPI spec even says that you must (or at least
> > should) put devices into D3. But the buggy BIOS on some old
> > laptops keels over when you do that. Hence we need this quirk.
> 
> Can we include a reference to this part of the ACPI spec

It's been years since I looked at that, but a quick trawl of the
ACPI 6.3 spec (what I had at hand) landed me this:

"7.4.2.5 System \_S4 State
 ...
 - Devices states are compatible with the current Power Resource states. In
   other words, all devices are in the D3 state when the system state is S4."

"16.1.6 Transitioning from the Working to the Sleeping State
 ...
 4. OSPM places all device drivers into their respective Dx state. If the
    device is enabled for wake, it enters the Dx state associated with the
    wake capability. If the device is not enabled to wake the system, it
    enters the D3 state."

> and some
> details on which laptops have this issue?

The known models are listed in a comment in i915 code (added
in the two mentioned commits), though I suspect there are
probably more because we couldn't find any obvious pattern
why these known models are affected.

> 
> I'm a little bit wary of changing the PCI core in a generic-looking
> way on the basis of some unspecified buggy old BIOS.  That feels like
> something we're likely to break in the future.
> 
> > > Or if this is some i915-specific thing, maybe a pointer to history
> > > like a lore or bugzilla reference.
> > 
> > The two relevant commits I can find are:
> > 
> > commit 54875571bbfd ("drm/i915: apply the PCI_D0/D3 hibernation
> > workaround everywhere on pre GEN6")
> > commit ab3be73fa7b4 ("drm/i915: gen4: work around hang during
> > hibernation")
> 
> Thanks, this feels like important history to include somewhere.
> 
> > > IIUC this is a cleanup that doesn't fix any known problem?  The
> > > overall diffstat doesn't make it look like a simplification, although
> > > it might certainly be cleaner somehow:
> > 
> > My main concern is that using pci_save_state() might cause the pci
> > code to deviate from the normal path in more ways than just skipping
> > the D0->D3 transition. And then we might end up constantly chasing
> > after driver/pci changes in order to match its behaviour.
> > 
> > Not to mention that having the pci_save_state() in the driver code
> > is clearly confusing a bunch of our developers.
> 
> I'm all in favor of removing pci_save_state() from drivers when
> possible.  I take it that this doesn't fix a functional issue.

No known issue so far.

But we are probably going to add eg. PME support at some point,
and the fact that pci_save_state() also skips pci_enable_wake()
makes me think we'd have to hand roll a lot more stuff in the
driver code if we keep using the pci_save_state(). Though I 
suppose we could do the pci_save_state() only on those
old systems which won't have PME anyway. 

-- 
Ville Syrjälä
Intel