[PATCH 1/2] PCI/PM: Add needs_resume flag to avoid suspend complete optimization

[PATCH 1/2] PCI/PM: Add needs_resume flag to avoid suspend complete optimization

[Imre Deak]

commit 4d071c3238987325b9e50e33051a40d1cce311cc upstream.

Some drivers - like i915 - may not support the system suspend direct
complete optimization due to differences in their runtime and system
suspend sequence.  Add a flag that when set resumes the device before
calling the driver's system suspend handlers which effectively disables
the optimization.

Needed by a future patch fixing suspend/resume on i915.

Suggested by Rafael.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: <stable@vger.kernel.org> # v4.8
(rebased on v4.8, added kernel version to commit message stable tag)
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/pci/pci.c   | 3 ++-
 include/linux/pci.h | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index aab9d51..970ee70 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2099,7 +2099,8 @@ bool pci_dev_keep_suspended(struct pci_dev *pci_dev)
 
 	if (!pm_runtime_suspended(dev)
 	    || pci_target_state(pci_dev) != pci_dev->current_state
-	    || platform_pci_need_resume(pci_dev))
+	    || platform_pci_need_resume(pci_dev)
+	    || (pci_dev->dev_flags & PCI_DEV_FLAGS_NEEDS_RESUME))
 		return false;
 
 	/*
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0ab8359..ebeec21 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -178,6 +178,11 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
 	/* Get VPD from function 0 VPD */
 	PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
+	/*
+	 * Resume before calling the driver's system suspend hooks, disabling
+	 * the direct_complete optimization.
+	 */
+	PCI_DEV_FLAGS_NEEDS_RESUME = (__force pci_dev_flags_t) (1 << 11),
 };
 
 enum pci_irq_reroute_variant {
-- 
2.7.4

[PATCH 2/2] drm/i915: Prevent the system suspend complete optimization

[Imre Deak]

commit 6ab92afc95c9bd6877cb42e7b24f65be887a5440 upstream.

Since

commit bac2a909a096c9110525c18cbb8ce73c660d5f71
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Date:   Wed Jan 21 02:17:42 2015 +0100

    PCI / PM: Avoid resuming PCI devices during system suspend

PCI devices will default to allowing the system suspend complete
optimization where devices are not woken up during system suspend if
they were already runtime suspended. This however breaks the i915/HDA
drivers for two reasons:

- The i915 driver has system suspend specific steps that it needs to
  run, that bring the device to a different state than its runtime
  suspended state.

- The HDA driver's suspend handler requires power that it will request
  from the i915 driver's power domain handler. This in turn requires the
  i915 driver to runtime resume itself, but this won't be possible if the
  suspend complete optimization is in effect: in this case the i915
  runtime PM is disabled and trying to get an RPM reference returns
  -EACCESS.

Solve this by requiring the PCI/PM core to resume the device during
system suspend which in effect disables the suspend complete optimization.

Regardless of the above commit the optimization stayed disabled for DRM
devices until

commit d14d2a8453d650bea32a1c5271af1458cd283a0f
Author: Lukas Wunner <lukas@wunner.de>
Date:   Wed Jun 8 12:49:29 2016 +0200

    drm: Remove dev_pm_ops from drm_class

so this patch is in practice a fix for this commit. Another reason for
the bug staying hidden for so long is that the optimization for a device
is disabled if it's disabled for any of its children devices. i915 may
have a backlight device as its child which doesn't support runtime PM
and so doesn't allow the optimization either.  So if this backlight
device got registered the bug stayed hidden.

Credits to Marta, Tomi and David who enabled pstore logging,
that caught one instance of this issue across a suspend/
resume-to-ram and Ville who rememberd that the optimization was enabled
for some devices at one point.

The first WARN triggered by the problem:

[ 6250.746445] WARNING: CPU: 2 PID: 17384 at drivers/gpu/drm/i915/intel_runtime_pm.c:2846 intel_runtime_pm_get+0x6b/0xd0 [i915]
[ 6250.746448] pm_runtime_get_sync() failed: -13
[ 6250.746451] Modules linked in: snd_hda_intel i915 vgem snd_hda_codec_hdmi x86_pkg_temp_thermal intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul
snd_hda_codec_realtek snd_hda_codec_generic ghash_clmulni_intel e1000e snd_hda_codec snd_hwdep snd_hda_core ptp mei_me pps_core snd_pcm lpc_ich mei prime_
numbers i2c_hid i2c_designware_platform i2c_designware_core [last unloaded: i915]
[ 6250.746512] CPU: 2 PID: 17384 Comm: kworker/u8:0 Tainted: G     U  W       4.11.0-rc5-CI-CI_DRM_334+ #1
[ 6250.746515] Hardware name:                  /NUC5i5RYB, BIOS RYBDWi35.86A.0362.2017.0118.0940 01/18/2017
[ 6250.746521] Workqueue: events_unbound async_run_entry_fn
[ 6250.746525] Call Trace:
[ 6250.746530]  dump_stack+0x67/0x92
[ 6250.746536]  __warn+0xc6/0xe0
[ 6250.746542]  ? pci_restore_standard_config+0x40/0x40
[ 6250.746546]  warn_slowpath_fmt+0x46/0x50
[ 6250.746553]  ? __pm_runtime_resume+0x56/0x80
[ 6250.746584]  intel_runtime_pm_get+0x6b/0xd0 [i915]
[ 6250.746610]  intel_display_power_get+0x1b/0x40 [i915]
[ 6250.746646]  i915_audio_component_get_power+0x15/0x20 [i915]
[ 6250.746654]  snd_hdac_display_power+0xc8/0x110 [snd_hda_core]
[ 6250.746661]  azx_runtime_resume+0x218/0x280 [snd_hda_intel]
[ 6250.746667]  pci_pm_runtime_resume+0x76/0xa0
[ 6250.746672]  __rpm_callback+0xb4/0x1f0
[ 6250.746677]  ? pci_restore_standard_config+0x40/0x40
[ 6250.746682]  rpm_callback+0x1f/0x80
[ 6250.746686]  ? pci_restore_standard_config+0x40/0x40
[ 6250.746690]  rpm_resume+0x4ba/0x740
[ 6250.746698]  __pm_runtime_resume+0x49/0x80
[ 6250.746703]  pci_pm_suspend+0x57/0x140
[ 6250.746709]  dpm_run_callback+0x6f/0x330
[ 6250.746713]  ? pci_pm_freeze+0xe0/0xe0
[ 6250.746718]  __device_suspend+0xf9/0x370
[ 6250.746724]  ? dpm_watchdog_set+0x60/0x60
[ 6250.746730]  async_suspend+0x1a/0x90
[ 6250.746735]  async_run_entry_fn+0x34/0x160
[ 6250.746741]  process_one_work+0x1f2/0x6d0
[ 6250.746749]  worker_thread+0x49/0x4a0
[ 6250.746755]  kthread+0x107/0x140
[ 6250.746759]  ? process_one_work+0x6d0/0x6d0
[ 6250.746763]  ? kthread_create_on_node+0x40/0x40
[ 6250.746768]  ret_from_fork+0x2e/0x40
[ 6250.746778] ---[ end trace 102a62fd2160f5e6 ]---

v2:
- Use the new pci_dev->needs_resume flag, to avoid any overhead during
  the ->pm_prepare hook. (Rafael)

v3:
- Update commit message to reference the actual regressing commit.
  (Lukas)

v4:
- Rebase on v4 of patch 1/2.

Fixes: d14d2a8453d6 ("drm: Remove dev_pm_ops from drm_class")
References: https://bugs.freedesktop.org/show_bug.cgi?id=100378
References: https://bugs.freedesktop.org/show_bug.cgi?id=100770
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Marta Lofstedt <marta.lofstedt@intel.com>
Cc: David Weinehall <david.weinehall@linux.intel.com>
Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: linux-pci@vger.kernel.org
Cc: <stable@vger.kernel.org> # v4.8
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reported-and-tested-by: Marta Lofstedt <marta.lofstedt@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1493726649-32094-2-git-send-email-imre.deak@intel.com
(cherry picked from commit adfdf85d795f4d4f487b61ee0b169d64c6e19081)
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
(rebased on v4.8, corrected kernel version in commit message stable tag)
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5de36d8..d69dfc6 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1246,6 +1246,15 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto out_free_priv;
 
 	pci_set_drvdata(pdev, &dev_priv->drm);
+	/*
+	 * Disable the system suspend direct complete optimization, which can
+	 * leave the device suspended skipping the driver's suspend handlers
+	 * if the device was already runtime suspended. This is needed due to
+	 * the difference in our runtime and system suspend sequence and
+	 * becaue the HDA driver may require us to enable the audio power
+	 * domain during system suspend.
+	 */
+	pdev->dev_flags |= PCI_DEV_FLAGS_NEEDS_RESUME;
 
 	ret = i915_driver_init_early(dev_priv, ent);
 	if (ret < 0)
-- 
2.7.4

Re: [PATCH 1/2] PCI/PM: Add needs_resume flag to avoid suspend complete optimization

[Bjorn Helgaas]

On Tue, Jun 13, 2017 at 01:38:56PM +0300, Imre Deak wrote:
> commit 4d071c3238987325b9e50e33051a40d1cce311cc upstream.
> 
> Some drivers - like i915 - may not support the system suspend direct
> complete optimization due to differences in their runtime and system
> suspend sequence.  Add a flag that when set resumes the device before
> calling the driver's system suspend handlers which effectively disables
> the optimization.
> 
> Needed by a future patch fixing suspend/resume on i915.
> 
> Suggested by Rafael.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: <stable@vger.kernel.org> # v4.8
> (rebased on v4.8, added kernel version to commit message stable tag)
> Signed-off-by: Imre Deak <imre.deak@intel.com>

The signoff chain above is incorrect.  It suggests that I wrote this
patch, but I did not.

The chain on 4d071c323898 ("PCI/PM: Add needs_resume flag to avoid
suspend complete optimization") is:

    Signed-off-by: Imre Deak <imre.deak@intel.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
    Cc: stable@vger.kernel.org

which indicates that Imre wrote the patch, and I received it from him
and merged it into the PCI tree.

> ---
>  drivers/pci/pci.c   | 3 ++-
>  include/linux/pci.h | 5 +++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index aab9d51..970ee70 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2099,7 +2099,8 @@ bool pci_dev_keep_suspended(struct pci_dev *pci_dev)
>  
>  	if (!pm_runtime_suspended(dev)
>  	    || pci_target_state(pci_dev) != pci_dev->current_state
> -	    || platform_pci_need_resume(pci_dev))
> +	    || platform_pci_need_resume(pci_dev)
> +	    || (pci_dev->dev_flags & PCI_DEV_FLAGS_NEEDS_RESUME))
>  		return false;
>  
>  	/*
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 0ab8359..ebeec21 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -178,6 +178,11 @@ enum pci_dev_flags {
>  	PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
>  	/* Get VPD from function 0 VPD */
>  	PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
> +	/*
> +	 * Resume before calling the driver's system suspend hooks, disabling
> +	 * the direct_complete optimization.
> +	 */
> +	PCI_DEV_FLAGS_NEEDS_RESUME = (__force pci_dev_flags_t) (1 << 11),
>  };
>  
>  enum pci_irq_reroute_variant {
> -- 
> 2.7.4
> 

Re: [PATCH 1/2] PCI/PM: Add needs_resume flag to avoid suspend complete optimization

[Imre Deak]

On Tue, Jun 13, 2017 at 06:57:44AM -0500, Bjorn Helgaas wrote:
> On Tue, Jun 13, 2017 at 01:38:56PM +0300, Imre Deak wrote:
> > commit 4d071c3238987325b9e50e33051a40d1cce311cc upstream.
> > 
> > Some drivers - like i915 - may not support the system suspend direct
> > complete optimization due to differences in their runtime and system
> > suspend sequence.  Add a flag that when set resumes the device before
> > calling the driver's system suspend handlers which effectively disables
> > the optimization.
> > 
> > Needed by a future patch fixing suspend/resume on i915.
> > 
> > Suggested by Rafael.
> > 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: <stable@vger.kernel.org> # v4.8
> > (rebased on v4.8, added kernel version to commit message stable tag)
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> The signoff chain above is incorrect.  It suggests that I wrote this
> patch, but I did not.
> 
> The chain on 4d071c323898 ("PCI/PM: Add needs_resume flag to avoid
> suspend complete optimization") is:
> 
>     Signed-off-by: Imre Deak <imre.deak@intel.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>     Cc: stable@vger.kernel.org
> 
> which indicates that Imre wrote the patch, and I received it from him
> and merged it into the PCI tree.

Yes, I moved my signoff to the end, since I rebased the original patch
and updated the stable tag line to

Cc: <stable@vger.kernel.org> # v4.8

tag. What's the proper way of signing off these changes, have two
signed-off lines, keeping the original and add a new one at the end?

--Imre

> 
> > ---
> >  drivers/pci/pci.c   | 3 ++-
> >  include/linux/pci.h | 5 +++++
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index aab9d51..970ee70 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -2099,7 +2099,8 @@ bool pci_dev_keep_suspended(struct pci_dev *pci_dev)
> >  
> >  	if (!pm_runtime_suspended(dev)
> >  	    || pci_target_state(pci_dev) != pci_dev->current_state
> > -	    || platform_pci_need_resume(pci_dev))
> > +	    || platform_pci_need_resume(pci_dev)
> > +	    || (pci_dev->dev_flags & PCI_DEV_FLAGS_NEEDS_RESUME))
> >  		return false;
> >  
> >  	/*
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 0ab8359..ebeec21 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -178,6 +178,11 @@ enum pci_dev_flags {
> >  	PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
> >  	/* Get VPD from function 0 VPD */
> >  	PCI_DEV_FLAGS_VPD_REF_F0 = (__force pci_dev_flags_t) (1 << 8),
> > +	/*
> > +	 * Resume before calling the driver's system suspend hooks, disabling
> > +	 * the direct_complete optimization.
> > +	 */
> > +	PCI_DEV_FLAGS_NEEDS_RESUME = (__force pci_dev_flags_t) (1 << 11),
> >  };
> >  
> >  enum pci_irq_reroute_variant {
> > -- 
> > 2.7.4
> > 

Re: [PATCH 1/2] PCI/PM: Add needs_resume flag to avoid suspend complete optimization

[Bjorn Helgaas]

On Tue, Jun 13, 2017 at 7:41 AM, Imre Deak <imre.deak@intel.com> wrote:
> On Tue, Jun 13, 2017 at 06:57:44AM -0500, Bjorn Helgaas wrote:
>> On Tue, Jun 13, 2017 at 01:38:56PM +0300, Imre Deak wrote:
>> > commit 4d071c3238987325b9e50e33051a40d1cce311cc upstream.
>> >
>> > Some drivers - like i915 - may not support the system suspend direct
>> > complete optimization due to differences in their runtime and system
>> > suspend sequence.  Add a flag that when set resumes the device before
>> > calling the driver's system suspend handlers which effectively disables
>> > the optimization.
>> >
>> > Needed by a future patch fixing suspend/resume on i915.
>> >
>> > Suggested by Rafael.
>> >
>> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> > Cc: <stable@vger.kernel.org> # v4.8
>> > (rebased on v4.8, added kernel version to commit message stable tag)
>> > Signed-off-by: Imre Deak <imre.deak@intel.com>
>>
>> The signoff chain above is incorrect.  It suggests that I wrote this
>> patch, but I did not.
>>
>> The chain on 4d071c323898 ("PCI/PM: Add needs_resume flag to avoid
>> suspend complete optimization") is:
>>
>>     Signed-off-by: Imre Deak <imre.deak@intel.com>
>>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>     Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>     Cc: stable@vger.kernel.org
>>
>> which indicates that Imre wrote the patch, and I received it from him
>> and merged it into the PCI tree.
>
> Yes, I moved my signoff to the end, since I rebased the original patch
> and updated the stable tag line to
>
> Cc: <stable@vger.kernel.org> # v4.8
>
> tag. What's the proper way of signing off these changes, have two
> signed-off lines, keeping the original and add a new one at the end?

I don't actually know.  Your proposal sounds reasonable to me.